[codex] add evm-only staking precompile - #3616
Conversation
|
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 @@
## codex/sei-v3-evm-only-scaffold #3616 +/- ##
==================================================================
- Coverage 58.29% 58.17% -0.12%
==================================================================
Files 2179 2190 +11
Lines 177143 178823 +1680
==================================================================
+ Hits 103258 104028 +770
- Misses 64811 65471 +660
- Partials 9074 9324 +250
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
PR SummaryHigh Risk Overview Custom precompiles are wired through an SDK-free The staking precompile at Reviewed by Cursor Bugbot for commit b2232ec. Bugbot is set up for automated code reviews on this repo. Configure here. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
966d056 to
ab82ec3
Compare
ab82ec3 to
23fc6d3
Compare
| record, ok, err := getUnbondingDelegation(ctx.Store, pair.DelegatorAddress, pair.ValidatorAddress) | ||
| if err != nil || !ok { | ||
| return err | ||
| } |
There was a problem hiding this comment.
Missing unbonding record skips payout
Medium Severity
In completeUnbonding and completeRedelegation, when the store lookup returns ok == false with a nil error, the functions return success instead of failing. The mature-queue loop still deletes queue entries, so a queued unbonding pair without a matching record can be dropped without releasing escrowed stake or cleaning redelegation indexes.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit b90eb37. Configure here.
There was a problem hiding this comment.
this is consistent with cosmos behavior
b90eb37 to
27e3acc
Compare
27e3acc to
550f6fa
Compare
![seidroid[bot]](https://avatars.githubusercontent.com/in/2745135?s=60&v=4){kind=small}
There was a problem hiding this comment.
Adds the first SDK-free staking custom precompile for the evm-only executor, with byte-keyed storage-backed state, an end-block hook, and a thorough test suite including a full delegate→redelegate→undelegate lifecycle. The implementation closely mirrors Cosmos staking semantics and looks correct; the main open concern is flat gas accounting for state-growing operations. No blocking correctness bugs found.
Findings: 0 blocking | 9 non-blocking | 4 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Gas accounting (Codex finding #1): custom precompile execution charges only the flat RequiredGas (writeGas=20000 + 16/byte). Staking writes JSON-encoded indexes (validators index, per-validator/per-delegator delegation lists, queues) that are re-read, sorted, and re-serialized into chunked storage slots on every mutation — O(n) SSTOREs charged as a flat fee. Block gas therefore does not bound precompile state growth or runtime, allowing cheap unbounded index growth. This mirrors existing Sei precompile flat-gas behavior and the evm-only path is documented as early-integration, so non-blocking — but should be revisited before production (per-operation/per-byte-of-stored-state metering).
- Disagree with Codex finding #2 (editValidator minSelfDelegation vs validator.Tokens): Cosmos x/staking msg_server.EditValidator also checks
MinSelfDelegation.GT(validator.Tokens), not self-delegation. The implementation here is faithful Cosmos parity, not a new invariant break. - Disagree with Codex finding #3 (commission max-change uses signed delta, not abs): Cosmos
Commission.ValidateNewRateuses signednewRate.Sub(c.Rate).GT(MaxChangeRate)and also permits unbounded decreases. The code (and its comment) intentionally matches upstream, so this is parity rather than a bug. - Cursor second-opinion review file (cursor-review.md) is empty — that pass produced no output. REVIEW_GUIDELINES.md is also empty/absent, so no repo-specific standards were applied.
- Documented limitations (no rewards/slashing/jailing, shares track tokens 1:1, historical info recorded in end-block rather than begin-block) are clearly noted in the README; just confirm downstream consumers/indexers tolerate the zero-amount DelegationRewardsWithdrawn events and the deferred historical-info availability.
- 4 suggestion(s)/nit(s) flagged inline on specific lines.
| if isTransaction(method.Name) { | ||
| gas = writeGas | ||
| } | ||
| return gas + inputByteGas*uint64(len(input)) //nolint:gosec // input length is bounded by memory. |
There was a problem hiding this comment.
[suggestion] Gas is flat per call (writeGas + 16/byte of input) regardless of how much module-like state the operation rewrites. delegate/redelegate/undelegate rewrite O(n) JSON index lists (validators index, per-validator/per-delegator delegation lists, time queues) into chunked storage slots, so the real SSTORE/CPU cost can far exceed the charged gas, and block gas no longer bounds state growth. This matches existing Sei precompile flat-gas semantics and the path is experimental, so not blocking — but worth per-operation metering before this backs production state. (Codex finding #1.)
| if err != nil { | ||
| return nil, err | ||
| } | ||
| if minSelfDelegation.Cmp(tokens) > 0 { |
There was a problem hiding this comment.
[nit] Codex flags this as comparing minSelfDelegation against validator.Tokens (which includes third-party delegations). Note this matches Cosmos x/staking msg_server.EditValidator, which also checks msg.MinSelfDelegation.GT(validator.Tokens). So this is faithful parity, not a newly introduced invariant break — no change needed unless intentional divergence from Cosmos is desired here.
| return errCommissionNegative | ||
| case newRate.Cmp(maxRate) > 0: | ||
| return errCommissionGTMaxRate | ||
| case new(big.Rat).Sub(newRate, oldRate).Cmp(maxChange) > 0: |
There was a problem hiding this comment.
[nit] Codex flags that large commission decreases bypass the max-change check because this uses signed newRate - oldRate rather than the absolute delta. This actually matches Cosmos Commission.ValidateNewRate, which likewise uses the signed delta and allows unbounded decreases. Given the stated goal of Cosmos parity (and the function doc), this is correct as written — not a bug.
| } else if err := addPoolNotBonded(ctx.Store, useiAmount); err != nil { | ||
| return nil, err | ||
| } | ||
| p.emit(ctx, "Delegate", ctx.Caller, validatorAddress, util.CloneBig(ctx.ApparentValue)) |
There was a problem hiding this comment.
[nit] Minor unit inconsistency: the Delegate event emits the raw wei ApparentValue (18 decimals), while Undelegate (line 392) and Redelegate (line 334) emit the usei amount arg (6 decimals). Worth confirming this matches the canonical Sei staking precompile event semantics so indexers decode all three consistently.
550f6fa to
accacbf
Compare
There was a problem hiding this comment.
Adds the first SDK-free staking custom precompile for the evm-only executor, with a byte-keyed storage-backed store, end-block validator-set/unbonding/redelegation processing, and a good lifecycle e2e test. No hard blockers, but there are a couple of behavioral divergences from Cosmos staking (redelegation-from-unbonded, self-delegation below MinSelfDelegation) worth noting, most of which are already flagged as unmodeled in the README.
Findings: 0 blocking | 6 non-blocking | 2 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Second-opinion coverage was partial:
cursor-review.mdis empty (Cursor produced no output) andREVIEW_GUIDELINES.mdis empty, so no repo-specific guideline overrides were applied. Codex's pass produced two findings, both incorporated below. - Broad SDK-parity caveat: the precompile does not model slashing, jailing, or rewards, and shares track tokens 1:1. This is documented in the README's limitations, but reviewers/integrators should treat the staking precompile as not yet safe for production validator economics — several security-relevant invariants (min-self-delegation enforcement via jailing, slash-driven share/token divergence) are intentionally absent.
- Tests cover the happy-path lifecycle and guardrails well, but there is no coverage for the two divergences noted inline (redelegation away from an already-unbonded source validator, and self-undelegation below MinSelfDelegation). Consider adding cases once the intended behavior is decided.
DelegationRewardsWithdrawnevents are emitted with a hard-coded zero amount on delegate/redelegate/undelegate; fine as a placeholder but callers relying on this event will always see zero, which could be misleading — worth documenting on the ABI/event side too (already partially noted in the README).- 2 suggestion(s)/nit(s) flagged inline on specific lines.
| if err := movePoolsForRedelegation(ctx.Store, src.Status, dst.Status, amount); err != nil { | ||
| return nil, err | ||
| } | ||
| if err := addRedelegation(ctx.Store, delegator, srcValidator, dstValidator, amount, util.SaturatingCompletionTime(ctx.Block.Time, params.UnbondingTime)); err != nil { |
There was a problem hiding this comment.
[suggestion] Divergence from Cosmos staking: this always schedules the redelegation entry with block.Time + UnbondingTime, regardless of the source validator's bond status. In the SDK, getBeginInfo returns completeNow = true when the source validator is already Unbonded, so the redelegation completes immediately and no in-progress redelegation record is created. Here, redelegating away from an unbonded source leaves a full-unbonding-period redelegation record, which then blocks further redelegation of the destination tokens via the transitive-redelegation check (hasReceivingRedelegation) for the entire period. Consider branching on src.Status == bondStatusUnbonded to complete immediately (matching the SDK) rather than always using SaturatingCompletionTime(...).
| if err := validateDelegationAmount(ctx.Store, delegator, validatorAddress, amount); err != nil { | ||
| return nil, err | ||
| } | ||
| if err := addDelegation(ctx.Store, delegator, validatorAddress, new(big.Int).Neg(amount)); err != nil { |
There was a problem hiding this comment.
[suggestion] Self-undelegation is allowed to drop a validator's self-delegation below its MinSelfDelegation with no consequence. In the SDK, when self-delegation falls below the minimum the validator is jailed (and removed from the active set); here the validator can keep validating with only external stake and zero self-bond. This follows from the README's documented decision not to model jailing, but it removes a real skin-in-the-game invariant. At minimum consider documenting this on the undelegate path, or adding an explicit guard/rejection until jailing is implemented.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| maxValidators = 0 | ||
| } | ||
| if maxValidators > len(candidates) { | ||
| maxValidators = len(candidates) | ||
| } | ||
|
|
||
| updates := make([]precompiles.ValidatorUpdate, 0) | ||
| totalPower := int64(0) | ||
| amtFromBondedToNotBonded := new(big.Int) | ||
| amtFromNotBondedToBonded := new(big.Int) | ||
|
|
||
| for i := 0; i < maxValidators; i++ { | ||
| validator := candidates[i] | ||
| newPower, err := validatorPower(validator) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| if newPower == 0 { | ||
| break |
There was a problem hiding this comment.
🟡 The staking precompile exposes MaxVotingPowerRatio and MaxVotingPowerEnforcementThreshold in Params (types.go:109-110) and returns them from the ABI params() method, but applyAndReturnValidatorSetUpdates (endblock.go:37-169) never consults them — validator selection is pure top-MaxValidators by power with no fraction cap, unlike Cosmos's Sei fork which does enforce MaxVotingPowerRatio. Currently benign because the defaults are 0.000000000000000000 (the disabled sentinel) and no writer exists, but the ABI is misleading — either implement the cap or add a note to README.md alongside the existing 'no slashing / no jailing / no rewards' limitations.
Extended reasoning...
The gap
Params declares two fields that shape validator-set updates in Cosmos's Sei fork:
MaxVotingPowerRatio— cap on any single validator's fraction of total voting powerMaxVotingPowerEnforcementThreshold— threshold below which the cap does not kick in
Both are populated with "0.000000000000000000" defaults in loadParams (state.go:241-250) and returned to EVM callers via the ABI-exposed params() method (staking.go:889, abi.json). But grepping the giga/evmonly tree shows they appear only in those three sites — types.go, state.go, and abi.json. No code reads them.
Specifically, applyAndReturnValidatorSetUpdates (endblock.go:37-169) walks validatorsByPower, takes the first MaxValidators entries by (power, operator-address-asc), and emits ValidatorUpdate records with each validator's raw power. There is no post-selection pass that scales down concentrations above MaxVotingPowerRatio or that applies the enforcement threshold.
Why this is inconsistent
Every other field on Params is actually enforced by this PR:
MinCommissionRate— checked invalidateInitialCommissionandvalidateCommissionUpdate(commission.go)HistoricalEntries— used intrackHistoricalInfopruning (state.go)MaxEntries— checked inredelegate/undelegate(staking.go)MaxValidators— bounds the top-N cut inapplyAndReturnValidatorSetUpdatesUnbondingTime— threaded into completion-time math
Only MaxVotingPowerRatio and MaxVotingPowerEnforcementThreshold are dead surface. An EVM caller reading params() reasonably assumes these caps are active, but they are not.
Why it's benign today
The defaults are 0.000000000000000000, which is Cosmos Sei's 'cap disabled' sentinel. There is also no setter method or genesis import that writes these fields today — loadParams always returns the hardcoded defaults. So a caller reading params() sees a disabled cap and correctly observes no cap being enforced. The runtime behavior is currently self-consistent.
Additionally the README already lists other Cosmos-parity gaps (no slashing, no jailing, no rewards), and this PR's ValidatorUpdates have no runtime consumer yet, so nothing hard-fails.
Step-by-step proof
- Create two validators A and B via
createValidatorwith self-stake 90 SEI and 10 SEI respectively. (For the sake of illustration assume params.MaxVotingPowerRatio has been set to"0.500000000000000000"— 50% cap.) - End-block runs:
validatorsByPowerreturns[A (power 90), B (power 10)]. applyAndReturnValidatorSetUpdatesloops through the topMaxValidatorsentries and emitsValidatorUpdate{PubKey: A.PubKey, Power: 90}andValidatorUpdate{PubKey: B.PubKey, Power: 10}.- A gets 90% of consensus power — no cap applied — even though
MaxVotingPowerRatiois set to 50%. Cosmos's Sei fork would scale A down to 50% here.
Because there's no writer today, step 1's "has been set" is not currently reachable — that's why this is a nit rather than a blocker. But the moment someone adds a setter (or genesis import) for these params, callers will observe values that the endblock code silently ignores.
Fix
Either (a) implement the cap in applyAndReturnValidatorSetUpdates — after the top-N selection, scale down any validator whose power exceeds totalPower * MaxVotingPowerRatio when totalPower >= MaxVotingPowerEnforcementThreshold — or (b) drop the two fields from Params (and abi.json) — or (c) add a note to README.md alongside the existing 'does not model slashing / jailing / rewards' list explicitly stating that MaxVotingPowerRatio/MaxVotingPowerEnforcementThreshold are exposed for parity with the Cosmos ABI but not enforced in the evm-only path.
Nit severity because this is an experimental precompile whose ValidatorUpdates aren't consumed yet, the defaults render the enforcement moot today, and other Cosmos-parity gaps are already documented in the README.
| if err := addUnbondingDelegation(ctx.Store, delegator, validatorAddress, amount, ctx.Block.Number, util.SaturatingCompletionTime(ctx.Block.Time, params.UnbondingTime)); err != nil { | ||
| return nil, err | ||
| } | ||
| if validator.Status == bondStatusBonded { | ||
| if err := addPoolBonded(ctx.Store, new(big.Int).Neg(amount)); err != nil { | ||
| return nil, err | ||
| } | ||
| if err := addPoolNotBonded(ctx.Store, amount); err != nil { | ||
| return nil, err | ||
| } | ||
| } | ||
| p.emit(ctx, "Undelegate", ctx.Caller, validatorAddress, amount) | ||
| p.emit(ctx, "DelegationRewardsWithdrawn", ctx.Caller, validatorAddress, new(big.Int)) | ||
| return method.Outputs.Pack(true) | ||
| } |
There was a problem hiding this comment.
🟡 Orphan Unbonded validators with zero shares are never garbage-collected. A validator created with self-stake < 1M usei never reaches Bonded (validatorsByPower filters power==0), never enters validatorQueueIndexKey (only populated in the Bonded→Unbonding branch of applyAndReturnValidatorSetUpdates), and thus never hits removeValidator (only wired to unbondAllMatureValidators in endblock.go). After the operator undelegates their self-stake, the validator record persists forever in validatorsIndexKey and validatorConsensusPubkeyKey — permanently bricking that EVM operator address and consensus pubkey from ever creating a new validator (createValidator rejects both duplicates). Cosmos x/staking calls RemoveValidator when shares reach 0 and status is Unbonded; consider mirroring that from addValidatorTokens (state.go:130) or undelegate (staking.go:381) when shares hit 0 and Status != Bonded.
Extended reasoning...
The bug
removeValidator (state.go:118) is the only function that deletes a validator record from validatorsIndexKey and validatorConsensusPubkeyKey. It is called from exactly one place: unbondAllMatureValidators at endblock.go:257, which iterates validatorQueueIndexKey.
That queue is populated in exactly one place: insertValidatorQueue inside the noLongerBonded branch of applyAndReturnValidatorSetUpdates (endblock.go), which fires only when a Bonded validator transitions to Unbonding.
A validator that never reaches Bonded therefore never enters the queue and can never be removed — regardless of what happens to its tokens/shares.
The trigger
With powerReduction = 1_000_000, any self-stake < 1M usei yields validatorPower = 0. validatorsByPower filters those out (endblock.go, if power == 0 { continue }), so the validator never becomes a bonding candidate. minSelfDelegation on createValidator is only required to be positive (staking.go:434, ValidatePositiveAmount(minSelfDelegation, ...)), so minSelfDelegation=1 combined with value=1e12 wei (= 1 usei) passes.
The permanent lock
Once the operator calls undelegate and the entry matures, the validator sits in validatorsIndexKey with Tokens=0, Shares=0, Status=Unbonded. createValidator at staking.go:432-441 then rejects any retry:
getValidator(store, validatorAddress)→exists→"validator already exists"getValidatorByConsensusPubkey(store, pubKey)→exists→errDuplicateConsensusKey
That EVM address and pubkey are permanently bricked from validator creation.
Cosmos parity gap
sei-cosmos x/staking/keeper/delegation.go (Unbond) does:
if validator.DelegatorShares.IsZero() && validator.IsUnbonded() {
k.RemoveValidator(ctx, validator.GetOperator())
}This PR's undelegate (staking.go:340-395) has no equivalent zero-shares removal path. Given the PR's explicit goal of Cosmos parity for the semantics it models, this is a genuine divergence.
Step-by-step proof
- Operator
OcallscreateValidator(pubKey, ..., minSelfDelegation=1, value=1e12 wei). AfterstakingValue,selfDelegation = 1 usei. ValidatorVis persisted withTokens=1, Shares=1, Status=bondStatusUnbonded(staking.go:466-482). - End-block runs. In
validatorsByPower(endblock.go),validatorPower(V) = 1/1_000_000 = 0, soVis filtered out.Vnever entersvalidatorQueueIndexKey. - Operator calls
undelegate(V, 1).addDelegationreduces (O,V) delegation to 0 and deletes the delegation record.addValidatorTokens(state.go:130) reducesV.TokensandV.DelegatorSharesto 0 and callssetValidator— no removal path exists here.V.Status != Bonded, so no pool moves.addUnbondingDelegationqueues an entry. - At maturity,
completeUnbondingtransfers 1 usei back to O and deletes the unbonding record.Vis untouched. - Operator retries
createValidatorwith samepubKey.staking.go:432-441—getValidatorreturnsexists=true(V still invalidatorsIndexKey); if the operator regenerated pubkey,getValidatorByConsensusPubkeystill rejects the old one. Both operator address and consensus pubkey are permanently locked.
Fix
The straightforward Cosmos-parity fix is in addValidatorTokens (state.go:130): after updating tokens/shares, if shares.Sign() == 0 && validator.Status != bondStatusBonded, call removeValidator instead of setValidator. Alternatively, add the check in undelegate (staking.go:381) after addValidatorTokens returns. Cosmos does the former.
Why nit
- The evm-only staking precompile is explicitly experimental; the README documents no slashing/rewards/jailing yet, and
ValidatorUpdatesis not yet consumed by the consensus runtime. - No fund safety issue:
completeUnbondingcorrectly returns escrowed usei. - Blast radius is bounded to the operator's own EVM address plus their pubkey — no cross-user impact.
- The fix is small and localized. Worth landing before this backs production state.
04870c2 to
83c1b62
Compare
There was a problem hiding this comment.
Adds the first SDK-free staking custom precompile for the evm-only executor, with storage-backed module state and an end-block hook; the implementation is careful and well-tested (including a full delegate→redelegate→undelegate lifecycle e2e). No blocking issues found — only minor fidelity gaps, mostly documented divergences from Cosmos.
Findings: 0 blocking | 6 non-blocking | 2 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Self-undelegate/redelegate paths do not enforce MinSelfDelegation on the operator's own delegation (Codex #2). This diverges from Cosmos, which jails the validator when self-delegation drops below the minimum. The README documents that jailing/slashing are not modeled, so this is an accepted limitation rather than a bug, but it is worth calling out explicitly in the code near undelegate/redelegate so future readers know the omission is intentional.
- Registering any custom precompile forces the whole executor onto the sequential path (useOCC returns false whenever hasCustomPrecompiles() is true), disabling OCC parallelism for all blocks. This appears intentional/by-design given the storage-backed store and end-block hook, but it is a meaningful performance tradeoff worth flagging.
- cursor-review.md is empty (Cursor produced no output) and REVIEW_GUIDELINES.md is empty/missing, so no repo-specific guidelines were applied. No prompt-injection content was found in the PR.
- Reward-related events (DelegationRewardsWithdrawn) are emitted with a zero amount and shares track tokens 1:1 with no slashing divergence; these are documented in the README limitations and are fine for this stage.
- 2 suggestion(s)/nit(s) flagged inline on specific lines.
| record.ValidatorSrcAddress = srcValidator | ||
| record.ValidatorDstAddress = dstValidator | ||
| record.Entries = append(record.Entries, RedelegationEntry{ | ||
| CreationHeight: 0, |
There was a problem hiding this comment.
[suggestion] Redelegation entries hardcode CreationHeight: 0, unlike addUnbondingDelegation (line 164) which records the actual block height via saturatingInt64FromUint64(creationHeight). Since redelegate also never threads the block number through, redelegations/redelegation query responses will always report a creation height of 0. Consider passing ctx.Block.Number into addRedelegation and storing saturatingInt64FromUint64(...) here for parity with unbonding entries and Cosmos. (Low severity — data fidelity only.)
| if err != nil { | ||
| return nil, err | ||
| } | ||
| if minSelfDelegation.Cmp(tokens) > 0 { |
There was a problem hiding this comment.
[nit] Note (not a bug): this validates the new minSelfDelegation against validator.Tokens, which includes external delegations, not just the operator's self-delegation — so a validator with tiny self-stake but large external delegations can raise its minimum above its real self-delegation. This exactly mirrors Cosmos SDK msgServer.EditValidator (which also compares against validator.Tokens), so it's faithful replication rather than a defect. Flagging for awareness in case stricter semantics are desired for the evm-only path.
83c1b62 to
58207d2
Compare
There was a problem hiding this comment.
A large, well-structured PR adding an SDK-free staking precompile to the evm-only executor with thorough lifecycle tests. The main concern is that validator consensus pubkeys are stored and emitted to consensus without any length/type validation, which can produce malformed consensus-critical validator updates.
Findings: 1 blocking | 4 non-blocking | 1 posted inline
Blockers
- None at the file/PR level.
- 1 blocking issue(s) flagged inline on specific lines.
Non-blocking
- ValidatorUpdates are lost on the legacy
ResultSinkpath (Codex #1):sinkBlockResultin executor.go only persists ChangeSet+Receipts for sinks that are not aBlockResultSink, so validator-set output (now populated onBlockResult) is silently dropped. No productionResultSink-only implementer exists yet, so this is a latent design gap rather than an active bug — but since these updates are consensus-critical, consider either extendingResultSinkto carry them or having the executor refuse to register custom precompiles (which emit validator updates) when only a plainResultSinkis configured. - Missing negative test coverage for invalid consensus pubkeys:
guardrails_test.gocovers duplicate keys but not empty/wrong-length keys. Add a test assertingcreateValidatorrejects an empty and a non-32-byte pubkey once validation is added. - Second-opinion passes:
cursor-review.mdis empty (Cursor produced no output) andREVIEW_GUIDELINES.mdis empty, so no repo-specific guidelines were applied. Codex's two findings are both incorporated above and inline. - Minor:
RequiredGasfully unpacks the input viaprepareandRununpacks again, so every state-changing call ABI-decodes its arguments twice — acceptable, but worth noting for a hot path.
| commissionMaxRate := args[3].(string) | ||
| commissionMaxChangeRate := args[4].(string) | ||
| minSelfDelegation := args[5].(*big.Int) | ||
| pubKey, err := hex.DecodeString(pubKeyHex) |
There was a problem hiding this comment.
[blocker] The consensus pubkey is only validated for hex decodability — there is no length/type check (Codex #2). hex.DecodeString("") returns an empty slice with no error, so a validator can be created with an empty (or arbitrary-length) ConsensusPubkey, which is then persisted and later emitted verbatim as ValidatorUpdate.PubKey in endblock.go:222. Cosmos/Tendermint require a valid 32-byte ed25519 consensus key; emitting an empty or malformed key into the validator-set update is consensus-critical and can corrupt or halt the set. Validate the expected key type and length (e.g. reject empty and enforce the 32-byte ed25519 length) before storing the validator.
58207d2 to
f83bde2
Compare
f83bde2 to
6077fa8
Compare
There was a problem hiding this comment.
This PR adds a large, well-structured SDK-free staking precompile for the evm-only executor with solid test coverage, but it diverges from the Cosmos staking semantics it aims to replicate in two confirmed ways: undelegation never jails a validator whose self-delegation drops below MinSelfDelegation, and delegation never enforces MaxVotingPowerRatio. The precompile is not yet wired into any production executor path (only tests construct the registry), so these are pre-integration correctness gaps rather than live vulnerabilities.
Findings: 2 blocking | 6 non-blocking | 3 posted inline
Blockers
- None at the file/PR level.
- 2 blocking issue(s) flagged inline on specific lines.
Non-blocking
- Cursor's second-opinion review (cursor-review.md) is empty — that pass produced no output. REVIEW_GUIDELINES.md is also empty, so no repo-specific standards were applied. Codex's two High findings were both reviewed and confirmed against the sei-cosmos keeper.
- Gas metering: registeredCustomPrecompile charges a flat readGas/writeGas + per-input-byte cost, but storageBackedStore.Set/Delete and the string-index rewrites write unbounded amounts of state (validator/delegation index lists grow O(n) and are rewritten on every add/remove) without per-SSTORE gas. This is fine while unwired, but will need real gas accounting before the precompile is exposed in production, or a caller can force large writes cheaply.
- Index maintenance is O(n) per mutation (getStringList reads+sorts the whole list, addStringListItem/removeStringListItem rewrite it), so validators-by-power, per-validator delegation lists, and the redelegation global index scale poorly as membership grows; acceptable for now but worth revisiting before mainnet volumes.
- Pagination (pageStrings) uses a positional integer nextKey into a sorted list, so entries added/removed between paged calls can be skipped or duplicated. Acceptable for a view helper but differs from Cosmos key-based pagination; consider documenting.
- README already documents that rewards, slashing, and jailing are unmodeled and DelegationRewardsWithdrawn events are emitted with zero amount — the jailing gap (see inline) is a correctness divergence rather than merely an unmodeled feature, so it deserves stronger treatment than the current doc note.
- 1 suggestion(s)/nit(s) flagged inline on specific lines.
| if err := validateDelegationAmount(ctx.Store, delegator, validatorAddress, amount); err != nil { | ||
| return nil, err | ||
| } | ||
| if err := addDelegation(ctx.Store, delegator, validatorAddress, new(big.Int).Neg(amount)); err != nil { |
There was a problem hiding this comment.
[blocker] Confirmed vs the reference keeper: undelegate reduces the delegation/self-stake but never jails the validator when its self-delegation falls below MinSelfDelegation. Cosmos Keeper.Unbond (sei-cosmos x/staking/keeper/delegation.go) checks isValidatorOperator && !validator.Jailed && TokensFromShares(...) < MinSelfDelegation and calls jailValidator. Here the operator-as-self-delegator case is never detected and Jailed is never set, so a validator can drop its self-delegation below MinSelfDelegation and remain bonded and eligible for the active set. Add the self-delegation-below-minimum jailing check before this precompile is wired into an executor.
| if err := addDelegation(ctx.Store, delegator, validatorAddress, useiAmount); err != nil { | ||
| return nil, err | ||
| } | ||
| if err := addValidatorTokens(ctx.Store, validatorAddress, useiAmount); err != nil { |
There was a problem hiding this comment.
[blocker] Confirmed vs the reference keeper: delegate commits the delegation and validator tokens without enforcing MaxVotingPowerRatio / MaxVotingPowerEnforcementThreshold. Cosmos Keeper.Delegate computes the validator's new power ratio and rejects with ErrExceedMaxVotingPowerRatio once total power is past the enforcement threshold. These params exist on Params but are never read on the delegate path, so a single validator can exceed the configured voting-power cap. Enforce the cap here (and on createValidator's self-delegation) to match keeper semantics.
| HistoricalEntries: 10_000, | ||
| BondDenom: bondDenom, | ||
| MinCommissionRate: "0.000000000000000000", | ||
| MaxVotingPowerRatio: "0.000000000000000000", |
There was a problem hiding this comment.
[suggestion] The default MaxVotingPowerRatio and MaxVotingPowerEnforcementThreshold are hard-coded to "0.000000000000000000", whereas sei-cosmos defaults are MaxVotingPowerRatio = 0.2 and threshold 1000000. Independent of the missing enforcement (see delegate), these defaults would not reproduce Cosmos behavior even once enforcement is added — align the defaults with the chain's staking defaults.
| } | ||
| record.DelegatorAddress = delegator | ||
| record.ValidatorSrcAddress = srcValidator | ||
| record.ValidatorDstAddress = dstValidator | ||
| record.Entries = append(record.Entries, RedelegationEntry{ | ||
| CreationHeight: 0, | ||
| CompletionTime: completionTime, | ||
| InitialBalance: amount.String(), | ||
| SharesDst: amount.String(), | ||
| }) | ||
| if err := util.SetJSON(store, redelegationKey(delegator, srcValidator, dstValidator), record); err != nil { | ||
| return err | ||
| } | ||
| if err := addStringListItem(store, redelegationsIndexKey(), redelegationID(delegator, srcValidator, dstValidator)); err != nil { | ||
| return err | ||
| } | ||
| return insertRedelegationQueue(store, completionTime, delegator, srcValidator, dstValidator) | ||
| } | ||
|
|
||
| func getRedelegation(store precompiles.Store, delegator string, srcValidator string, dstValidator string) (Redelegation, bool, error) { |
There was a problem hiding this comment.
🟡 The redelegation entry in addRedelegation (state.go:194) hardcodes CreationHeight: 0, while the parallel addUnbondingDelegation (state.go:164) correctly threads saturatingInt64FromUint64(creationHeight) from the caller. redelegate() at staking.go:331 also doesn't pass ctx.Block.Number down, so ABI-exposed RedelegationEntry.creationHeight returned by redelegations()/redelegation() queries will always be 0 for every redelegation. Fix by adding a creationHeight uint64 parameter to addRedelegation and threading ctx.Block.Number from redelegate() like the unbonding path already does. Funds and bookkeeping are unaffected — this is purely query-response fidelity.
Extended reasoning...
The gap
addRedelegation in giga/evmonly/precompiles/staking/state.go:189-208 writes each new redelegation entry with a literal CreationHeight: 0:
record.Entries = append(record.Entries, RedelegationEntry{
CreationHeight: 0,
CompletionTime: completionTime,
InitialBalance: amount.String(),
SharesDst: amount.String(),
})The sibling function addUnbondingDelegation (state.go:164) — modeled on the same Cosmos pattern — does the right thing:
record.Entries = append(record.Entries, UnbondingDelegationEntry{
CreationHeight: saturatingInt64FromUint64(creationHeight),
...
})The plumbing is also missing at the caller: redelegate() in staking.go:326-334 calls addRedelegation(ctx.Store, delegator, srcValidator, dstValidator, amount, util.SaturatingCompletionTime(ctx.Block.Time, params.UnbondingTime)) — no ctx.Block.Number argument. The unbonding path at staking.go correctly passes ctx.Block.Number through.
Why this is user-visible
The ABI declares RedelegationEntry.creationHeight as int64 (see abi.json), and the redelegations() and redelegation() view methods return it. Every EVM client walking historical redelegation state via the ABI will read a creation height of 0 regardless of when the redelegation actually happened — the field is dead. Cosmos x/staking records the real block height here.
Why nit and not normal
Funds and bookkeeping remain correct — the entry still contains the correct CompletionTime, InitialBalance, and SharesDst, and the end-block maturity logic keys off CompletionTime, not CreationHeight. Nothing crashes and nothing gets locked. Clients that render creation-height metadata will just show 0; that's a UI defect, not a safety issue. This lines up with the maintainer's own three timeline notes flagging this at low/nit severity ('data fidelity only').
Step-by-step proof
- Delegator D delegates 7 usei to validator V1 at block 3 (t=150). Delegation record is created; no redelegation entry yet.
- At block 4 (t=200), D redelegates 3 usei from V1 to V2 by calling
redelegate(V1, V2, 3). redelegate()(staking.go:326-334) computescompletionTime = 200 + 1_814_400and callsaddRedelegation(ctx.Store, delegator, V1, V2, 3, completionTime)—ctx.Block.Number == 4is not passed.addRedelegation(state.go:194) writes an entry withCreationHeight: 0, CompletionTime: 2014600, InitialBalance: "3", SharesDst: "3".- Client calls the ABI
redelegations(delegator, V1, V2, nil)query. The returnedRedelegationEntry.creationHeightis 0, not 4. Cosmosx/stakingwould report 4. - Same flow for any subsequent redelegation — every entry reports height 0 forever.
Fix
Two-line change:
- state.go:189: change
func addRedelegation(store precompiles.Store, delegator string, srcValidator string, dstValidator string, amount \*big.Int, completionTime int64) errorto add acreationHeight uint64parameter. - state.go:194: replace
CreationHeight: 0withCreationHeight: saturatingInt64FromUint64(creationHeight). - staking.go:331: thread
ctx.Block.Numberthrough:addRedelegation(ctx.Store, delegator, srcValidator, dstValidator, amount, ctx.Block.Number, util.SaturatingCompletionTime(ctx.Block.Time, params.UnbondingTime)).
This exactly mirrors the shape of addUnbondingDelegation and its caller.
| pubKey, err := hex.DecodeString(pubKeyHex) | ||
| if err != nil { | ||
| return nil, errors.New("invalid public key hex format") | ||
| } |
There was a problem hiding this comment.
🟡 createValidator (staking.go:410) accepts any hex-decodable pubkey with no length or format check. hex.DecodeString("") returns a zero-length slice without error, and any wrong-length blob (e.g. the 4-byte "01020304" used in the PR's own tests) is stored verbatim on Validator.ConsensusPubkey and later emitted as ValidatorUpdate.PubKey via validatorUpdate() in endblock.go. Cosmos x/staking enforces the consensus key type/length (ed25519 = 32 bytes) at create time; consider validating the decoded pubkey length here before persisting. Latent today because no runtime consumes the executor's ValidatorUpdates yet, so nit — but worth landing before this backs production state.
Extended reasoning...
The bug
At giga/evmonly/precompiles/staking/staking.go:410, createValidator performs only a hex decode with no length or format validation:
pubKey, err := hex.DecodeString(pubKeyHex)
if err != nil {
return nil, errors.New("invalid public key hex format")
}hex.DecodeString("") returns []byte{}, nil (no error), and any even-length hex string decodes to a byte slice of the corresponding size with no error. The decoded bytes are then persisted directly on Validator.ConsensusPubkey (staking.go:467-482) and end up as ValidatorUpdate.PubKey verbatim in validatorUpdate() at endblock.go:220-225. Cosmos x/staking msg_server.CreateValidator instead constructs a typed cryptotypes.PubKey via PubKey.GetCachedValue() and its TypeURL gate, which enforces that (e.g.) ed25519 keys are exactly 32 bytes and secp256k1 keys are 33 bytes.
Why existing code doesn't catch it
- No caller between the ABI parse and
hex.DecodeStringinspects the length. setValidator/getValidatorByConsensusPubkeytreat the bytes opaquely, so any wrong-length slice is a valid duplicate-check key but not a valid consensus pubkey.- The end-block path copies the bytes into
precompiles.ValidatorUpdate{PubKey: ...}with no validation.
Step-by-step proof
- Caller invokes
createValidator("", "moniker", "0.1", "0.2", "0.01", 1)with a payable value of 1 SEI (1e18 wei). hex.DecodeString("")returns[]byte{}, nil— no error is raised.getValidatorreports no existing operator;getValidatorByConsensusPubkey([]byte{})reports no duplicate (this is itself a smell — an empty pubkey uniquely identifies this validator forever).- The validator is persisted with
ConsensusPubkey: []byte{}and self-stake 1 usei. - Later, when self-stake crosses
powerReduction,applyAndReturnValidatorSetUpdatesinendblock.goemitsValidatorUpdate{PubKey: []byte{}, Power: N}. - Once a consensus runtime consumes these
ValidatorUpdates, the empty (or malformed) key produces an invalid consensus update.
The PR's own test file exercises this — TestExecutorStakingDelegationLifecycleE2E uses 4-byte pubkeys "01020304" and "05060708" and asserts they flow through end-block as ValidatorUpdate.PubKey. Those tests pass today precisely because there is no length check.
Fix
Add a length check after hex.DecodeString, e.g.:
pubKey, err := hex.DecodeString(pubKeyHex)
if err != nil {
return nil, errors.New("invalid public key hex format")
}
if len(pubKey) != ed25519.PubKeySize { // 32
return nil, errors.New("consensus pubkey must be 32 bytes")
}Or, to match the Cosmos surface more closely, take a TypeURL+bytes pair and enforce the length matching the declared type.
Why nit
- The staking precompile is documented as experimental in
giga/evmonly/README.md; no runtime currently consumes the executor'sValidatorUpdates, so malformed keys sit in module-like state without reaching consensus today. - The same issue has already been raised twice on this PR by seidroid (originally as
[blocker]on 2026-06-30, downgraded to[suggestion]on 2026-07-02) and by Codex. The author is aware; the outstanding comment thread is the right vehicle for tracking the fix. - The fix is a two-line length guard, but merging without it does not cause any concrete failure today — only latent when
ValidatorUpdatesgets wired to a real runtime.
Address review findings on the SDK-free staking precompile: - redelegate/undelegate now enforce Cosmos guardrails: reject self-redelegation, transitive redelegation, and over-MaxEntries unbonding/redelegation entries. - delegation shares are reported as an sdk.Dec (scaled by 10^precision) to match the keeper-backed precompile. - create/editValidator validate commission bounds, the 24h rate-change rule, and min-self-delegation increase/cap; stricter decimal parsing rejects fraction/scientific forms. - historicalInfo query is read-only again; historical info is tracked and pruned in the end-block hook. - powerReduction set to 1_000_000 (Sei DefaultPowerReduction); tests use SEI-scale stakes. - validators query no longer re-reads validators; README documents the staking parity limitations. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
6077fa8 to
8749485
Compare
| if recordDelegator == delegator && dst == validator { | ||
| return true, nil | ||
| } | ||
| } |
There was a problem hiding this comment.
Redelegation guard ignores maturity
Medium Severity
hasReceivingRedelegation treats any redelegation index id targeting the source validator as still “receiving,” without checking whether entries are past CompletionTime at the current block time. Redelegations that are already mature can still be rejected until end-block index cleanup runs, which diverges from Cosmos-style checks during transaction delivery.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 8749485. Configure here.
There was a problem hiding this comment.
Adds a well-structured, thoroughly-tested SDK-free staking precompile for the evm-only executor; no clear correctness/security blockers were found, but there are a few consistency and persistence gaps worth addressing. REVIEW_GUIDELINES.md and cursor-review.md were both empty; Codex's three findings were partially confirmed.
Findings: 0 blocking | 6 non-blocking | 1 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Persistence gap (Codex #3):
Executor.sinkBlockResultonly storesChangeSetandReceiptswhen the sink is a plainResultSink(not aBlockResultSink); the newresult.ValidatorUpdatesare silently dropped on that path. Since validator-set updates are now consensus-relevant, either persist them on the non-BlockResultSink path or explicitly document that consumers needing validator updates must implementBlockResultSink. - Self-delegation/jailing gap (Codex #1, partial):
undelegate/redelegatecan push an operator's self-delegation belowMinSelfDelegationwithout jailing or rejecting. This is consistent with the README's stated 'no slashing/jailing' limitation, so acceptable for now, but worth tracking before this path is production-facing. Note: Codex's secondary claim thateditValidatordiverges by checkingminSelfDelegationagainst totalvalidator.Tokensis not a divergence — CosmosEditValidatoralso validates againstvalidator.Tokens. - Reward/slashing modeling:
DelegationRewardsWithdrawnevents are emitted with a hardcoded zero amount and shares track tokens 1:1 with no slash-driven divergence. These are documented limitations in the README; flagging for visibility since downstream consumers reading these events may be misled. - Scale note: several operations do full O(n) global list scans (
hasReceivingRedelegationover all redelegations, validator/delegation index iteration, historical-info pruning). Fine for current validator/delegation counts but may need revisiting if these sets grow large. - Tooling note:
REVIEW_GUIDELINES.mdwas empty (no repo-specific guidelines applied) andcursor-review.mdwas empty (the Cursor pass produced no output); only the Codex second-opinion pass had content. - 1 suggestion(s)/nit(s) flagged inline on specific lines.
| record.ValidatorSrcAddress = srcValidator | ||
| record.ValidatorDstAddress = dstValidator | ||
| record.Entries = append(record.Entries, RedelegationEntry{ | ||
| CreationHeight: 0, |
There was a problem hiding this comment.
[suggestion] RedelegationEntry.CreationHeight is hardcoded to 0 for every redelegation, while the unbonding path (addUnbondingDelegation) records the actual block height via saturatingInt64FromUint64(creationHeight). This makes the redelegations query return incorrect historical data and is inconsistent with unbonding. Consider threading block.Number into addRedelegation and setting CreationHeight accordingly. (Confirmed by Codex.)
Superseded: latest AI review found no blocking issues.
27b6097 to
0f4741e
Compare
## Summary Adds dynamic gas accounting for evm-only custom precompiles, with the staking precompile using the shared meter through the existing store, balance-transfer, and log boundaries. ## Changes - Add a reusable precompile gas meter for base gas, SLOAD/SSTORE-style storage costs, keccak slot derivation, native value transfers, and log emission. - Wrap `storageBackedStore`, `nativeBalanceTransfer`, and log emission in the custom precompile adapter so gas is charged based on the actual execution path. - Reduce staking `RequiredGas` to only base/input gas, avoiding double charging now that state access is dynamically metered. - Add executor coverage for dynamic store gas and update staking lifecycle tests to use realistic gas limits. ## Validation - `go test ./giga/evmonly/...` - `golangci-lint v2.8.0 run ./giga/evmonly/...`
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 2 potential issues.
There are 4 total unresolved issues (including 2 from previous reviews).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit b2232ec. Configure here.
| } | ||
| if err := addRedelegation(ctx.Store, delegator, srcValidator, dstValidator, amount, util.SaturatingCompletionTime(ctx.Block.Time, params.UnbondingTime)); err != nil { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
Redelegation ignores source bond status
Medium Severity
redelegate always creates a redelegation entry with now + UnbondingTime, and never consults the source validator's bond status. Cosmos getBeginInfo completes immediately when the source is unbonded, and uses the validator's remaining unbonding time/height when the source is unbonding. That mismatch over-locks redelegations and keeps hasReceivingRedelegation blocking transitive redelegates longer than Cosmos does.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit b2232ec. Configure here.
| CompletionTime: completionTime, | ||
| InitialBalance: amount.String(), | ||
| SharesDst: amount.String(), | ||
| }) |
There was a problem hiding this comment.
Redelegation creation height always zero
Low Severity
addRedelegation hardcodes CreationHeight to 0 on every entry, while unbonding entries correctly record the block height. Redelegation queries will always report a zero creation height, which does not match Cosmos or the unbonding path in this same package.
Reviewed by Cursor Bugbot for commit b2232ec. Configure here.
There was a problem hiding this comment.
Beyond the inline findings (a gosec lint failure and a dead-code nit), this run also examined two previously-flagged concerns and ruled them out: hasReceivingRedelegation not checking entry maturity mid-block is consistent with Cosmos, since mature redelegation entries are removed by the end-block completion hook before any transaction can observe them; and undelegate not jailing a validator whose self-delegation falls below MinSelfDelegation matches the README's documented decision not to model jailing, rather than being an undisclosed gap.
Extended reasoning...
This is a follow-up note, not a full review. The gosec G115 finding on giga/evmonly/occ.go:134 (confirmed by running golangci-lint against this repo's own config — it is the only issue reported in the package, and is CI-blocking per make lint) and the dead isTransaction helper in helpers.go are posted as inline comments and are the primary items needing attention. I additionally checked two candidates raised by finder agents against the current code: state.go's hasReceivingRedelegation only appears not to check CompletionTime because matured redelegation entries are already deleted by completeMatureRedelegations in the end-block hook, so any record still present mid-block is genuinely still in progress. And the lack of jailing on self-delegation dropping below MinSelfDelegation in undelegate is called out in README.md as an explicit, intentional scope limitation (no slashing/jailing modeled) rather than an unnoticed omission. This does not resolve or supersede the outstanding blocker-level concerns raised by other reviewers earlier in the thread (e.g. missing voting-power-cap enforcement, consensus pubkey validation) which remain unaddressed.
| req.Context, | ||
| p, | ||
| txIndex, | ||
| uint(txIndex), |
There was a problem hiding this comment.
🔴 giga/evmonly/occ.go:134 passes the loop variable txIndex (int) directly into uint(txIndex) without a //nolint:gosec suppression, which trips golangci-lint's gosec G115 (integer overflow conversion int -> uint) and fails make lint/CI as configured in this repo.
Extended reasoning...
I independently reproduced this by running golangci-lint run ./giga/evmonly/... against the repo's own .golangci.yml (gosec enabled, run.tests: false, with only a 'weak random number generator' text exclusion for gosec — no G115 exclusion). The only issue reported in the entire package is:
giga/evmonly/occ.go:134:7: G115: integer overflow conversion int -> uint (gosec)
The offending call is inside executeTxSpeculative (added by this PR in occ.go), which is invoked once per transaction from the OCC speculative-execution path in executeBlockOCC:
txResult, receipt, err := e.executeTx(
evm,
stateDB,
gasPool,
req.Context,
p,
txIndex,
uint(txIndex), // <-- flagged: int -> uint conversion, no suppression
baseFee,
)txIndex is the plain int loop index coming from idx := txRange.start; idx < txRange.end in executeBlockOCC's worker callback. gosec's G115 check flags any int->uint (or similarly narrowing/sign-changing) conversion that isn't provably bounded at compile time or explicitly suppressed, because a negative int silently wraps to a huge uint. run.tests: false does not exempt this from CI because occ.go is production code, not a _test.go file — the four other G115 hits from similar patterns in executor_test.go are excluded specifically for that reason, but this one isn't.
What makes this a real gap rather than an inherent false positive: the PR's own sequential-execution path in executor.go avoids the same warning by maintaining a separately incremented uint counter (txIndexUint) alongside the int loop variable, rather than converting the loop index inline:
var txIndexUint uint
for txIndex, p := range req.Txs {
...
txResult, receipt, err := e.executeTx(evm, stateDB, gasPool, req.Context, p, txIndex, txIndexUint, baseFee)
...
txIndexUint++
}That pattern doesn't trip gosec because there's no int->uint conversion for it to flag. The author also applied //nolint:gosec with bounds-justification comments to numerous other genuinely-bounded int/uint conversions introduced elsewhere in this same PR (e.g. precompile_gas.go's uint64(size), state.go's saturatingInt64FromUint64, executor_test.go's initCode's byte(len(runtime))), showing gosec is active and expected to be satisfied throughout this PR — this occ.go:134 site is simply the one call that was missed, with neither the separate-counter pattern nor a //nolint:gosec suppression applied.
Impact: per AGENTS.md, make lint (golangci-lint v2.8.0 + go vet + gofmt + go mod tidy/verify) is a required check, and .github/workflows/golangci.yml runs golangci-lint in CI mirroring this exact configuration. As it stands, this PR will fail that CI job on this line alone, blocking merge until fixed.
Fix: either thread a separately-incremented uint counter through the OCC speculative path (mirroring executor.go's txIndexUint pattern — arguably preferable since txIndex is already known non-negative and bounded by len(req.Txs)), or add //nolint:gosec // txIndex is bounded by len(req.Txs) next to the conversion, matching the suppression style used elsewhere in this PR.
Step-by-step proof:
- Checkout this PR's branch/commit and run
golangci-lint run ./giga/evmonly/...using the repo's checked-in.golangci.yml. - Observe the sole finding:
giga/evmonly/occ.go:134:7: G115: integer overflow conversion int -> uint (gosec). - Confirm
run.tests: falsedoesn't apply —occ.gohas no_test.gosuffix. - Confirm no exclusion applies — the only gosec exclusion rule in
.golangci.ymlmatches the text 'Use of weak random number generator', unrelated to G115. - Confirm
make lintinvokes golangci-lint over the whole module (includinggiga/evmonly), and CI (.github/workflows/golangci.yml) runs the same tool/config, so this finding is CI-blocking as-is.
| func isTransaction(method string) bool { | ||
| switch method { | ||
| case DelegateMethod, RedelegateMethod, UndelegateMethod, CreateValidatorMethod, EditValidatorMethod: | ||
| return true | ||
| default: | ||
| return false | ||
| } |
There was a problem hiding this comment.
🟡 The isTransaction helper in helpers.go is dead code — it has zero call sites anywhere in the repo. It looks like a leftover from porting the sibling precompiles/common convention (DefaultGasCost), where an equivalent flag drives different write-vs-view gas costs, but this precompile's RequiredGas always charges the same baseGas+inputByteGas*len(input) regardless of method. Either remove the unused function or wire it into RequiredGas to differentiate write/view gas.
Extended reasoning...
isTransaction(method string) bool is defined at giga/evmonly/precompiles/staking/helpers.go:65-71 and switches on the five state-mutating methods (delegate, redelegate, undelegate, createValidator, editValidator), returning true for those and false for view/query methods like validators, validator, pool, params, etc. A repo-wide grep for isTransaction turns up exactly one hit in this package — its own definition — confirming it is never called from anywhere in giga/evmonly or the rest of the repo.
The shape of the function strongly suggests it was ported from the sibling (non-giga) precompile convention in precompiles/common/precompiles.go, where DefaultGasCost takes an isTransaction bool and charges a different flat-plus-per-byte gas cost depending on whether the method mutates state or just reads it. This new staking precompile's actual gas accounting, however, lives in RequiredGas at staking.go and unconditionally returns baseGas + inputByteGas*len(input) for every method — it never branches on write vs. view, and never calls isTransaction. So the helper reads like an incomplete step in that port: the flag was written, but the branch that would consume it in RequiredGas was never added (or was simplified away, leaving the flag orphaned).
This does not break the build or fail CI as configured here: Go only errors on unused imports and local variables, not unused unexported package-level functions, so the package compiles cleanly. It also does not trip make lint/golangci-lint, because .golangci.yml enables staticcheck but not the separate unused linter (which is what carries the U1000 dead-code check in golangci-lint v2) — running golangci-lint against giga/evmonly/precompiles/staking reports 0 issues.
Step-by-step: (1) grep the repo for isTransaction — the only occurrence is the definition at helpers.go:65; every other match anywhere in the tree is an unrelated isTransaction bool parameter inside the legacy (non-giga) precompiles/common packages. (2) Trace RequiredGas in staking.go: it calls p.prepare(input) to validate the method exists, then always returns baseGas + inputByteGas*uint64(len(input)) — no reference to isTransaction or any write/view distinction. (3) Confirm the package still builds and lints (go build ./giga/evmonly/..., golangci-lint run ./giga/evmonly/precompiles/staking/...) — both succeed, so the dead code silently persists with no fix incentive from tooling.
This is a pure code-cleanliness issue, not a functional defect: charging the same gas for write and view calls is a legitimate (if divergent-from-Cosmos) design choice on its own, and nothing in the current code path is incorrect or unsafe as a result. The fix is small either way — delete the unused function, or thread a write/view gas differential into RequiredGas using it — but it is not something that needs to block this PR from merging.
2 participants
Summary
Adds the first SDK-free custom precompile for the evm-only executor: staking at
0x0000000000000000000000000000000000001005.This PR wires custom precompile execution into the evm-only executor, stores precompile module-like state as storage owned by the precompile address, and adds an end-block hook for staking validator-set updates and delayed redelegation/undelegation completion. It also keeps staking token handling explicitly usei-only for the evm-only path.
Details
giga/evmonly/precompiles/util.Validation
go test ./giga/evmonly/...