feat: Fee granting [WiP]#412
Conversation
jrriehl
left a comment
jrriehl
left a comment
There was a problem hiding this comment.
LGTM, although it would probably make sense to include these in some test cases.
|
Visit the preview URL for this PR (updated for commit 1a0e099): https://fetch-docs-preview--pr412-feat-fee-grant-wpthqm1d.web.app (expires Sun, 11 May 2025 11:53:24 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: f2de39fd4e81249941960b74fbab0a62d90d69f8 |
| Fee( | ||
| amount=parse_coins(amount), | ||
| gas_limit=gas_limit, | ||
| granter=granter, |
There was a problem hiding this comment.
I will have to double check this, it was None in previous implementation.
There was a problem hiding this comment.
Granter is the external wallet with funds, so payer can be None. Payer is namedgrantee in newer verision of CosmosSDK.
| return ( | ||
| Fee( | ||
| amount=parse_coins(amount), | ||
| gas_limit=gas_limit, |
There was a problem hiding this comment.
gas_limit can be None
previous code was gracefully handling None case
There was a problem hiding this comment.
I've reworked the code, it is much cleaner now and fixes that problem.
| amount: Optional[str] = None, | ||
| gas_limit: Optional[int] = None, | ||
| granter: Optional[Address] = None, | ||
| account: Optional["Account"] = None, # type: ignore # noqa: F821 |
There was a problem hiding this comment.
Question out of ignorance: what is the difference between account and sender? Thanks
I can see below that account can be set from sender
There was a problem hiding this comment.
Account carries sequence and account_id and needs to be queried from node, that's why I am avoiding to re-query it.
| granter=granter, | ||
| payer=sender.address(), | ||
| ), | ||
| account, |
There was a problem hiding this comment.
is it ok for the account inside the Fee to be None?
Otherwise, I can see that the account for signing transaction will be set correctly
There was a problem hiding this comment.
Looks like previously it was None, I think this would apply only for multi-msg transactions from different wallets where you need to specify which wallet will be paying.
|
Closed in favour of #413 |
3 participants
Proposed Changes
[describe the changes here...]
Linked Issues
[if applicable, add links to issues resolved by this PR]
Types of changes
What type of change does this pull request make (put an
xin the boxes that apply)?Checklist
Put an
xin the boxes that apply:If applicable
Further comments
[if this is a relatively large or complex change, kick off a discussion by explaining why you chose the solution you did, what alternatives you considered, etc...]