feat(precompiles): expose module Msg rpcs as precompile methods - #3825
feat(precompiles): expose module Msg rpcs as precompile methods#3825codchen wants to merge 8 commits into
Conversation
Follow-up to #3767: add transaction methods for the module Msg rpcs that had no EVM precompile counterpart — bank multiSend, distribution fundCommunityPool, addr associateContractAddress, authz grant/exec/revoke, feegrant grantAllowance/revokeAllowance, and slashing unjail. evidence SubmitEvidence and vesting CreateVestingAccount are intentionally omitted (no evidence router is set, and the vesting module is deprecated); oracle and tokenfactory are out of scope. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
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 #3825 +/- ##
==========================================
- Coverage 60.24% 59.28% -0.96%
==========================================
Files 2332 2240 -92
Lines 195184 184698 -10486
==========================================
- Hits 117586 109498 -8088
+ Misses 66953 65404 -1549
+ Partials 10645 9796 -849
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
PR SummaryMedium Risk Overview The handler resolves the validator operator from the caller’s associated Sei address (same idea as distribution’s operator-only txs), rejects staticcall, delegatecall, and payable calls, and marks Reviewed by Cursor Bugbot for commit da9e73c. Bugbot is set up for automated code reviews on this repo. Configure here. |
![seidroid[bot]](https://avatars.githubusercontent.com/in/2745135?s=60&v=4){kind=small}
There was a problem hiding this comment.
Well-structured, consistent extension of five precompiles with real-keeper tests; the msg-server plumbing, delegatecall/staticcall guards, fundCommunityPool's payable pattern, and the re-implemented authz nested-EVM/depth ante checks all match existing conventions and the sei-cosmos APIs they call. One blocking issue: bank.multiSend is gated on the caller being the denom's native ERC20 pointer, but the only contract that can hold that role never calls it, so the method is unreachable on-chain; separately, authz.exec quietly exposes the entire cosmos msg router to the EVM, including modules this PR intentionally leaves out.
Findings: 2 blocking | 9 non-blocking | 6 posted inline
Blockers
bank.multiSendhas no reachable caller on-chain (details in the inline comment onprecompiles/bank/bank.go). Since this is a consensus-visible ABI addition, it should either ship together with the pointer-contract entry point that calls it, or the caller restriction/rationale should be reconsidered — as written the method can only be exercised from Go tests that fake the pointer address.- 1 blocking issue(s) flagged inline on specific lines.
Non-blocking
- Test gaps: (a) the
maxNestedMsgsdepth cap incheckNoNestedEVMMessagesis never exercised (>5 nestedMsgExec) — the test only covers one level of nesting with an EVM msg; (b) no test covers theexecpath where the inner msg's signer is the caller (no grant needed), which is the widest new capability; (c)multiSendis only tested against a mock pointer address, so nothing validates it end-to-end from a pointer contract. authz/feegrant/slashingreject delegatecall only for the new tx methods (checked after the query switch), whilegov/distribution/pointerreject delegatecall for the whole precompile. Not a bug, but the inconsistency is worth a comment or normalizing.- In
multiSend,total = total.Add(amount)usessdk.Int, which panics if the sum exceeds 256 bits (reachable with two largeuint256amounts). The panic is caught by theExecute-levelrecover()and surfaces asexecution reverted, so it's safe, but an explicit overflow check would give a clearer error and not consume all gas. - Second-opinion passes: Codex reported "No material issues found";
cursor-review.mdis empty, i.e. the Cursor pass produced no output — its coverage should not be assumed. - 5 suggestion(s)/nit(s) flagged inline on specific lines.
| if denom == "" { | ||
| return nil, 0, errors.New("invalid denom") | ||
| } | ||
| pointer, _, exists := p.evmKeeper.GetERC20NativePointer(ctx, denom) |
There was a problem hiding this comment.
[blocker] multiSend looks unreachable on-chain as shipped. The gate requires caller == GetERC20NativePointer(denom), and a native ERC20 pointer address is only ever set by UpsertERCNativePointer (via the pointer precompile's addNativePointer, HandleAddERCNativePointerProposalV2, and pointer_upgrade.go), which always deploys the chain's own NativeSeiTokensERC20 artifact. That contract only ever calls BankPrecompile.send(...) in _update — there is no batch-transfer entry point, and contracts/src/precompiles/IBank.sol wasn't extended either. So no address that can satisfy this check has code that calls multiSend; the only caller in existence is TestMultiSend, which registers a MockAddressPair EOA as the pointer.
If the intent is to land the precompile side first and add the pointer-contract path (plus a new native artifact version) in a follow-up, please say so explicitly — otherwise this adds a consensus-visible ABI method that can never be invoked.
| // authz grantee. Messages whose signer is not the caller require a matching | ||
| // grant. EVM messages are rejected, mirroring the cosmos ante handler's | ||
| // restriction on nested EVM messages in MsgExec. | ||
| func (p PrecompileExecutor) exec(ctx sdk.Context, method *abi.Method, caller common.Address, args []interface{}, value *big.Int) ([]byte, uint64, error) { |
There was a problem hiding this comment.
[suggestion] exec is materially broader than the other methods in this PR. Keeper.DispatchActions implicitly accepts any message whose single signer equals the grantee, so with no grant at all a caller can dispatch any message registered on the MsgServiceRouter from their own Sei address, inside an EVM call frame. That includes exactly the msgs this PR deliberately excludes — oracle and tokenfactory ("out of scope"), vesting CreateVestingAccount, evidence SubmitEvidence — plus x/evm's own MsgSend, MsgRegisterPointer, and MsgAssociate*. Only two ante decorators are re-implemented here (nested-EVM rejection and depth cap); everything else in app/ante.go is bypassed, and per-module precompile guards (e.g. wasmd's pointer/delegatecall checks) are sidestepped for the same operations.
The state transitions are already reachable by the same actors via a cosmos MsgExec tx, so I don't think this is privilege escalation — but making the entire msg router EVM-reachable is a much bigger surface decision than "expose the authz Msg rpcs," and it silently undoes this PR's own scoping choices. Consider an explicit allow/deny list of dispatchable msg type URLs (or at minimum documenting the decision in the PR/Authz.sol).
| msgs[i] = m | ||
| } | ||
|
|
||
| if err := checkNoNestedEVMMessages(msgs, 0); err != nil { |
There was a problem hiding this comment.
[suggestion] MsgExec.ValidateBasic() in sei-cosmos only checks the grantee address and that Msgs is non-empty — it does not validate the inner messages. So attacker-supplied inner msgs go straight to DispatchActions, whose first step is msg.GetSigners(); most msg types (banktypes.MsgSend, authz.MsgGrant, …) panic there on a malformed bech32 address. It only becomes a revert because of the recover() at the top of Execute, and that path returns remainingGas = 0, so the caller is charged all supplied gas for a plain input error. Calling m.ValidateBasic() on each parsed msg in the loop above (alongside the nested-EVM scan) would turn these into clean, cheap errors.
| return nil, 0, fmt.Errorf("failed to parse authorization JSON: %w", err) | ||
| } | ||
|
|
||
| expiration := time.Unix(args[2].(int64), 0).UTC() |
There was a problem hiding this comment.
[suggestion] Nothing validates that expiration is in the future, and neither MsgGrant.ValidateBasic nor Grant.ValidateBasic checks it in this SDK version (the block-time check in authz.NewGrant is commented out). So grant(grantee, auth, 0) — the natural encoding a caller would reach for to mean "no expiration" — succeeds and stores a grant dated 1970 that GetCleanAuthorization deletes on first use, i.e. a silently dead grant that still shows up in granterGrants. Consider rejecting non-future timestamps here, and documenting in Authz.sol that there is no "never expires" value.
| event DelegationRewardsWithdrawn(address indexed delegator, string validator, uint256 amount); | ||
| event MultipleDelegationRewardsWithdrawn(address indexed delegator, string[] validators, uint256[] amounts); | ||
| event ValidatorCommissionWithdrawn(string indexed validator, uint256 amount); | ||
| event CommunityPoolFunded(address indexed depositor, uint256 amount); |
There was a problem hiding this comment.
[nit] Worth documenting that amount is in usei, not wei: the caller supplies msg.value in wei, but the event carries coin.Amount (usei) from HandlePaymentUsei. Consumers indexing this event will otherwise reasonably assume it mirrors msg.value.
| "github.com/sei-protocol/sei-chain/app" | ||
| "github.com/sei-protocol/sei-chain/precompiles/slashing" | ||
| "github.com/sei-protocol/sei-chain/sei-cosmos/crypto/keys/ed25519" | ||
| crptotypes "github.com/sei-protocol/sei-chain/sei-cosmos/crypto/types" |
There was a problem hiding this comment.
[nit] Typo in the import alias: crptotypes → cryptotypes (used again on the setupValidator signature below).
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
No bugs found by the automated review, but this is a large, app-hash-breaking PR that adds new fund-moving and permission-granting entry points (authz grant/exec/revoke, bank multiSend, distribution fundCommunityPool, feegrant grant/revokeAllowance, slashing unjail) across five precompiles, so it's worth a human look before merge.
What was reviewed: authz exec's nested-EVM-message rejection and depth-5 cap were checked line-by-line against app/antedecorators.AuthzNestedMessageDecorator and match its recursion and cap exactly. multiSend's input/output construction (single Input summing to the total, per-output zero-amount no-ops) was checked for balance-conservation and mirrors send's existing pointer-only caller restriction. fundCommunityPool's payment handling (HandlePaymentUsei refund-then-debit) was compared against the existing gov deposit payable flow and follows the same established pattern. Staticcall/delegatecall guards are present on all new authz/feegrant/slashing methods; bank's new multiSend only guards staticcall, consistent with (not weaker than) the existing send method it mirrors.
Extended reasoning...
This PR adds five new state-mutating precompile methods (authz grant/exec/revoke, bank multiSend, distribution fundCommunityPool, feegrant grantAllowance/revokeAllowance, slashing unjail) that let EVM contracts move funds, grant permissions, and execute messages on behalf of other accounts. It carries the app-hash-breaking label and touches money-movement and authorization logic in five modules simultaneously (authz, bank, distribution, feegrant, slashing), plus the shared keeper wiring in app/precompiles.go and precompiles/utils/expected_keepers.go.
The main security-relevant surface is authz's exec, which can spend a granter's funds on behalf of a grantee once a grant exists (this is authz's intended behavior, not a bug) and which re-implements the ante handler's nested-EVM-message rejection since precompile calls bypass ante decorators; I compared that reimplementation against app/antedecorators/authz_nested_message.go and found it to be a faithful port (same recursion structure, same depth-5 cap). All new caller-identity methods that act 'on behalf of' the caller correctly reject delegatecall and staticcall except bank's multiSend, which only rejects staticcall — but this matches the existing send method's guard level, not a regression.
Given the combination of the app-hash-breaking label, the number of modules touched in one PR, and the security-sensitive nature of authorization/fund-movement precompiles, I believe this exceeds the bar for auto-approval even with no automated findings; a human with authz/bank/distribution domain context should confirm the money-movement and permission semantics are exactly as intended before this rides the next upgrade.
Test coverage is solid (real-keeper round-trip tests for grant/exec/revoke, community-pool balance+event assertions via signed EVM tx, jailed-validator unjail, and negative tests for staticcall/delegatecall/unassociated-caller/malformed-JSON/nested-EVM-message), which lowers but doesn't eliminate the need for a human pass given the blast radius of a mistake in this code.
Drop the ERC20-pointer gating: multiSend no longer requires the denom to have a native pointer nor the caller to be it. The caller can only spend its own funds (association required), so delegatecall is rejected. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
06c9bc2 to
a849a0a
Compare
There was a problem hiding this comment.
No bugs found in this run. This PR adds four new consensus-visible, fund/state-moving precompile methods (bank multiSend, distribution fundCommunityPool, feegrant grantAllowance/revokeAllowance, slashing unjail) touching real money movement and validator state, so I think it's worth a human look before merging even without findings.
What was reviewed: the multiSend caller-authentication path (confirmed it now derives from the caller's associated Sei address rather than the pointer-gate that an earlier review round flagged as unreachable), the fundCommunityPool payable/usei-only flow and its precompile-wide delegatecall guard, the feegrant grant/revoke protobuf-JSON allowance parsing and msg-server wiring, and the slashing unjail caller-to-validator-operator derivation.
Extended reasoning...
This PR (app-hash-breaking) adds four new state-changing precompile methods across bank, distribution, feegrant, and slashing that mirror existing Cosmos SDK Msg handlers. Security-sensitive surface includes: multiSend batch-spending the caller's own funds (guarded by associated-Sei-address check + delegatecall/staticcall rejection, not pointer-gated), fundCommunityPool moving usei via the shared HandlePaymentUsei payable pattern, feegrant grant/revoke allowances via protobuf-JSON-encoded Any parsing, and slashing unjail deriving the validator operator from the caller's associated address.
An earlier review round on this PR flagged multiSend as unreachable on-chain because it was gated on the ERC20 native pointer address (which has no batch-send caller). The current diff no longer has that gate — it authenticates via the caller's associated Sei address instead, matching the PR description's stated intent ("not gated on pointer contracts"), so that specific concern appears resolved in this revision. Other prior review comments on this thread (authz exec/grant validation, expiration checks) reference precompiles/authz/authz.go, which is not part of this PR's diff (18 changed files, no authz.go) — those appear to be stale from a different revision/PR and don't apply to what's currently proposed here.
Level of scrutiny: this warrants careful human review despite the bug hunter finding nothing, because it's labeled app-hash-breaking, moves real funds (multiSend, community pool funding), and changes validator jail state — mistakes here have direct financial/consensus impact for a live chain, which is a different risk profile than incorrect view/query methods.
There was a problem hiding this comment.
Well-structured extension of four precompiles with sound caller-identity guards (delegatecall/staticcall rejection, association requirements) and good happy-path + rejection test coverage. One blocker: fundCommunityPool makes the distribution precompile payable but distribution.DistrAddress was never added to the payablePrecompiles allowlist, so every call will emit a spurious, unbalanced cosmos transfer event to 0x…1007.
Findings: 2 blocking | 8 non-blocking | 4 posted inline
Blockers
fundCommunityPoolmakes the distribution precompile payable, butdistribution.DistrAddress(0x0000000000000000000000000000000000001007) is not added to thepayablePrecompilesmap inx/evm/keeper/precompile.goor its giga mirrorgiga/deps/xevm/keeper/precompile.go. Both files carry the comment "add any payable precompiles here / these will suppress transfer events to/from the precompile address", and every other payable precompile (bank, staking, gov, wasmd) is registered. Without the entry,k.GetVMBlockContext'stxferhook takes thecore.Transferbranch instead ofstate.TransferWithoutEvents, soDBImpl.SubBalance/AddBalancerun witheventsSuppressed == falseand emitcoin_spent/coin_received/transfercosmos events for the caller→precompile leg.pcommon.HandlePaymentUseithen refunds the depositor on a fresh, discarded event manager (ctx.WithEventManager(sdk.NewEventManager())), so the return leg emits nothing. Net result: everyfundCommunityPooltx leaves a spurious, unbalanced transfer event showing usei moving into the distribution precompile address and never coming back — event-derived balance tracking in indexers/explorers will drift. Fix by addingdistribution.DistrAddress: {}to both maps.- 1 blocking issue(s) flagged inline on specific lines.
Non-blocking
- The Cursor second-opinion pass produced no output —
cursor-review.mdis empty, so that pass contributed nothing to this synthesis. - Both Codex findings are stale and should be disregarded. Codex P1 ("missing authz implementation") and P2 ("
multiSendomits the documented pointer restriction") both describe the first commit's message rather than the final diff. The net PR diff touches only bank, distribution, feegrant, slashing,precompiles/utils/expected_keepers.goandapp/precompiles.go— no authz/addr changes remain — and the PR description explicitly descopes authz and states thatmultiSendis intentionally not pointer-gated. The design rationale holds on review: unlikesend,multiSendderives the sender from the caller's own association rather than an arbitraryfromAddressargument, and it rejects delegatecall, so the pointer gate is not load-bearing here. - Test coverage gaps.
TestFundCommunityPooldoes not exercise the delegatecall rejection that the PR description claims for all new caller-identity methods (the guard is inherited from distribution'sExecute, so it works, but it is untested for this method).TestMultiSendcovers only a tokenfactory-style denom (ufoo) — no case for the base denom (usei) or for an output addressed to a blocked module account (BlockedAddrinmsgServer.MultiSend). TestFundCommunityPool's ordering dependency is a latent maintenance hazard. As the PR notes, the test must sit last indistribution_test.gobecause funding the pool grows the FeePool and shifts later tests' exact gas assertions. A future test appended after it will silently break unrelated assertions with a confusing failure. Consider at.Cleanupthat restores the pool, using an isolated app instance for this test, or at minimum a comment at the top of the file warning against appending after it.- Nit: the new
crptotypesimport alias inprecompiles/slashing/slashing_test.gois a typo forcryptotypes(copied from the pre-existing one indistribution_test.go). - 3 suggestion(s)/nit(s) flagged inline on specific lines.
| return | ||
| } | ||
|
|
||
| coin, err := pcommon.HandlePaymentUsei(ctx, p.evmKeeper.GetSeiAddressOrDefault(ctx, p.address), depositor, value, p.bankKeeper, p.evmKeeper, hooks, evm.GetDepth()) |
There was a problem hiding this comment.
[blocker] This makes the distribution precompile payable for the first time, but distribution.DistrAddress was not added to the payablePrecompiles allowlist in x/evm/keeper/precompile.go (or the giga mirror giga/deps/xevm/keeper/precompile.go).
That map drives txfer in Keeper.GetVMBlockContext: registered addresses get state.TransferWithoutEvents (which sets eventsSuppressed, so DBImpl.SubBalance/AddBalance swap in a throwaway event manager), unregistered ones get plain core.Transfer. Because 0x…1007 is unregistered, the EVM's caller→precompile value transfer will emit real coin_spent/coin_received/transfer cosmos events, while the HandlePaymentUsei refund on this line runs on ctx.WithEventManager(sdk.NewEventManager()) and emits nothing.
So each fundCommunityPool call leaves a one-sided transfer event moving usei into the distribution precompile address with no offsetting event — the exact inconsistency the allowlist exists to prevent (bank, staking, gov and wasmd are all listed for their own payable methods).
Please add distribution.DistrAddress: {} to payablePrecompiles in both files.
| return nil, 0, err | ||
| } | ||
| denom := args[1].(string) | ||
| if denom == "" { |
There was a problem hiding this comment.
[suggestion] multiSend accepts any denom, including the base denom (usei). Because it hands off straight to bankMsgServer.MultiSend, a usei transfer here bypasses DBImpl entirely: no OnBalanceChange, and none of the hooks.OnEnter/OnExit legs that sendNative emits (lines 372–384) to make native value movement visible to tracers. debug_traceTransaction will therefore show EVM-visible balances changing with nothing in the trace to account for it.
send avoids this in practice only incidentally — its pointer gate means a denom needs a registered ERC20 native pointer to be movable. multiSend drops that gate, so this becomes reachable for the base denom.
Suggest either rejecting denom == p.evmKeeper.GetBaseDenom(ctx) here (pointing callers at sendNative), or emitting the same tracing hooks sendNative does.
| } | ||
| toAddresses := args[0].([]common.Address) | ||
| amounts := args[2].([]*big.Int) | ||
| if len(toAddresses) == 0 || len(toAddresses) != len(amounts) { |
There was a problem hiding this comment.
[nit] Slightly inconsistent handling of no-op calls: multiSend([], denom, []) reverts with "must be non-empty", but multiSend([a], denom, [0]) falls through the zero-amount continue to the len(outputs) == 0 short circuit at line 306 and returns true having done nothing. Worth either dropping the len(toAddresses) == 0 check (letting both hit the short circuit) or documenting the asymmetry in Bank.sol.
| event DelegationRewardsWithdrawn(address indexed delegator, string validator, uint256 amount); | ||
| event MultipleDelegationRewardsWithdrawn(address indexed delegator, string[] validators, uint256[] amounts); | ||
| event ValidatorCommissionWithdrawn(string indexed validator, uint256 amount); | ||
| event CommunityPoolFunded(address indexed depositor, uint256 amount); |
There was a problem hiding this comment.
[nit] amount here is in usei (coin.Amount), while the caller supplies the deposit as msg.value in wei — a 10^12 discrepancy an integrator comparing the two would hit. This matches the existing ValidatorCommissionWithdrawn convention, so no need to change the unit, but the natspec on fundCommunityPool documents the wei-remainder rule and not the event's unit. Worth an explicit /// @dev amount is denominated in usei on the event.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
fundCommunityPool is the first payable method on the distribution precompile, so distribution.DistrAddress must be in payablePrecompiles (both the x/evm and giga copies): otherwise the EVM value transfer into the precompile emits a coin_received event for the precompile address with no offsetting event, since the refund inside HandlePaymentUsei runs on a discarded event manager. Also assert the absence of that event in TestFundCommunityPool; the assertion fails without the allowlist entry. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
No new bugs found this run, but this PR remains worth a human look given its history and scope: it adds real state-moving precompile methods (community-pool funding, fee-grant allowances, validator unjail) at the current head ABI version and is marked app-hash-breaking.
What was reviewed:
- Confirmed the payable-allowlist gap from earlier review rounds is now fixed:
distribution.DistrAddressis present inpayablePrecompilesin bothx/evm/keeper/precompile.goand the giga mirror. - Confirmed
multiSend(bank) andexec/grant(authz), both flagged as broken/unreachable in earlier rounds, were removed entirely rather than patched — verified neither exists in the currentprecompiles/bank/bank.goorprecompiles/authz/authz.go. - Checked the feegrant
grantAllowancezero-expiration candidate this run's bug hunt raised — ruled out (matches upstreamauthz/feegrantvalidation, which doesn't check expiration either; not a regression introduced here).
Extended reasoning...
This run found no new bugs. The three concrete issues raised by reviewers across this PR'''s earlier revisions have all been resolved as of the current head (commit 350b1da): the missing payablePrecompiles entry for distribution was added, and the unreachable/overbroad multiSend/authz exec methods were dropped from scope entirely rather than patched (per the PR description'''s explicit descoping and the Drop bank multiSend/Drop authz msg methods commits).
Despite the clean state, I'''m deferring rather than approving because the remaining surface is still consensus-critical: fundCommunityPool moves real value through a newly-payable precompile, grantAllowance/revokeAllowance create/destroy fee-spending authority for other addresses, and unjail lets a caller submit MsgUnjail on a validator'''s behalf. The PR is tagged app-hash-breaking, meaning these additions change consensus-visible behavior at the current ABI version. Given the history of real, non-trivial bugs found and fixed across this PR'''s life (the payable-allowlist gap, the unreachable/overbroad methods), I think a human should do a final pass before merge even though the current diff itself looks correct.
There was a problem hiding this comment.
Adds fundCommunityPool (payable), feegrant grantAllowance/revokeAllowance, and slashing unjail to the head precompile versions; the payment flow, delegatecall/staticcall/association guards, ABI↔Solidity consistency, and the mirrored payablePrecompiles change all check out. No blockers — only documentation, test-coverage, and test-ordering-fragility notes.
Findings: 0 blocking | 9 non-blocking | 5 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Second-opinion passes:
cursor-review.mdis empty (that pass produced no output);codex-review.mdreported no material issues. My independent pass also found no blockers. - Missing negative-path tests for
fundCommunityPool: (a) a value with a non-zero wei remainder (not a multiple of 10^12) should be rejected byHandlePaymentUsei, and (b) delegatecall rejection. The delegatecall guard for distribution lives in the sharedExecuteprologue rather than in the method, so a future refactor that moves it would not be caught by any distribution test. TestUnjail/setupValidatormutate the sharedtestkeeper.EVMTestApp(create + jail a validator, and forceUpdateStatus(Unbonded)viaSetValidatorwithout refreshing the power index). Harmless today since it is the last test in the only file in that package, but it carries the same ordering fragility asTestFundCommunityPool.- Consider also asserting the new methods in a shared "all tx methods reject delegatecall/staticcall" table test rather than per-method ad-hoc calls, so future precompile methods can't silently skip the guards.
- 5 suggestion(s)/nit(s) flagged inline on specific lines.
| staking.StakingAddress: {}, | ||
| gov.GovAddress: {}, | ||
| wasmd.WasmdAddress: {}, | ||
| distribution.DistrAddress: {}, |
There was a problem hiding this comment.
[suggestion] Making the distr precompile payable is app-hash-affecting, but TestIsPayablePrecompile (in x/evm/keeper/precompile_test.go and the giga mirror giga/deps/xevm/keeper/precompile_test.go) still only asserts bank/staking/gov. Please add require.True(t, evmkeeper.IsPayablePrecompile(toAddr(distribution.DistrAddress))) to both so an accidental removal of this entry is caught by unit tests rather than only by TestFundCommunityPool's event assertion.
| event DelegationRewardsWithdrawn(address indexed delegator, string validator, uint256 amount); | ||
| event MultipleDelegationRewardsWithdrawn(address indexed delegator, string[] validators, uint256[] amounts); | ||
| event ValidatorCommissionWithdrawn(string indexed validator, uint256 amount); | ||
| event CommunityPoolFunded(address indexed depositor, uint256 amount); |
There was a problem hiding this comment.
[nit] amount here is the usei amount (the coin.Amount packed at distribution.go:862), not the wei msg.value the caller sent — i.e. sending 100_000_000_000_000 wei emits amount = 100. That matches the other events on this interface, but since this is the first payable method it's easy for an EVM consumer to misread. Worth a natspec line on the event (or on fundCommunityPool) stating the event amount is denominated in usei.
| // allowance is protobuf-JSON encoded (the same encoding the queries | ||
| // return), e.g. | ||
| // {"@type":"/cosmos.feegrant.v1beta1.BasicAllowance","spend_limit":[{"denom":"usei","amount":"1000"}],"expiration":null} | ||
| function grantAllowance(address grantee, bytes memory allowance) external returns (bool success); |
There was a problem hiding this comment.
[suggestion] grantAllowance resolves the grantee via pcommon.GetSeiAddressFromArg, which errors with AssociationMissing for an EVM address that has no Sei association (and for the zero address). Since a fee grant is most useful for a new account that hasn't transacted yet, this precondition is likely to surprise callers — worth documenting here (and on revokeAllowance) that the grantee must already have an associated Sei address.
| return | ||
| } | ||
|
|
||
| if err := p.distrKeeper.FundCommunityPool(ctx, sdk.NewCoins(coin), depositor); err != nil { |
There was a problem hiding this comment.
[nit] Unlike the feegrant/slashing methods in this PR (which route through the module msg servers), this calls the keeper directly, so no MsgFundCommunityPool typed/message cosmos event is emitted — only the underlying bank transfer events. That's consistent with the rest of this precompile, so not a blocker, but given the PR's framing as "expose module Msg rpcs", routing through distrtypes msg server would keep indexer output uniform with the other new methods.
| } | ||
| } | ||
|
|
||
| func TestFundCommunityPool(t *testing.T) { |
There was a problem hiding this comment.
[suggestion] The PR description notes this test must stay at the end of the file because it grows the shared EVMTestApp FeePool and earlier tests assert exact gas values. That constraint isn't recorded anywhere in the code — please add a short comment here so a future reader doesn't move the function or insert a new test after it and get a confusing gas-assertion failure elsewhere in the file.
Superseded: latest AI review found no blocking issues.
Also reverts the distr entry in payablePrecompiles, which existed only for fundCommunityPool. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
LGTM — this PR has been narrowed down to just the slashing unjail() precompile method; the earlier bank/authz/distribution/feegrant additions that drew the blocker/suggestion comments above were all dropped in later commits (a849a0a, 8a81e7c, 68d3043, da9e73c), so those findings no longer apply to the current diff.
What was reviewed: the unjail flow (caller → Sei address → sdk.ValAddress → MsgUnjail → slashing msg server), the delegatecall/staticcall/non-payable/unassociated-caller/non-operator rejection paths, and the new SlashingMsgServer plumbing in app/precompiles.go and precompiles/utils/expected_keepers.go. This follows the same caller-derived-operator pattern as distribution's existing withdrawValidatorCommission, and test coverage exercises both the happy path and each rejection case.
Extended reasoning...
Overview
This PR now only adds a single new precompile transaction method: slashing unjail(), which lets a validator operator unjail themselves via the EVM by deriving their validator address from their associated Sei address and calling MsgUnjail through a new narrow SlashingMsgServer interface. Wiring is added to app/precompiles.go and precompiles/utils/expected_keepers.go following the existing keeper-interface pattern used for other msg-server-backed precompile methods. The PR previously included several additional precompile methods (bank multiSend, authz grant/exec/revoke, distribution fundCommunityPool, feegrant grantAllowance/revokeAllowance) which drew multiple blocker/suggestion comments from Cursor bugbot, seidroid bugbot, and my own prior review — all of that code has since been removed in commits a849a0a, 8a81e7c, 68d3043, and da9e73c, and the current diff (confirmed via the PR API) touches only the 6 slashing/precompile-plumbing files reviewed here.
Security risks
unjail acts on behalf of the caller only (derived from their own EVM→Sei association, never a passed-in argument), and rejects both delegatecall (which would otherwise let a contract act on behalf of its own caller) and staticcall. The underlying MsgUnjail handler enforces that the caller is actually the validator's operator and applies the module's own jailing/self-delegation preconditions, so there's no new privilege beyond what the existing Cosmos msg already allows. No funds movement, no new precompile address, no changes to payability.
Level of scrutiny
This is a narrow, additive change following a well-established pattern in this codebase (same shape as distribution's withdrawValidatorCommission), touching non-critical-path code (an opt-in precompile method), with unit tests covering the happy path and every rejection branch (staticcall, delegatecall, non-payable, unassociated caller, non-operator caller). Light-to-moderate scrutiny is appropriate, and I found nothing beyond what's already covered by tests.
Other factors
The PR is flagged app-hash-breaking since it's a consensus-visible ABI addition, but the author's description notes this is expected to ride the next upgrade, consistent with how #3767 (the referenced prior PR) was handled. No CODEOWNERS are currently enforced on the touched paths. The prior review threads (mine and the two bots') were entirely about code that is no longer part of this PR, so none of those concerns carry over to the current state.
There was a problem hiding this comment.
Adds a well-scoped unjail() tx method to the slashing precompile; authorization (operator derived from the caller's associated Sei address), delegatecall/staticcall/non-payable guards, IsTransaction classification, keeper plumbing and versioning all follow the established distribution/gov patterns and look correct. Only nits and consistency notes — no blockers.
Findings: 0 blocking | 8 non-blocking | 4 posted inline
Blockers
- None at the file/PR level.
Non-blocking
cursor-review.mdis empty — the Cursor pass produced no output, so this review is Claude + Codex only.- Codex's only finding (P2: "PR description is stale, claims distribution and feegrant methods/tests are included") does not hold against the current description, which lists only slashing
unjail()and explicitly descopes distributionFundCommunityPooland feegrantGrant/RevokeAllowance. Likely written against an earlier revision; no action needed. unjailemits no EVM log, so contracts/indexers can't observe it from receipts (distribution emitsValidatorCommissionWithdrawnetc. for its tx methods, though gov'svotealso emits nothing). Optional, and adding one later is itself a consensus-visible change, so if it's wanted it's cheaper to add now in the same ABI version.- There is no e2e coverage for the slashing precompile in
contracts/test/— consistent with the current state (slashing has none at all), just notingunjail()is unit-test-only. - 4 suggestion(s)/nit(s) flagged inline on specific lines.
| // Transaction methods act on behalf of the caller, so they must not be | ||
| // reachable through delegatecall (which would let a contract act on | ||
| // behalf of its own caller) or staticcall. | ||
| if ctx.EVMPrecompileCalledFromDelegateCall() { |
There was a problem hiding this comment.
[nit] Worth an explicit note that placing the delegatecall guard after the query dispatch is deliberate. gov, distribution, and staking all reject delegatecall at the top of Execute for every method including views; here the existing params/signingInfo/signingInfos views stay delegatecall-reachable. That's the right call (tightening them would be a consensus-visible behavior change for existing contracts), but the current comment only explains why tx methods are guarded, not why the guard sits below the view switch — a future reader could easily "fix" the ordering to match gov and silently break callers.
| case UnjailMethod: | ||
| return p.unjail(ctx, method, caller, args, value) | ||
| } | ||
| return |
There was a problem hiding this comment.
[nit] This naked return yields (nil, 0, nil) — reported success, empty output, and remainingGas == 0 so the caller's entire gas is consumed. Pre-existing shape (gov/distribution do the same), but now that Execute has two dispatch switches it's easier for a future method added to abi.json to land here unhandled. Returning an explicit fmt.Errorf("unknown method %s", method.Name) would fail loudly instead.
| "github.com/sei-protocol/sei-chain/app" | ||
| "github.com/sei-protocol/sei-chain/precompiles/slashing" | ||
| "github.com/sei-protocol/sei-chain/sei-cosmos/crypto/keys/ed25519" | ||
| crptotypes "github.com/sei-protocol/sei-chain/sei-cosmos/crypto/types" |
There was a problem hiding this comment.
[nit] Typo in the import alias: crptotypes → cryptotypes (the alias used elsewhere in the repo).
| require.Equal(t, vm.ErrExecutionReverted, err) | ||
| } | ||
|
|
||
| func setupValidator(t *testing.T, ctx sdk.Context, a *app.App, bondStatus stakingtypes.BondStatus, valPub crptotypes.PubKey) sdk.ValAddress { |
There was a problem hiding this comment.
[nit] bondStatus is only ever passed stakingtypes.Unbonded, and since CreateValidatorMsg/Handle already leaves the validator Unbonded until the staking EndBlocker runs, val.UpdateStatus(bondStatus) + SetValidator are effectively no-ops. Consider dropping the parameter (and the redundant status write) until a second caller actually needs it.
2 participants
Describe your changes and provide context
Follow-up to #3767 (which exposed the missing Query rpcs): the same audit of
service Msgrpc defs under/protoand/sei-cosmos/protofound several module Msgs with no EVM precompile counterpart. After review, this PR closes the one remaining applicable gap:unjail()Unjailunjailderives the validator operator from the caller's associated Sei address (like the distribution precompile'swithdrawValidatorCommission) and calls the slashing msg server.SlashingMsgServerinterface inprecompiles/utils/expected_keepers.go, wired toslashingkeeper.NewMsgServerImplinapp/precompiles.go.precompiles/setup.gochanges. Like feat(precompiles): expose module Query rpcs as precompile methods #3767, this is a consensus-visible addition at the current head ABI version and should ride the next upgrade.Reviewed and intentionally not implemented
MultiSend, authzGrant/Exec/Revoke, distributionFundCommunityPool, feegrantGrantAllowance/RevokeAllowance— descoped in review.AssociateContractAddress— deprecated.SubmitEvidence— app.go never sets an evidence router, so the msg can never succeed on sei.CreateVestingAccount— the vesting module is deprecated (ErrVestingDeprecated).Testing performed to validate your change
go build ./...,go vet,gofmt -s/goimportsclean;go test -race ./precompiles/... ./apppasses locally.🤖 Generated with Claude Code