Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Integrated the delegation factory #364

Merged
merged 5 commits into from
Dec 5, 2023
Merged

Conversation

popenta
Copy link
Collaborator

@popenta popenta commented Nov 24, 2023

Now using the DelegationFactory from sdk-py-core to perform delegation operations.
Also added tests.

@popenta popenta self-assigned this Nov 24, 2023
@popenta popenta marked this pull request as draft November 24, 2023 13:09
Base automatically changed from integrate-sc-factory to feat/next November 28, 2023 15:00
@popenta popenta marked this pull request as ready for review November 28, 2023 15:10
@andreibancioiu andreibancioiu self-requested a review November 29, 2023 10:20
"--arguments", "0",
"--send", "--wait-result"
])
assert True if return_code else False
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

assert return_code

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@@ -320,43 +318,6 @@ def deploy(args: Any):
_send_or_simulate(tx, contract_address, args)


def _prepare_sender(args: Any) -> Account:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to double/triple check - deployments still work with Ledger now, correct?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, I've changed the cli_shared.prepare_account() function, to load a LedgerAccount as previously done in the _prepare_sender() function. Now, since it's the same, I assume all operations should work using a Ledger device.



class DelegationOperations:
def __init__(self, config: IConfig) -> None:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can use the config class (the concrete class) directly, since we don't necessarily think of the components in mxpy as reusable objects (not a library). Can also stay as it is 👍

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've done it like this to match the SmartContract class. Will leave it as it is for now.

tx.version = int(args.version)
tx.options = int(args.options)
tx.guardian = args.guardian
tx.signature = bytes.fromhex(owner.sign_transaction(tx))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In other places, the sign_transaction() function is invoked in cli_....py files. But can also stay here.

Perhaps rename functions to have the prefix create_ instead of get_, since they create transactions? Or prepare.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Renamed with the prefix prepare_.


return validator_public_keys

def _get_public_keys_and_signed_messages(self, args: Any) -> Tuple[List[ValidatorPublicKey], List[bytes]]:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe some of the logic can be moved to ValidatorsFile? E.g. load_secret_keys()? Or even load_signers() - and the returned object (signers) allows us to both sign a message, and get the underlying public keys.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Made a function load_signers() inside the ValidatorsFile class that returns a list of ValidatorSigners.


return public_keys, signed_messages

def _get_pubkey_for_signing(self, args: Any) -> bytes:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_to_be_signed(), instead of _for_signing().

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Renamed.


return account.address.get_public_key()

def __get_empty_transaction(self) -> Transaction:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead, can we do else raise Error("...")?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That sounds good. Done!

andreibancioiu
andreibancioiu previously approved these changes Dec 4, 2023
signed_messages.append(signed_message)

return public_keys, signed_messages

def _get_pubkey_for_signing(self, args: Any) -> bytes:
def _get_pubkey_to_be_signed(self, args: Any) -> bytes:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now I see, this does not work with Ledger yet.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should work now.

@ssd04 ssd04 self-requested a review December 5, 2023 14:25
${CLI} --verbose validator stake --pem="${USERS}/bob.pem" --value="2500${DENOMINATION}" --validators-file=./testdata/validators.json --reward-address=${REWARD_ADDRESS} --chain=${CHAIN_ID} --proxy=${PROXY} --estimate-gas --nonce=300 --send || return 1
${CLI} --verbose validator stake --pem="${USERS}/bob.pem" --value="2500${DENOMINATION}" --validators-file=./testdata/validators.json --reward-address=${REWARD_ADDRESS} --chain=${CHAIN_ID} --proxy=${PROXY} --estimate-gas --nonce=300 || return 1
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why removing --send here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The nonce was previously set to a higher number so that the transaction would stay in the mempool. Since the nonce limit has been lowered, so that the protocol only accepts transactions with max_nonce=current_nonce + 100 and because the address used it's Bob's address which is a public one, the tests would keep failing on every PR.

@popenta popenta merged commit 463dc81 into feat/next Dec 5, 2023
9 checks passed
@popenta popenta deleted the integrate-delegation-factory branch December 5, 2023 16:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants