Conversation
There was a problem hiding this comment.
Overall Review
The core design of leveraging feegrant for EVM fee sponsorship is a reasonable approach. However, there are several correctness issues, leftover debug code, and edge cases that need to be addressed before this is mergeable. See inline comments for details.
Key items:
- Remove all
fmt.Printlndebug statements - Fix balance verification logic when sponsor pays fees (sender still needs value-check; nil-check for fee payer account)
- Fix silent fallback when
UseGrantedFeesfails — sender balance was never verified for fees in the sponsor path - Fix
CanTransferinteraction with sponsorship - Fix typos (
haveSponpor,gloalFeePayer) - Keep
FeegrantKeeperas an interface for testability - Add tests — the PR has zero test coverage for the new module and modified ante handler
Missing functionality: no way to unset/disable the fee payer once set (ValidateFeePayerAddress rejects empty strings). Also no tests. We need to have tests for this
ante/ante.go
Outdated
| FeeMarketKeeper anteinterfaces.FeeMarketKeeper | ||
| EvmKeeper anteinterfaces.EVMKeeper | ||
| FeegrantKeeper ante.FeegrantKeeper | ||
| FeegrantKeeper feegrantkeeper.Keeper |
There was a problem hiding this comment.
🟡 Changed from interface to concrete type. This was previously ante.FeegrantKeeper (interface). All other keepers in this struct use interfaces (anteinterfaces.EVMKeeper, etc.). Using a concrete type breaks dependency inversion and makes unit testing the ante handler significantly harder (can't mock the feegrant keeper). Consider defining an interface.
There was a problem hiding this comment.
test fixed, We can not reuse ante interface cause it doesn't have Allowance for grant check
There was a problem hiding this comment.
But we can exten the interface and use that instead here can't we?
There was a problem hiding this comment.
That inteface from cosmos-sdk import, unless we make an our own
There was a problem hiding this comment.
So thats what I am saying, you can make one on our repo, that extends the cosmos sdk. That way we keep things being interface based
|
@facs95 for testing pls reference realiotech/realio-network#333 for integration tests |
|
Added |
facs95
left a comment
There was a problem hiding this comment.
Overall the design is sound — leveraging feegrant for allowance management is the right approach. A few issues to address before merging.
| t.Run(tc.name, func(t *testing.T) { | ||
| td.keeper.SetFeePayerToStore(td.ctx, tc.feePayerAddr) | ||
|
|
||
| // Verify it was stored |
There was a problem hiding this comment.
Bug: setupFn captures wrong td variable
The "remove existing fee payer" test case creates a new testData (td := newTestData(t)) inside the loop, but the setupFn closure captures the outer td from the table-driven test range. The setup writes to a different keeper/context than the one used for the removal assertion.
The test passes by accident because removing a non-existent key is a no-op — it doesn't actually verify removal of an existing entry.
Fix: Either pass td into setupFn as a parameter, or don't re-create td inside the loop.
Description
Add
feesponsormodule:FeePayeraddress field to store the global fee sponsor account.Refactor eth ante handler. Whenever user send a evm tx, it will:
Checks if the sender has been granted a fee allowance by the FeePayer
If a grant exists, verifies that:
The FeePayer has sufficient balance to cover the transaction cost (excluding the transferred value)
The grant limit can cover the transaction cost
The grant has not expired
If any condition fails, the sender pays the transaction fee
Deducts the fee from the FeePayer account and updates the grant accordingly