Transactions creation refactoring & fixes#478
Conversation
| "--gas-limit", | ||
| "50000", | ||
| "--chain", | ||
| "integration tests chain ID", |
There was a problem hiding this comment.
Can be a little bit more machine-readable (for the sake of the example) e.g. test.
There was a problem hiding this comment.
The test was duplicated from an already existing test. Changed it.
| def get_guardian_address(guardian: Union[IAccount, None], args: Any) -> Union[Address, None]: | ||
| if guardian: | ||
| return guardian.address | ||
| address_pem = guardian.address if guardian else None |
There was a problem hiding this comment.
The suffix _pem is a bit ambiguous, I think. Maybe address_of_loaded_wallet (or something similar)?
address_from_account vs. address_from_args?
| def get_relayer_address(relayer: Union[IAccount, None], args: Any) -> Union[Address, None]: | ||
| if relayer: | ||
| return relayer.address | ||
| address_pem = relayer.address if relayer else None |
There was a problem hiding this comment.
Same comment as above, if applicable.
| if hasattr(args, "guardian") and args.guardian: | ||
| return Address.new_from_bech32(args.guardian) | ||
| if address_pem and address_arg and address_pem != address_arg: | ||
| raise IncorrectWalletError("Guardian wallet does not match the guardian's address set in the transaction.") |
There was a problem hiding this comment.
Good check!
"... set in the transaction ..." - maybe replace this with, say, "... set in the arguments ..."? Or the one from the arguments gets to be the one "in the transaction", actually?
💭
There was a problem hiding this comment.
If the addresses don't match, no address is used since an error is raised. Changed to ... set in the arguments.
|
|
||
| def get_transaction(args: Any): | ||
| args = utils.as_object(args) | ||
| # args = utils.as_object(args) |
| tx.guardian_signature = guardian_account.sign_transaction(tx) | ||
| elif args.guardian: | ||
| tx = cosign_transaction(tx, args.guardian_service_url, args.guardian_2fa_code) | ||
| cosign_transaction(tx, args.guardian_service_url, args.guardian_2fa_code) |
There was a problem hiding this comment.
Maybe add some comments around if guardian_account etc. E.g. if the guardian account is provided, we sign locally. Otherwise, we reach for the trusted cosign service.
💭
There was a problem hiding this comment.
Now sure, at some point can we use the transactions controller here, as well (e.g. the if-else from above is implemented in the base controller's sign facility, as well)?
💭
There was a problem hiding this comment.
added the comment. Indeed, I think the BaseTransactionsController can be used and will be used in a later PR.
| options: int, | ||
| guardian: Union[Address, None], | ||
| relayer: Union[Address, None], | ||
| guardian_account: Optional[IAccount] = None, |
| # fmt: on | ||
|
|
||
|
|
||
| class TransactionsController(BaseTransactionsController): |
There was a problem hiding this comment.
Good refactoring! args isn't to be handled by this component, indeed, it's a concern of the "CLI" components (closer to "presentation layer") 👍
| guardian_service_url: str = "", | ||
| guardian_2fa_code: str = "", | ||
| ): | ||
| """Signs the transaction with the sender's account and if necessarry, signs with guardian's account and relayer's account. Also sets proper transaction options if needed.""" |
There was a problem hiding this comment.
Signs the transaction using the sender's account and, if required, additionally signs with the guardian's and relayer's accounts. Ensures the appropriate transaction options are set as needed.
| hrp = config.get_address_hrp() | ||
|
|
||
| if args.pem: | ||
| return Account.new_from_pem(file_path=Path(args.pem), index=args.pem_index, hrp=hrp) |
There was a problem hiding this comment.
Just a question: when loading a key from a PEM file, shouldn't we learn the HRP directly from the comment within the PEM file (i.e. the label)? Oh, I don't think the SDK is wired that way anyway 💭
There was a problem hiding this comment.
Indeed, the sdk is not wired that way.
| ) | ||
| cli_shared.add_wallet_args(args, sub) | ||
| # we add the wallet args, but don't make the args mandatory | ||
| cli_shared.add_wallet_args(args, sub, True) |
There was a problem hiding this comment.
Can use the named parameter, skip_...=True.
There was a problem hiding this comment.
Will do in the next PR.
3 participants
No description provided.