Skip to content

Fee sponsor for eth tx - #3

Merged
hieuvubk merged 20 commits into
release/v0.5.xfrom
hieu/evm-fee-sponsor
Mar 17, 2026
Merged

Fee sponsor for eth tx#3
hieuvubk merged 20 commits into
release/v0.5.xfrom
hieu/evm-fee-sponsor

Conversation

@hieuvubk

@hieuvubk hieuvubk commented Mar 2, 2026

Copy link
Copy Markdown
Collaborator

Description

Add feesponsor module:

  • Adds a FeePayer address field to store the global fee sponsor account.
  • The fee payer is a normal account controlled by its owner, who grants fee allowances to users
  • Includes governance proposal SetFeePayer to set or update the fee payer address.

Refactor eth ante handler. Whenever user send a evm tx, it will:

  1. Checks if the sender has been granted a fee allowance by the FeePayer

    • If no grant exists, the sender pays the transaction fee
  2. 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

  3. Deducts the fee from the FeePayer account and updates the grant accordingly

@hieuvubk
hieuvubk requested a review from facs95 March 2, 2026 08:57
@hieuvubk hieuvubk changed the title Hieu/evm fee sponsor Fee sponsor for eth tx Mar 2, 2026

@facs95 facs95 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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:

  1. Remove all fmt.Println debug statements
  2. Fix balance verification logic when sponsor pays fees (sender still needs value-check; nil-check for fee payer account)
  3. Fix silent fallback when UseGrantedFees fails — sender balance was never verified for fees in the sponsor path
  4. Fix CanTransfer interaction with sponsorship
  5. Fix typos (haveSponpor, gloalFeePayer)
  6. Keep FeegrantKeeper as an interface for testability
  7. 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

Comment thread ante/evm/mono_decorator.go Outdated
Comment thread x/feesponsor/keeper/keeper.go Outdated
Comment thread x/feesponsor/keeper/msg_server.go Outdated
Comment thread ante/evm/mono_decorator.go Outdated
Comment thread ante/evm/mono_decorator.go Outdated
Comment thread ante/evm/mono_decorator.go Outdated
Comment thread ante/evm/mono_decorator.go
Comment thread ante/ante.go Outdated
FeeMarketKeeper anteinterfaces.FeeMarketKeeper
EvmKeeper anteinterfaces.EVMKeeper
FeegrantKeeper ante.FeegrantKeeper
FeegrantKeeper feegrantkeeper.Keeper

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

🟡 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.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

test fixed, We can not reuse ante interface cause it doesn't have Allowance for grant check

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

But we can exten the interface and use that instead here can't we?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

That inteface from cosmos-sdk import, unless we make an our own

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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

Comment thread ante/evm/06_account_verification.go
Comment thread ante/evm/mono_decorator.go
@hieuvubk

hieuvubk commented Mar 3, 2026

Copy link
Copy Markdown
Collaborator Author

@facs95 for testing pls reference realiotech/realio-network#333 for integration tests

@hieuvubk

hieuvubk commented Mar 3, 2026

Copy link
Copy Markdown
Collaborator Author

Added MsgRemoveFeePayer

Comment thread ante/evm/mono_decorator.go Outdated
Comment thread x/feesponsor/types/msg.go

@facs95 facs95 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Overall the design is sound — leveraging feegrant for allowance management is the right approach. A few issues to address before merging.

Comment thread ante/evm/mono_decorator.go
Comment thread ante/evm/mono_decorator.go
Comment thread ante/ante.go Outdated
Comment thread ante/ante.go Outdated
t.Run(tc.name, func(t *testing.T) {
td.keeper.SetFeePayerToStore(td.ctx, tc.feePayerAddr)

// Verify it was stored

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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.

Comment thread x/feesponsor/types/errors.go
Comment thread ante/evm/mono_decorator_test.go
@hieuvubk
hieuvubk marked this pull request as ready for review March 13, 2026 02:21
@hieuvubk
hieuvubk merged commit 2468eaf into release/v0.5.x Mar 17, 2026
14 of 23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants