feat(staking): add staking contract - #482
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughChangesThe PR adds a genesis staking rWasm contract with ABI dispatch, namespaced storage, validator lifecycle management, delegation and rewards, consensus committees, liveness and equivocation slashing, governance configuration, genesis integration, unit tests, and end-to-end staking coverage. Genesis staking contract
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Beneficiary
participant StakingContract
participant BLSVerifier
Beneficiary->>StakingContract: commitEquivocationReport(commitment)
StakingContract-->>Beneficiary: store commitment and block number
Beneficiary->>StakingContract: slashEquivocation*(evidence, beneficiary, salt)
StakingContract->>StakingContract: verify commitment maturity and match
StakingContract->>BLSVerifier: verify evidence signatures
StakingContract-->>Beneficiary: pay committed reporter reward
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Criterion results (vs baseline)Heads-up: runner perf is noisy; treat deltas as a smoke check. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (6)
contracts/staking/src/rewards.rs (2)
683-692: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCollapse the two consecutive
if !assigned.is_zero()blocks.Both branches test the same condition; merging them (with the
else { StipendSkipped }attached once) removes a redundant check and keeps disbursement and crediting visibly atomic.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@contracts/staking/src/rewards.rs` around lines 683 - 692, Merge the consecutive `if !assigned.is_zero()` blocks in the surrounding reward-stipend flow into one conditional. Keep the reserve disbursement, short-disbursement validation, and accounting credit together in the true branch, and attach the existing `StipendSkipped` else branch once so the behavior remains unchanged.
116-185: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftDeduplicate the claimable computation shared with
consume_delegator_claim.Lines 131-183 and Lines 247-307 implement the same epoch-walk math; only cursor persistence differs. Divergence between the read path (
getDelegatorFee) and the settlement path would silently mis-report or mis-pay rewards. Consider one core walk parameterized by acommit: bool(or returning the final gaps for the caller to persist).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@contracts/staking/src/rewards.rs` around lines 116 - 185, Deduplicate the epoch-walk and claimable calculation currently implemented by delegator_claimable and consume_delegator_claim into one shared helper, parameterized by whether cursor gaps should be persisted or by returning the final gaps for the caller. Keep read-only getDelegatorFee behavior non-mutating while ensuring settlement persists its updated cursors, with both paths using identical reward and undelegation calculations.e2e/src/staking.rs (1)
191-195: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSet the block number explicitly instead of relying on the default.
The comment cites block 1_400, but no
with_block_numbercall precedes it, soatEpoch == 2depends onEvmTestingContext::default()'s implicit block height. Any change to that default silently breaks this assertion and makes the comment stale.♻️ Make the epoch derivation explicit
+ context = context.with_block_number(1_400); let output = call( &mut context, OWNER, GENESIS_STAKING, IStakingRwasm::getValidatorDelegationCall { validator: VALIDATOR, delegator: OWNER, } .abi_encode(), );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@e2e/src/staking.rs` around lines 191 - 195, Update the test setup before the IStakingRwasm::getValidatorDelegationCall assertion to explicitly set the EVM block number to 1_400 using the existing with_block_number mechanism. Keep the atEpoch == 2 assertion and its epoch calculation comment aligned with that explicit block height.contracts/staking/src/consensus.rs (1)
26-47: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated
external_callhelper across two modules. The same selector-prefixing, ABI-encoding, and revert-data-propagating helper is defined twice, so any change to call semantics (gas limit, error mapping, revert bubbling) must be made in both places.
contracts/staking/src/consensus.rs#L26-L47: remove this definition and import the shared helper fromcontracts/staking/src/util.rs.contracts/staking/src/equivocation.rs#L25-L46: remove this definition and import the same shared helper.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@contracts/staking/src/consensus.rs` around lines 26 - 47, Remove the duplicated external_call helper from contracts/staking/src/consensus.rs lines 26-47 and contracts/staking/src/equivocation.rs lines 25-46, then import and use the shared external_call from contracts/staking/src/util.rs in both modules. Preserve the existing selector prefixing, ABI encoding, call behavior, and revert-data propagation through the shared implementation.contracts/staking/src/config.rs (1)
92-109: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNo event emitted for dependency changes.
Every other governance setter in this module emits a
*Changedevent;configure_dependenciessilently rewritesliveness_slashingandblend_reserve, both consensus/economics-critical. Consider emitting change events for indexer/audit parity.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@contracts/staking/src/config.rs` around lines 92 - 109, Update configure_dependencies to emit the established *Changed events after successfully setting liveness_slashing and blend_reserve, matching the event patterns used by other governance setters in this module and including both updated values.contracts/staking/src/util.rs (1)
463-559: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider extracting the shared backward snapshot scan.
touch_snapshot_at_or_beforeandvalidator_total_atimplement the same "walk back frommin(epoch, changed_at)toflooruntil a non-empty snapshot" predicate, duplicated field-by-field. A shared helper returning the resolved base epoch would keep the two in sync if the emptiness predicate ever changes. Also, lines 537-538 readchanged_at_accessortwice; reuse one read.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@contracts/staking/src/util.rs` around lines 463 - 559, Extract the duplicated backward snapshot scan from touch_snapshot_at_or_before and validator_total_at into a shared helper that returns the resolved base epoch using the existing non-empty snapshot predicate and floor bounds. Update both callers to use that helper while preserving their current result handling, and reuse the single changed_at value in validator_total_at instead of reading changed_at_accessor twice.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@contracts/staking/src/economics.rs`:
- Around line 136-148: Align the snapshot epoch used by delegate and undelegate
accounting: update the delegate flow around touch_validator_snapshot so its
total_delegated credit uses the same epoch boundary as undelegate_from’s debit
via next_epoch(current_epoch). Reuse the existing epoch calculation or shared
accounting delay, preserving overflow checks and snapshot updates.
- Around line 220-264: Update undelegate_from around the delegation queue
handling and validator snapshot updates to reject undelegations when the latest
delegator checkpoint is scheduled after before_epoch, unless the pending
delegation is explicitly cancelled and its corresponding validator-total
checkpoints are adjusted. Ensure every affected future delegator and validator
checkpoint applies the undelegation delta consistently, preventing validator
totals from retaining stake without a matching delegator balance.
In `@contracts/staking/src/handlers.rs`:
- Around line 590-597: Update the ERR_VALIDATOR_OWNER_ALREADY_IN_USE revert in
the owner_validators_accessor conflict check to pass command.value, the new
owner address, instead of command.validator. Keep the existing validation
condition and error code unchanged, matching set_validator’s payload behavior.
- Around line 505-545: Update remove_validator after the total_delegated check
to clear/reset the validator’s self-delegation delegate_queue in
validator_delegations[validator][validator]. Ensure this cleanup occurs before
the validator is removed, while preserving the existing active-delegation
validation and removal flow.
In `@contracts/staking/src/liveness.rs`:
- Around line 183-215: Update the STATUS_JAIL branch in slash so repeated
slashes extend jailed_before to the maximum of its existing deadline and the
newly calculated jail_until. Ensure this deadline extension occurs even when the
quorum halt guard skips removal, while preserving the guard behavior for
non-jailed validators.
---
Nitpick comments:
In `@contracts/staking/src/config.rs`:
- Around line 92-109: Update configure_dependencies to emit the established
*Changed events after successfully setting liveness_slashing and blend_reserve,
matching the event patterns used by other governance setters in this module and
including both updated values.
In `@contracts/staking/src/consensus.rs`:
- Around line 26-47: Remove the duplicated external_call helper from
contracts/staking/src/consensus.rs lines 26-47 and
contracts/staking/src/equivocation.rs lines 25-46, then import and use the
shared external_call from contracts/staking/src/util.rs in both modules.
Preserve the existing selector prefixing, ABI encoding, call behavior, and
revert-data propagation through the shared implementation.
In `@contracts/staking/src/rewards.rs`:
- Around line 683-692: Merge the consecutive `if !assigned.is_zero()` blocks in
the surrounding reward-stipend flow into one conditional. Keep the reserve
disbursement, short-disbursement validation, and accounting credit together in
the true branch, and attach the existing `StipendSkipped` else branch once so
the behavior remains unchanged.
- Around line 116-185: Deduplicate the epoch-walk and claimable calculation
currently implemented by delegator_claimable and consume_delegator_claim into
one shared helper, parameterized by whether cursor gaps should be persisted or
by returning the final gaps for the caller. Keep read-only getDelegatorFee
behavior non-mutating while ensuring settlement persists its updated cursors,
with both paths using identical reward and undelegation calculations.
In `@contracts/staking/src/util.rs`:
- Around line 463-559: Extract the duplicated backward snapshot scan from
touch_snapshot_at_or_before and validator_total_at into a shared helper that
returns the resolved base epoch using the existing non-empty snapshot predicate
and floor bounds. Update both callers to use that helper while preserving their
current result handling, and reuse the single changed_at value in
validator_total_at instead of reading changed_at_accessor twice.
In `@e2e/src/staking.rs`:
- Around line 191-195: Update the test setup before the
IStakingRwasm::getValidatorDelegationCall assertion to explicitly set the EVM
block number to 1_400 using the existing with_block_number mechanism. Keep the
atEpoch == 2 assertion and its epoch calculation comment aligned with that
explicit block height.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: a80eea23-80c3-4c65-99a3-e05965533565
⛔ Files ignored due to path filters (6)
contracts/Cargo.lockis excluded by!**/*.lockcrates/sdk-derive/derive-core/src/snapshots/fluentbase_sdk_derive_core__event__tests__all_indexed.snapis excluded by!**/*.snapcrates/sdk-derive/derive-core/src/snapshots/fluentbase_sdk_derive_core__event__tests__anonymous.snapis excluded by!**/*.snapcrates/sdk-derive/derive-core/src/snapshots/fluentbase_sdk_derive_core__event__tests__basic_transfer.snapis excluded by!**/*.snapcrates/sdk-derive/derive-core/src/snapshots/fluentbase_sdk_derive_core__event__tests__dynamic_indexed.snapis excluded by!**/*.snapcrates/sdk-derive/derive-core/src/snapshots/fluentbase_sdk_derive_core__event__tests__no_indexed.snapis excluded by!**/*.snap
📒 Files selected for processing (25)
contracts/README.mdcontracts/staking/Cargo.tomlcontracts/staking/README.mdcontracts/staking/src/config.rscontracts/staking/src/consensus.rscontracts/staking/src/consts.rscontracts/staking/src/economics.rscontracts/staking/src/equivocation.rscontracts/staking/src/events.rscontracts/staking/src/handlers.rscontracts/staking/src/lib.rscontracts/staking/src/liveness.rscontracts/staking/src/math.rscontracts/staking/src/rewards.rscontracts/staking/src/storage.rscontracts/staking/src/tests.rscontracts/staking/src/types.rscontracts/staking/src/util.rscrates/codec/src/lib.rscrates/genesis/build.rscrates/sdk-derive/derive-core/src/event.rscrates/sdk/src/storage/primitive.rscrates/types/src/genesis.rse2e/src/lib.rse2e/src/staking.rs
f91bbc5 to
6467f0f
Compare
293c320 to
3f0614e
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
contracts/staking/README.md (1)
67-69: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winClarify the Solidity parity warning.
“Remains affected” does not identify the actual behavior or the required deployment action. State explicitly that the Solidity self-stake lookup depends on mutable validator ownership and that owner changes must be disabled before deployment/use.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@contracts/staking/README.md` around lines 67 - 69, Update the Solidity parity warning in the staking README to explicitly state that the self-stake lookup depends on mutable validator ownership, and instruct users to disable validator ownership changes before deploying or using it as the canonical implementation.
🧹 Nitpick comments (2)
contracts/staking/src/equivocation.rs (2)
112-138: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
commit_reportsilently overwrites an existing commitment.Re-committing resets
committed_at, so a beneficiary with a mature commitment that re-commits (e.g. duplicate/retried tx) loses maturity and must wait again. Self-inflicted only, but an explicit revert or no-op on identical commitment would make the flow less surprising.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@contracts/staking/src/equivocation.rs` around lines 112 - 138, The commit_report function overwrites existing commitments and resets their maturity timestamp. Before setting commitment and committed_at, read the beneficiary’s current commitment and explicitly revert or no-op when one already exists, with identical commitments handled without resetting committed_at; preserve the existing initialization, validation, storage, and event behavior for new commitments.
89-97: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMaturity window is exactly one block.
current_block <= committed_atonly forces the reveal into a later block. That blocks same-block copy/front-running, but an observer can still commit in block N+1 after seeing your commitment in N and reveal in N+2 — they gain nothing here because the reward is bound tocommand.beneficiary, so the guarantee holds, but consider a configurable minimum delay if the intent is a real reveal delay.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@contracts/staking/src/equivocation.rs` around lines 89 - 97, Review the maturity check in the equivocation handling flow around committed_at_accessor().get_checked and make the minimum reveal delay configurable if the feature requires more than preventing same-block reveals. Use the configured delay when comparing current_block with committed_at, preserve ERR_EQUIVOCATION_COMMITMENT_NOT_MATURE and its diagnostic values, and keep the beneficiary-bound reward behavior unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@contracts/staking/README.md`:
- Around line 67-69: Update the Solidity parity warning in the staking README to
explicitly state that the self-stake lookup depends on mutable validator
ownership, and instruct users to disable validator ownership changes before
deploying or using it as the canonical implementation.
---
Nitpick comments:
In `@contracts/staking/src/equivocation.rs`:
- Around line 112-138: The commit_report function overwrites existing
commitments and resets their maturity timestamp. Before setting commitment and
committed_at, read the beneficiary’s current commitment and explicitly revert or
no-op when one already exists, with identical commitments handled without
resetting committed_at; preserve the existing initialization, validation,
storage, and event behavior for new commitments.
- Around line 89-97: Review the maturity check in the equivocation handling flow
around committed_at_accessor().get_checked and make the minimum reveal delay
configurable if the feature requires more than preventing same-block reveals.
Use the configured delay when comparing current_block with committed_at,
preserve ERR_EQUIVOCATION_COMMITMENT_NOT_MATURE and its diagnostic values, and
keep the beneficiary-bound reward behavior unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 3b065254-866b-465e-a4f5-d0aac93b46ac
📒 Files selected for processing (9)
contracts/staking/README.mdcontracts/staking/src/consts.rscontracts/staking/src/equivocation.rscontracts/staking/src/events.rscontracts/staking/src/lib.rscontracts/staking/src/storage.rscontracts/staking/src/tests.rscontracts/staking/src/types.rscontracts/staking/src/util.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- contracts/staking/src/util.rs
|
FLU-1004 is implemented in Changes:
Validation:
|
|
FLU-1005 is implemented in Root cause and fix:
Coverage:
Validation:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
contracts/staking/src/consensus.rs (1)
25-42: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate
external_callhelper across modules.This function is byte-identical to
external_callincontracts/staking/src/equivocation.rs. Sinceutil.rsalready centralizesencode_external_call/write_abi/write_returns, consider moving this wrapper (encode + call + write-on-revert) there too so both call sites share one implementation.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@contracts/staking/src/consensus.rs` around lines 25 - 42, Move the shared external_call wrapper from consensus.rs into util.rs alongside encode_external_call, write_abi, and write_returns. Update both consensus.rs and equivocation.rs to use the centralized helper, preserving its encoding, SDK call, revert-data writing, and error propagation behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@contracts/staking/src/equivocation.rs`:
- Around line 410-412: Update seize_self_stake to use only the latest cumulative
delegate_queue snapshot for the validator’s self-delegation, rather than summing
historical entries. Preserve the existing reporter, validator, and owner flow
while ensuring stake seizure uses the latest queued amount and does not include
prior snapshots.
---
Nitpick comments:
In `@contracts/staking/src/consensus.rs`:
- Around line 25-42: Move the shared external_call wrapper from consensus.rs
into util.rs alongside encode_external_call, write_abi, and write_returns.
Update both consensus.rs and equivocation.rs to use the centralized helper,
preserving its encoding, SDK call, revert-data writing, and error propagation
behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 9a3c058b-c763-4beb-99bc-6d1700fbf59b
📒 Files selected for processing (13)
contracts/staking/README.mdcontracts/staking/src/consensus.rscontracts/staking/src/consts.rscontracts/staking/src/equivocation.rscontracts/staking/src/events.rscontracts/staking/src/handlers.rscontracts/staking/src/lib.rscontracts/staking/src/storage.rscontracts/staking/src/tests.rscontracts/staking/src/types.rscontracts/staking/src/util.rscrates/codec/src/bytes_codec.rse2e/src/staking.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- contracts/staking/src/util.rs
8e17acf to
aef526e
Compare
|
FLU-996 is implemented in
Validation: staking tests (38 passed), staking clippy, release no-std wasm build, staking E2E tests (2 passed), and E2E clippy all pass. Touched-file formatting passes; the package-wide formatting check still encounters the existing unrelated |
|
FLU-997 is implemented in
Validation: staking tests (42 passed), staking/testing/E2E clippy, testing crate tests, release no-std wasm build, and staking E2E tests (2 passed) all pass. Touched-file formatting passes; package-wide formatting still encounters the existing unrelated |
|
FLU-1006 is implemented in
Validation: staking tests (44 passed), SDK event-derive tests (7 passed), staking and SDK-derive clippy with warnings denied, and touched-file rustfmt all pass. The contracts package-wide fmt check still encounters the pre-existing unrelated |
Reject undelegation while a later cumulative checkpoint is pending so validator totals and delegator balances cannot diverge. Add regression coverage for pending checkpoints and latest-only equivocation seizure.
c2f8235 to
ea7c6da
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (9)
crates/testing/src/host.rs (1)
347-363: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueNested calls surface a misleading panic.
If the installed handler itself triggers
SharedAPI::call(mock calling back into the contract), the handler is alreadytake()n and theexpectreports "call handler is not configured", which points debugging in the wrong direction. Distinguishing the two states makes mock failures self-explanatory.♻️ Suggested tweak
- let mut handler = self - .inner - .borrow_mut() - .call_handler - .take() - .expect("call handler is not configured for testing context"); + let mut handler = self.inner.borrow_mut().call_handler.take().expect( + "call handler is missing: either it was never configured, or a nested call was made \ + from within the handler itself", + );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/testing/src/host.rs` around lines 347 - 363, Update the call-handler retrieval in call to distinguish an initially unconfigured handler from a handler temporarily absent during a nested call. Preserve the existing configuration panic for the first case, but use a distinct, self-explanatory panic message when call_handler is already taken by an active handler invocation, and continue restoring the handler after the invocation.contracts/staking/src/rewards.rs (2)
19-55: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
external_call/call_decodeare re-implemented in three staking modules. All three copies do the same thing — encode selector + params,sdk.call, forward revert data, decode the result — differing only in the encoder trait bound (EncodervsFunctionArgs). One shared helper inutil.rs(alongsideencode_external_call) would keep revert-propagation and decode-error mapping uniform.
contracts/staking/src/rewards.rs#L19-L55: move both helpers intoutil.rs(generic over the encoder bound) and import them here.contracts/staking/src/consensus.rs#L23-L40: delete the localexternal_calland use the shared helper.contracts/staking/src/equivocation.rs#L177-L209: delete the localexternal_call/call_decodepair and use the shared helpers.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@contracts/staking/src/rewards.rs` around lines 19 - 55, Consolidate the duplicated external-call and result-decoding logic into shared generic helpers in util.rs alongside encode_external_call, preserving revert-data forwarding and decode-error mapping while supporting the differing Encoder and FunctionArgs bounds. In contracts/staking/src/rewards.rs#L19-55, move external_call and call_decode to util.rs and import them; in contracts/staking/src/consensus.rs#L23-40, remove the local external_call and use the shared helper; in contracts/staking/src/equivocation.rs#L177-209, remove both local helpers and use the shared implementations.
108-177: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider extracting the shared claim-walk to avoid reward-math divergence.
delegator_claimableandconsume_delegator_claimduplicate the delegate/undelegate queue traversal and payout math almost line for line; the only real difference is cursor persistence. A shared iterator/walker that takes a&mut"commit" callback (or a flag) would keep view and mutating paths provably identical.Also applies to: 223-304
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@contracts/staking/src/rewards.rs` around lines 108 - 177, Extract the duplicated delegate/undelegate queue traversal and reward calculation from delegator_claimable and consume_delegator_claim into one shared walker. Make the walker own the common payout and balance logic while accepting the caller-specific cursor commit behavior, so the read-only and mutating paths remain identical except for persistence. Update both functions to use this shared implementation.contracts/staking/src/handlers.rs (2)
165-180: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winConsider rejecting a past
dpos_activation_blockhere too.set_dpos_activation_blockguards withERR_ACTIVATION_BLOCK_IN_PAST(config.rsLines 440-442), butconfigureonly checks interval alignment, so a one-shot misconfiguration can pin activation in the past and immediately activate DPoS.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@contracts/staking/src/handlers.rs` around lines 165 - 180, Update the validation in configure near the dpos_activation_block alignment check to reject activation blocks earlier than the current block, using the existing ERR_ACTIVATION_BLOCK_IN_PAST error and the established current-block value. Preserve the interval-alignment validation for non-past activation blocks.
28-35: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDrop the redundant
InitializeCommandre-wrap. The tuple fromdecode_argsis immediately repackaged and then destructured field-by-field; using the tuple bindings directly removes a whole indirection.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@contracts/staking/src/handlers.rs` around lines 28 - 35, Remove the intermediate InitializeCommand construction after decode_args in the handler. Destructure or pass the decoded tuple bindings directly into the subsequent logic, eliminating the redundant field-by-field re-wrap while preserving the existing argument order and behavior.contracts/staking/src/math.rs (1)
50-52: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRun
cargo fmton this file. The PR notes package-wide formatting still reports a diff incontracts/staking/src/math.rs; line 52 is the likely offender (rustfmt wraps theassert_eq!args at width 100 given the nested expression). CI treats fmt/clippy as errors.As per coding guidelines: "Run
cargo fmtorcargo fmt --checkfor all touched Rust code to ensure formatting compliance."♻️ Suggested formatting
- assert_eq!(expand_balance(U112::MAX), overflow - BALANCE_COMPACT_PRECISION); + assert_eq!( + expand_balance(U112::MAX), + overflow - BALANCE_COMPACT_PRECISION + );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@contracts/staking/src/math.rs` around lines 50 - 52, Run cargo fmt on contracts/staking/src/math.rs, ensuring the assert_eq! involving expand_balance(U112::MAX) is formatted according to rustfmt’s line-width rules. Verify the touched Rust code passes cargo fmt --check without unrelated changes.Source: Coding guidelines
contracts/staking/src/config.rs (1)
106-123: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueNo event for dependency changes. Every other setter in this module emits a
*Changedevent;liveness_slashing/blend_reservemutations are silent, which hurts off-chain governance auditing.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@contracts/staking/src/config.rs` around lines 106 - 123, The configure_dependencies function updates liveness_slashing and blend_reserve without emitting change events. After successfully setting each dependency, emit the corresponding *Changed event using the module’s existing event pattern and ensure events are only emitted for mutations that complete successfully.contracts/staking/src/util.rs (1)
526-604: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDeduplicate the ERC-20 call paths.
safe_transfer_fromandsafe_transferdiffer only in the encoded input and the zero-amount shortcut; the token lookup,sdk.call, status propagation, and boolean-return validation are copy/paste. Extract a singletoken_call(sdk, input)helper.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@contracts/staking/src/util.rs` around lines 526 - 604, Extract the shared staking-token call logic from safe_transfer_from and safe_transfer into a token_call helper that accepts the SDK and encoded input, handling token lookup, sdk.call status propagation, and boolean-return validation. Update both callers to build their existing ERC-20 inputs and delegate to token_call, preserving safe_transfer’s zero-amount early return.contracts/staking/src/tests.rs (1)
2755-2807: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReduce the repeated inline base struct via a small helper.
The same
EquivocationCommandbase literal is re-inlined three times just to vary one field. A helper (or derivingCloneonEquivocationCommandand using..base.clone()) makes the intent of each case obvious.♻️ Sketch
let base = |beneficiary, salt, evidence: Bytes| EquivocationCommand { evidence, pk_uncompressed: Bytes::new(), sig1_uncompressed: Bytes::new(), sig2_uncompressed: Bytes::new(), beneficiary, salt, }; let redirected = base(front_runner, salt, evidence.clone()); let wrong_salt = base(beneficiary, B256::with_last_byte(0x53), evidence.clone()); let wrong_evidence = base(beneficiary, salt, Bytes::from_static(b"modified evidence"));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@contracts/staking/src/tests.rs` around lines 2755 - 2807, Reduce duplication in the equivocation commitment tests by defining a small local constructor/helper for the shared EquivocationCommand fields, then use it to create redirected, wrong_salt, and wrong_evidence with only their differing values specified. Preserve each case’s existing beneficiary, salt, and evidence inputs.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@contracts/staking/src/config.rs`:
- Around line 475-508: Update set_min_validator_stake_amount and
set_min_staking_amount to validate that the decoded value is a multiple of
BALANCE_COMPACT_PRECISION, matching handlers::configure, while retaining the
existing non-zero validation. Reject dust-precision values before writing
storage or emitting the change events.
In `@contracts/staking/src/handlers.rs`:
- Around line 224-263: Update configure’s change-event emissions to use the
actual previously stored configuration values instead of 0 or U256::ZERO. Read
each relevant field before overwriting it, including active_validators_length,
epoch_block_interval, felony_threshold, validator_jail_epoch_length,
undelegate_period, min_validator_stake_amount, min_staking_amount, and
dpos_activation_block, then pass those captured values as each event’s
prev_value.
- Around line 21-51: Restrict the staking initialize flow in initialize to the
trusted genesis/system caller before writing owner_accessor or any
initialization state. Validate the caller against the existing
GENESIS_GOVERNANCE or SYSTEM_CALLER mechanism and revert unauthorized
invocations, while preserving the current validation and setup behavior for the
authorized genesis transition.
In `@contracts/staking/src/util.rs`:
- Around line 505-524: Replace the per-epoch decrementing scan in
validator_total_at at contracts/staking/src/util.rs:505-524 with a stored
back-pointer lookup to the previous materialized snapshot epoch, preserving the
existing floor and value checks. Apply the same back-pointer-based lookup in
touch_snapshot_at_or_before at contracts/staking/src/util.rs:457-483; both sites
should jump directly to the prior materialized snapshot instead of iterating
epoch by epoch.
In `@e2e/src/staking.rs`:
- Around line 300-301: Update the staking test around delegation.atEpoch to
explicitly set the block to the intended pre-activation state before calling
delegate, or otherwise derive the assertion from the established current epoch
and WARMUP_DELAY setup. Make the test setup clearly demonstrate why atEpoch
equals 2 instead of relying on an implicit default block.
---
Nitpick comments:
In `@contracts/staking/src/config.rs`:
- Around line 106-123: The configure_dependencies function updates
liveness_slashing and blend_reserve without emitting change events. After
successfully setting each dependency, emit the corresponding *Changed event
using the module’s existing event pattern and ensure events are only emitted for
mutations that complete successfully.
In `@contracts/staking/src/handlers.rs`:
- Around line 165-180: Update the validation in configure near the
dpos_activation_block alignment check to reject activation blocks earlier than
the current block, using the existing ERR_ACTIVATION_BLOCK_IN_PAST error and the
established current-block value. Preserve the interval-alignment validation for
non-past activation blocks.
- Around line 28-35: Remove the intermediate InitializeCommand construction
after decode_args in the handler. Destructure or pass the decoded tuple bindings
directly into the subsequent logic, eliminating the redundant field-by-field
re-wrap while preserving the existing argument order and behavior.
In `@contracts/staking/src/math.rs`:
- Around line 50-52: Run cargo fmt on contracts/staking/src/math.rs, ensuring
the assert_eq! involving expand_balance(U112::MAX) is formatted according to
rustfmt’s line-width rules. Verify the touched Rust code passes cargo fmt
--check without unrelated changes.
In `@contracts/staking/src/rewards.rs`:
- Around line 19-55: Consolidate the duplicated external-call and
result-decoding logic into shared generic helpers in util.rs alongside
encode_external_call, preserving revert-data forwarding and decode-error mapping
while supporting the differing Encoder and FunctionArgs bounds. In
contracts/staking/src/rewards.rs#L19-55, move external_call and call_decode to
util.rs and import them; in contracts/staking/src/consensus.rs#L23-40, remove
the local external_call and use the shared helper; in
contracts/staking/src/equivocation.rs#L177-209, remove both local helpers and
use the shared implementations.
- Around line 108-177: Extract the duplicated delegate/undelegate queue
traversal and reward calculation from delegator_claimable and
consume_delegator_claim into one shared walker. Make the walker own the common
payout and balance logic while accepting the caller-specific cursor commit
behavior, so the read-only and mutating paths remain identical except for
persistence. Update both functions to use this shared implementation.
In `@contracts/staking/src/tests.rs`:
- Around line 2755-2807: Reduce duplication in the equivocation commitment tests
by defining a small local constructor/helper for the shared EquivocationCommand
fields, then use it to create redirected, wrong_salt, and wrong_evidence with
only their differing values specified. Preserve each case’s existing
beneficiary, salt, and evidence inputs.
In `@contracts/staking/src/util.rs`:
- Around line 526-604: Extract the shared staking-token call logic from
safe_transfer_from and safe_transfer into a token_call helper that accepts the
SDK and encoded input, handling token lookup, sdk.call status propagation, and
boolean-return validation. Update both callers to build their existing ERC-20
inputs and delegate to token_call, preserving safe_transfer’s zero-amount early
return.
In `@crates/testing/src/host.rs`:
- Around line 347-363: Update the call-handler retrieval in call to distinguish
an initially unconfigured handler from a handler temporarily absent during a
nested call. Preserve the existing configuration panic for the first case, but
use a distinct, self-explanatory panic message when call_handler is already
taken by an active handler invocation, and continue restoring the handler after
the invocation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 70be606f-b121-422a-a7d3-ef03ac40b249
⛔ Files ignored due to path filters (6)
contracts/Cargo.lockis excluded by!**/*.lockcrates/sdk-derive/derive-core/src/snapshots/fluentbase_sdk_derive_core__event__tests__all_indexed.snapis excluded by!**/*.snapcrates/sdk-derive/derive-core/src/snapshots/fluentbase_sdk_derive_core__event__tests__anonymous.snapis excluded by!**/*.snapcrates/sdk-derive/derive-core/src/snapshots/fluentbase_sdk_derive_core__event__tests__basic_transfer.snapis excluded by!**/*.snapcrates/sdk-derive/derive-core/src/snapshots/fluentbase_sdk_derive_core__event__tests__dynamic_indexed.snapis excluded by!**/*.snapcrates/sdk-derive/derive-core/src/snapshots/fluentbase_sdk_derive_core__event__tests__no_indexed.snapis excluded by!**/*.snap
📒 Files selected for processing (27)
contracts/README.mdcontracts/staking/Cargo.tomlcontracts/staking/README.mdcontracts/staking/src/config.rscontracts/staking/src/consensus.rscontracts/staking/src/consts.rscontracts/staking/src/economics.rscontracts/staking/src/equivocation.rscontracts/staking/src/events.rscontracts/staking/src/handlers.rscontracts/staking/src/lib.rscontracts/staking/src/liveness.rscontracts/staking/src/math.rscontracts/staking/src/rewards.rscontracts/staking/src/storage.rscontracts/staking/src/tests.rscontracts/staking/src/types.rscontracts/staking/src/util.rscrates/codec/src/bytes_codec.rscrates/codec/src/lib.rscrates/genesis/build.rscrates/sdk-derive/derive-core/src/event.rscrates/sdk/src/storage/primitive.rscrates/testing/src/host.rscrates/types/src/genesis.rse2e/src/lib.rse2e/src/staking.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- contracts/README.md
- crates/codec/src/lib.rs
- crates/types/src/genesis.rs
fc97ab7 to
50ea34d
Compare
Require verified consensus keys in every validator creation path, store compressed BLS keys as fixed words, and filter keyless validators before top-k committee selection.
Prevent validator and delegator reward cursors from consuming unsettled epochs while keeping matured undelegated principal independently claimable.
2 participants
Summary
0x0000000000000000000000000000000000520011as a normal rWasm contract usingSharedAPI0x0000000000000000000000000000000000520012initializehandlerArchitecture
Staking is a genesis-deployed rWasm smart contract rather than a system precompile because delegation, reward claims, slashing payouts, and genesis bootstrap require BLEND ERC-20 calls.
getChainConfig()resolves to the staking contract itself.Initialization and governance
The one-shot
initializehandler configures the complete contract in one transaction:Initialization is permissionless because genesis deployment installs and initializes the contract atomically before public transactions can execute. The initializer flag is stored separately and prevents later reinitialization. Runtime configuration and validator administration remain restricted to the reserved genesis-governance address.
Storage and source layout
State is split across four ERC-7201 roots:
Fluent.storage.InitializerFluent.storage.ChainConfigFluent.storage.ConsensusFluent.storage.StakingStoragelib.rscontains grouped selector dispatch for Initializer, ChainConfig, Staking, and Consensus methods. Domain implementation is kept in flat files:initializer.rs: atomic one-shot initializationconfig.rs: chain configuration, dependencies, getters, and governance settersstaking.rs: validator lifecycle, selection, delegation, snapshots, and rewardsconsensus.rs: consensus keys, committees, liveness, jail handling, and equivocationstorage.rs: namespaced storage layoutsutil.rs: shared ABI, validation, revert, epoch, and ERC-20 helpersAll 90 public handlers document their selector hex, ABI method name, and behavior.
Implemented parity
trueor no dataValidator identity is permanent. Governance may disable and reactivate validators, but disabling does not delete validator records, consensus keys, ownership mappings, or stake history.
changeValidatorOwnerremains in the compatibility ABI but always reverts because the owner is the immutable administrative, reward, self-stake, and slashing identity.Supporting changes
bytesdecoding bounds and dynamic event-data encodingIntentional differences from Solidity
The rWasm contract omits UUPS/Ownable proxy-management scaffolding and the unused legacy
getSystemReward()/getStakingPool()dependency getters. These are not staking behavior in the non-proxy genesis deployment.Validation
cargo test --manifest-path contracts/Cargo.toml -p fluentbase-contracts-staking --lib: 48 passedcargo test -p fluentbase-e2e staking:: -- --nocapture: 2 passed-D warnings: passed--no-default-features --features stdcheck: passedLinear: FLU-989
Summary by CodeRabbit
bytesdecoding bounds checks and made event ABI encoding failures return an error instead of aborting.