Remove feegrant module - #3958
Conversation
Remove the feegrant module and its keeper, store, CLI, APIs, protobuf definitions, simulation support, and application wiring. * Delete the feegrant store during the v6.7 upgrade. * Reject transactions specifying a distinct fee granter. * Retain the transaction granter field for wire compatibility. * Retire precompile 0x1010 with a deterministic revert. * Remove feegrant from SeiDB, wasmd, tooling, and Swagger. * Preserve historical genesis files unchanged. * Regenerate embedded API documentation and update tests.
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3958 +/- ##
==========================================
- Coverage 58.61% 57.50% -1.11%
==========================================
Files 2323 2224 -99
Lines 198666 186991 -11675
==========================================
- Hits 116442 107528 -8914
+ Misses 71469 69627 -1842
+ Partials 10755 9836 -919
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
# Conflicts: # precompiles/feegrant/versions
PR SummaryHigh Risk Overview The v6.7 upgrade deletes the Docs/README no longer advertise fee grants. Ante, CheckTx/DeliverTx, and SeiDB store-key lists are unwired from feegrant. Reviewed by Cursor Bugbot for commit 0fa29cb. Bugbot is set up for automated code reviews on this repo. Configure here. |
There was a problem hiding this comment.
Clean, thorough removal of the feegrant module: the fee-granter rejection is correctly placed at a single choke point (Tx.ValidateBasic, reached by both CosmosStatelessChecks and ValidateBasicDecorator), the v6.7 module-version delete is needed because SetModuleVersionMap is additive, and no dangling references remain. No blockers; a few non-blocking notes on flatkv store deletion, ante-level test coverage, historical tx decodability, and the 0x1010 "deterministic revert" claim.
Findings: 0 blocking | 5 non-blocking | 2 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- [suggestion] The new granter rejection is only exercised at the
tx.ValidateBasic()level (TestRejectsDistinctFeeGranter). Both fee-deduction sites that previously rejected a distinct granter —DeductFeeDecorator.checkDeductFeeandapp/ante.chargeFees— now silently deduct from the fee payer when a granter is set, so their safety rests entirely on ValidateBasic running first. An end-to-end case throughCosmosCheckTxAnte/CosmosDeliverTxAnte(and note thatapp/ante.goordersDeductFeeDecoratorbeforeNewValidateBasicDecorator) would pin the actual rejection rather than the helper. - [suggestion] Dropping
feegrantmodule.AppModuleBasic{}fromModuleBasicsunregisterscosmos.feegrant.v1beta1from the interface registry, so historical transactions containingMsgGrantAllowance/MsgRevokeAllowance(feegrant was live on pacific-1) become undecodable byseid q tx, the/cosmos/tx/v1beta1/txsservice, and anything else that decodes archived txs. Worth calling out explicitly in the CHANGELOG upgrade guide alongside the CLI/API removals. - [suggestion] Removing
--fee-accountfromAddTxFlagsToCmd(sei-cosmos/client/flags/flags.go) makes any existing script that passes it fail withunknown flagrather than a deprecation message. The CHANGELOG covers "CLI ... removal" generically; naming the flag would save operators a bisect. - 2 suggestion(s)/nit(s) flagged inline on specific lines.
|
|
||
| if upgradeInfo.Name == "v6.7" && !app.UpgradeKeeper.IsSkipHeight(upgradeInfo.Height) { | ||
| storeUpgrades := storetypes.StoreUpgrades{ | ||
| Deleted: []string{feegrantModuleName}, |
There was a problem hiding this comment.
[suggestion] CompositeCommitStore.ApplyUpgrades (sei-db/state_db/sc/composite/store.go:751) forwards only to memIAVL — its doc comment states "Data in flatKV is not affected by this method" — and returns early when cs.memIAVL == nil. Under the memIAVL-backed modes this deletion works as intended, but for write modes where non-EVM stores have moved to flatkv (MigrateAllButBank, AllMigratedButBank, FlatKVOnly), the feegrant keys survive the upgrade and keep contributing to the flatkv lattice hash.
This is the first store deletion since flatkv landed (the dex/accesscontrol precedents predate it), so it's worth confirming which write modes will be live at the v6.7 height, and whether orphaned feegrant data in flatkv is acceptable or needs an explicit cleanup.
| func TestFeegrantPrecompileIsNotRegistered(t *testing.T) { | ||
| const feegrantAddress = "0x0000000000000000000000000000000000001010" | ||
|
|
||
| _, exists := precompiles.GetCustomPrecompiles("v6.7", &utils.EmptyKeepers{})[common.HexToAddress(feegrantAddress)] |
There was a problem hiding this comment.
[suggestion] The PR description says "Retire precompile 0x1010 with a deterministic revert," but nothing implements a revert — 0x1010 simply becomes an unassigned address, so an EVM CALL to it succeeds with empty returndata, and this test only asserts absence from the map rather than call behavior.
Unlike Codex's read of this, I don't think it's blocking: feegrant.GetVersioned registered the precompile only under latestUpgrade, and v6.7 has not activated on any network, so there is no released height at which 0x1010 behaved differently. Either add a tombstone precompile that always reverts and assert the call result, or drop the "deterministic revert" claim from the description so the merge commit matches the code.
|
Running xreview locally |
| if upgradeInfo.Name == "v6.7" && !app.UpgradeKeeper.IsSkipHeight(upgradeInfo.Height) { | ||
| storeUpgrades := storetypes.StoreUpgrades{ | ||
| Deleted: []string{feegrantModuleName}, |
| cmd.Flags().String(FlagKeyringBackend, DefaultKeyringBackend, "Select keyring's backend (os|file|kwallet|pass|test|memory)") | ||
| cmd.Flags().String(FlagSignMode, "", "Choose sign mode (direct|amino-json), this is an advanced feature") | ||
| cmd.Flags().Uint64(FlagTimeoutHeight, 0, "Set a block timeout height to prevent the tx from being committed past a certain height") | ||
| cmd.Flags().String(FlagFeeAccount, "", "Fee account pays fees for the transaction instead of deducting from the signer") |
There was a problem hiding this comment.
|
|
||
| var AllModules = []string{ | ||
| "evm", "wasm", "oracle", "epoch", "mint", "acc", "bank", "feegrant", "staking", "distribution", "slashing", "gov", "params", "ibc", "upgrade", "evidence", "transfer", "tokenfactory", | ||
| "evm", "wasm", "oracle", "epoch", "mint", "acc", "bank", "staking", "distribution", "slashing", "gov", "params", "ibc", "upgrade", "evidence", "transfer", "tokenfactory", |
There was a problem hiding this comment.
@codchen we probably want to remove oracle in https://github.com/sei-protocol/sei-chain/pull/3944/changes#diff-b4430a7436ccec6a9ef7972cecc645769378a358db50314c67b0e00512354664R311 as well
Remove the feegrant module and its keeper, store, CLI, APIs, protobuf definitions, simulation support, and application wiring.