Feat/lock-staking-migration #56
Conversation
WalkthroughAdds Changes
Sequence Diagram(s)sequenceDiagram
participant User as User/Account
participant LI as vip::lock_incentive
participant Incentive as incentive module
participant SA as StakingAccount (on-chain)
rect rgb(240,248,255)
Note over LI: stake_internal flow
User->>LI: stake_internal(metadata, amount, release_time)
LI->>LI: validate metadata whitelisted & amount>0
LI->>SA: create/acquire StakingAccount for signer
LI->>SA: increment entries[StakingKey(metadata, release_time)]
LI->>Incentive: incentive::stake(...)
LI->>LI: emit StakeEvent
LI-->>User: success
end
rect rgb(255,240,245)
Note over LI: unstake_internal flow
User->>LI: unstake_internal(metadata, amount?, release_time)
LI->>LI: ensure current_time >= release_time
LI->>SA: verify entry exists & sufficient balance
LI->>SA: decrement/remove entries[StakingKey]
LI->>Incentive: incentive::unstake(...)
LI->>LI: emit UnstakeEvent
LI-->>User: return unstaked amount
end
sequenceDiagram
participant Account as Account/User
participant LS as vip::lock_staking
participant MS as MigrationStore
participant MI as MigrationInfo
rect rgb(240,248,255)
Note over LS: request_migration
Account->>LS: request_migration(new_validator)
LS->>MS: lookup/create MigrationStore for metadata
LS->>MI: create/register MigrationInfo (unbond_time, entries, validator)
LS->>MS: register info in MigrationStore.infos
LS->>LS: emit RequestMigrationEvent
LS-->>Account: acknowledged
end
rect rgb(245,255,250)
Note over LS: finalize_migration
Account->>LS: finalize_migration(owner)
LS->>MI: validate & process entries, set finalized=true
LS->>LS: emit FinalizeMigrationEvent
LS-->>Account: finalized
end
rect rgb(255,250,240)
Note over LS: protected operations check
Account->>LS: protected op (stake/withdraw/undelegate...)
LS->>MI: is_in_migration(account)?
MI-->>LS: migration flag/result
LS->>LS: route/restrict operation based on migration state
LS-->>Account: execute or reject
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (9)
sources/vesting.move (1)
604-607: Avoid hard abort; guard-skip non-previous vestings insteadAsserting every element is strictly before claim_info.start_stage makes the loop brittle if vestings_vec ever contains same/later starts. Prefer skipping those entries.
Apply this minimal guard and wrap the logic below:
- assert!( - value.start_stage < claim_info.start_stage, - error::invalid_argument(EINVALID_STAGE) - ); + if (!(value.start_stage < claim_info.start_stage)) { + return; + };sources/vip.move (1)
1524-1526: Block stage 0 in claims—goodPrevents invalid genesis claims. Optional: also enforce start_stage ≥ bridge.init_stage for first claim to tighten invariants.
sources/mock/mstaking.move (1)
1140-1197: Pagination-normalization in (un)set_delegator_delegations: clarify reference constructionThe new logic that normalizes
DelegatorDelegationsRequestV2whenpaginationisnoneis helpful to keep query keys consistent with the Cosmos SDK behavior.However, you currently build a reference to a struct literal:
let req = if (option::is_none(&req.pagination)) { let new_req = &DelegatorDelegationsRequestV2 { ... }; new_req } else { req };Same pattern appears in both
set_delegator_delegationsandunset_delegator_delegations.In Move, it’s more idiomatic (and clearer for the borrow checker) to first bind the struct to a local and then borrow that local, instead of borrowing a literal directly. This also avoids any ambiguity about the lifetime of the borrowed value.
Consider refactoring along these lines:
-fun set_delegator_delegations( - req: &DelegatorDelegationsRequestV2, res: &DelegatorDelegationsResponse -) { - let req = - if (option::is_none(&req.pagination)) { - let new_req = - &DelegatorDelegationsRequestV2 { - delegator_addr: req.delegator_addr, - pagination: option::some( - PageRequest { ... } - ), - status: req.status - }; - new_req - } else { req }; +fun set_delegator_delegations( + req: &DelegatorDelegationsRequestV2, res: &DelegatorDelegationsResponse +) { + let normalized_req_val; + let normalized_req = + if (option::is_none(&req.pagination)) { + normalized_req_val = + DelegatorDelegationsRequestV2 { + delegator_addr: req.delegator_addr, + pagination: option::some( + PageRequest { key: option::none(), offset: option::none(), + limit: option::none(), count_total: option::none(), + reverse: option::none() } + ), + status: req.status + }; + &normalized_req_val + } else { req }; - set_query_response( - b"/initia.mstaking.v1.Query/DelegatorDelegations", - marshal(req), - marshal(res) - ); + set_query_response( + b"/initia.mstaking.v1.Query/DelegatorDelegations", + marshal(normalized_req), + marshal(res) + );You can mirror the same pattern in
unset_delegator_delegations. This keeps the semantics while making the borrowing rules explicit and less error-prone.sources/lock_incentive.move (1)
321-347: Overall stake/unstake design looks solid once ownership semantics are fixedConceptually, the design is clean:
StakingAccounttracks per-(metadata, release_time) stakes viaTable<StakingKey, u64>.stake_internallazily creates the account, accumulates stake per key, and delegates toincentive::stake, emittingStakeEvent.unstake_internalenforces release-time, validates account and entry existence, supports partial or full unstake, cleans up empty entries, delegates toincentive::unstake, and emitsUnstakeEvent.- Tests cover happy paths and common failure modes (early unstake, missing account/entry, zero or excessive amount).
Once the
metadatamove/copy issues and thestaked_amountzero-check are corrected, this module should be robust for friend modules to build on.sources/lock_staking.move (5)
132-161: MigrationInfo lifetime: one-shot per account & permanent on-chain footprint
MigrationInfois stored under the staking-account address and never removed; onlyfinalizedis toggled and the corresponding entry inMigrationStore.infosis removed. At the same time,request_migrationguards with!exists<MigrationInfo>(staking_account_addr)(Line 228), so once a migration has been finalized, the account can never request a new migration, and theMigrationInforesource for that account remains on-chain indefinitely.If that “one-time migration per account + permanent record” behavior is intentional, it may be worth documenting it explicitly. If you expect multiple migrations over time or want to avoid unbounded storage growth, consider either:
- Dropping the
MigrationInforesource on finalization (e.g.,move_from<MigrationInfo>inmigrate_to_incentive/migration_hook), or- Keeping a compact history elsewhere (e.g., via events only) and reusing/remapping
MigrationInfofor subsequent migrations.
272-316: Finalize / migrate_to_init / migrate_to_incentive / migration_hook: behavior & rounding considerationsThe finalize side is generally well‑structured:
finalize_migrationvalidates:
- Both
StakingAccountandMigrationInfoexist forowner.- Migration not already finalized.
curr_time > migration_info.unbond_time(strict), which aligns with only treating entries as mature when strictly past the unbond time.- Uses
lp_amount = min(balance, expected_total_lp)to avoid over‑consuming LP.
migrate_to_init:
- Withdraws LP from the staking account, unwraps it via DEX, and splits into init vs counterparty asset, depositing init back to the staking account and the counterparty to
owner.- Assumes exactly one side of the pool has metadata equal to
get_init_metadata().
migration_hook:
- Recomputes the new
locked_shareon the init validator, redistributes it acrossmigration_info.entriesusing their stored weights, and then marks the migration as finalized and removes the entry fromMigrationStore.infos.- Emits
FinalizeMigrationEvent.
migrate_to_incentive:
- Stakes
migration_amountintolock_incentiveby distributingamount_i = mul_by_u64_truncate(entry.weight, migration_amount)per entry, then setsfinalized = true, removes theinfosentry, and emitsFinalizeMigrationEvent.Two things to consider tightening:
LP‑pair assumption in
migrate_to_initThe code assumes that
dex::withdraw_liquidityreturns a pair where exactly one ofcoin_a,coin_bhas metadata equal toget_init_metadata(). Ifmigration_store.metadatawere misconfigured to an LP that doesn’t containuinit, this would silently treat the wrong asset as “init” and still proceed.You could defensively assert this invariant:
let init_metadata = get_init_metadata(); let (coin_a_meta, coin_b_meta) = (fungible_asset::asset_metadata(&coin_a), fungible_asset::asset_metadata(&coin_b)); assert!( (coin_a_meta == init_metadata) || (coin_b_meta == init_metadata), error::invalid_argument(ENOT_INIT_METADATA) );before selecting
(init_fa, counterparty_fa).Remainder handling in
migrate_to_incentiveUsing
mul_by_u64_truncate(entry.weight, migration_amount)guarantees you don’t over‑stake, but it can leave a small LP remainder on the staking account (bounded by number of entries). That’s probably fine, but if you intend to consume all LP into incentives, you might want to:
- Track the cumulative sum and assign any leftover to the last entry, or
- Explicitly document that tiny remainders remain withdrawable as LP.
Overall, the lifecycle (request → unbond → finalize) and state cleanup (removing from
MigrationStore.infos, settingfinalized) are coherent; the above are mostly robustness and clarity improvements.Also applies to: 1037-1147, 1385-1455
441-480: Migration gating on asset withdrawal and hooks is sound but subtleThe new guard:
let owner = object::owner(object); assert!(!is_in_migration(owner), error::invalid_state(EMIGRATION_IN_PROGRESS));inside
withdraw_asset_for_staking_account(Line 455) combined with wiring all hook paths (delegate_hook,redelegate_hook_v2,undelegate_hook) through:withdraw_asset_for_staking_account( staking_account_signer, get_init_metadata(), option::none() );achieves two important properties:
- While a migration is active (
is_in_migration(owner) == true), no one can withdraw assets (including auto‑withdrawal ofuinitin hooks), preventing users from draining funds while migration is in progress.- Once the migration finalizes (
finalized = true),is_in_migrationbecomes false even thoughMigrationInfostill exists, and hooks/withdrawals resume working.Given this coupling, it might be worth:
- Adding a brief comment on
withdraw_asset_for_staking_accountexplaining that it intentionally blocks withdrawals during migration (including hook‑driven flushes).- Double‑checking that no migration path itself relies on
withdraw_asset_for_staking_account(currently it does not, which is good).Behaviorally, this looks correct and aligns with the migration safety goals.
Also applies to: 1245-1383, 1863-1867, 2184-2186
1178-1202: delegate_internal gating on MigrationStore existenceThe added check:
let metadata = fungible_asset::metadata_from_asset(&fa); assert!( !exists<MigrationStore>(@vip) || metadata == get_init_metadata(), error::invalid_argument(ENOT_INIT_METADATA) );means:
- Before any migration is configured (
MigrationStoreabsent),delegate_internalaccepts delegations of arbitrary LP tokens (current behavior).- After
MigrationStoreis initialized, onlyget_init_metadata()(uinit) delegations are allowed via the normal delegate/provide_delegate paths; LP delegations will abort withENOT_INIT_METADATA.This is a strong behavior change: new LP‑based lock staking is effectively frozen once migration is turned on, and only plain‑uinit delegations remain possible.
If that’s the intended policy (no new LP delegations once we start migrating away from LP), then this is a nice, explicit guard. If you instead intend to allow LP delegations that will later be migrated as well, you may want to narrow the condition, e.g.:
- Restrict only the specific LP metadata configured in
MigrationStore, or- Gate only particular entry functions rather than all
delegate_internalcallers.
4510-4924: Migration and incentive test coverage is strong; consider one or two additional edge casesThe new tests (
test_migration_incentive,test_migration_init, thefail_test_*cases, andtest_incentive) do a good job of:
- Exercising both migration targets (incentive vs init validator) end‑to‑end.
- Verifying timestamps (
unbond_time), validator selection, andis_in_migration/get_migration_addrsbehavior.- Checking failure conditions for:
- Double request (
EMIGRATION_ALREADY_REQUESTED),- No locked delegations (
ENOTHING_TO_MIGRATE),- Missing/already finalized/in‑progress finalization.
- Validating incentive staking/unstaking independently of migration.
If you want to further harden behavior, a couple of edge‑case tests that could be useful (optional):
- Migration where slashing occurs between
request_migrationandfinalize_migration, to validate thelp_amount = min(balance, expected_total_lp)path and distribution logic.- A small multi‑entry migration to assert that the sum of amounts staked into incentive plus any leftover LP on the staking account matches the matured LP balance (rounding sanity check).
Overall, though, this test suite already gives good confidence in the new flows.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (8)
.gitignore(1 hunks)Move.toml(1 hunks)sources/lock_incentive.move(1 hunks)sources/lock_staking.move(38 hunks)sources/mock/mstaking.move(132 hunks)sources/test.move(1 hunks)sources/vesting.move(1 hunks)sources/vip.move(4 hunks)
🔇 Additional comments (15)
.gitignore (1)
9-10: LGTMAdding .coverage_map.mvcov is harmless and useful; build ignore remains intact.
sources/test.move (1)
371-373: Liquidity seed increase looks fineBumping initial amounts reduces slippage in tests. Ensure minted supplies remain >> 1e8 (they are earlier), so no underfund.
If desired, double-check balances before pair creation by asserting coin::balance(chain_addr, init_metadata) and usdc_metadata each ≥ 100_000_000.
sources/vip.move (3)
79-80: New EZERO_STAGE constant—OKClearer erroring for stage 0; no conflicts observed with existing codes.
956-975: Auto-register + strict adjacency check—good guardrail
- Registers user vesting store on first claim.
- Enforces start_stage == last_claimed_stage + 1 when applicable.
This closes gaps in jump-claim attempts and aligns with tests.
Consider adding a unit test for the very first claim on a re-registered bridge version (last_claimed_stage == 0) to document expected behavior.
3488-3488: Abort code mapping verified—correctThe abort code 0x10010 at line 3488 correctly maps to EINVALID_CLAIMABLE_STAGE. The encoding follows Move's standard: module prefix 0x1 (0x10000) + error constant 16 (0x0010) = 0x10010. This is confirmed in vip.move line 52:
const EINVALID_CLAIMABLE_STAGE: u64 = 16;. The mapping is accurate and semantically aligned with the functionfailed_claim_already_claimed.Move.toml (2)
11-12: Dev address change verified—safe to proceedChecked sources/vip.move, sources/test.move, sources/vault.move, and sources/weight_vote.move. All hard-coded @0x2/@0x3 references are explicit test fixture parameters (agent, operator, user, funder), not implicit dependencies on dev-address values. Tests correctly use
@vipwhere the vip module is needed. The bump from vip=0x2→0x3 and vesting=0x3→0x4 does not break these tests.
15-16: Dependency verification confirmed—modules available in specified pathThe initia-labs/movevm repository includes all specified modules under precompile/modules/initia_stdlib, confirming the dependency paths and versions are valid and compatible.
sources/mock/mstaking.move (3)
1682-1880: New delegator delegation tests look comprehensiveThe added
test_delegatescenario (and related assertions forget_delegationandget_delegator_delegations) exercise:
- multiple delegators,
- correct share computation via
staking::amount_to_share,- end-to-end consistency between delegation and delegator-delegations views.
Given the complexity of the mock staking state, these tests significantly improve confidence in the revamped pagination and query wiring.
2291-2522: End-to-end teste2e_testnicely covers delegation, redelegation, unbonding, and reward flowsThe
e2e_testfunction validates a rich scenario:
- symmetric delegations across two validators,
- redelegation and undelegation updates to delegator views,
- clearing of completed unbonding and redelegation entries via
clear_completed_entries,- final balance checks on LP tokens.
This is a good high-level regression test for the behavior that
vip::lock_incentiveandvip::lock_stakingwill rely on.
1542-1557: The review comment is incorrect;Stringhas thecopyability in MoveMove's standard
Stringtype (std::string::String) is declared withcopy, drop, storeabilities, which means values with the copy ability are copied (not moved) when passed as function parameters. Therefore,symbolininit_and_mint_coinis automatically copied when passed to bothcoin::initialize()andcoin::metadata(), and the code requires no modification. The suggested explicitcopy symbolis unnecessary.Likely an incorrect or invalid review comment.
sources/lock_incentive.move (3)
59-91: Remove this review comment; it is based on a misunderstanding of Move'scopytype semantics.In Move, a struct can only have the
copyability if ALL its fields havecopy. SinceStakingKeyis declared withhas copy, dropand successfully compiles with fieldmetadata: Object<Metadata>, the typeObject<Metadata>must already havecopyanddropabilities.When a type has
copy, Move automatically copies the value (not moves it) at each use site. The code instake_internalis correct as written:let key = StakingKey { metadata, release_time: table_key::encode_u64(release_time) }; // ... incentive::stake(staking_account_signer, metadata, amount); event::emit(StakeEvent { /* ... */, metadata, /* ... */ });Each use of
metadatahere is valid and idiomatic—no explicitcopykeyword is needed or required. The proposed diff adds unnecessary verbosity and does not address an actual issue.Likely an incorrect or invalid review comment.
196-231: I'll search for Initia's stdlib implementation to verify the error code encodings.Let me search more specifically for Initia's error module:
Based on my web search, I can confirm that upstream Move/Aptos use those category values, but I cannot definitively verify that Initia's
std::errormodule uses the identical high-nibble encoding (0x1, 0x3, 0x6) without direct access to Initia's stdlib implementation.The original review comment correctly identifies that the error codes are well-aligned with the expected categories for the test cases shown. However, verifying that this alignment will remain stable across Initia stdlib upgrades requires confirming Initia's actual error encoding implementation, which is not publicly available through standard web searches.
Verify Initia error module encoding independently
The test expectations (0x10001 for
EZERO_AMOUNT, 0x30002 forENOT_RELEASED, 0x60004 for missing staking entry, 0x10005 for insufficient balance) assume Initia'sstd::erroruses the same high-nibble encoding as upstream Move. Before finalizing this test suite, confirm directly in Initia's stdlib that these error category values (0x1 for invalid_argument, 0x3 for invalid_state, 0x6 for not_found) are indeed used, or update theexpected_failurecodes if Initia's encoding differs.
160-231: The review comment is incorrect and should be ignored.The review's core premise—that
Object<Metadata>is not copyable and requires explicitcopyon reuse—is contradicted by the code itself. The structStakingKeyis declared withhas copy, dropand contains a fieldmetadata: Object<Metadata>. By Move's strict compiler rules, a struct can only have thecopyability if all its fields are copyable. Since this code exists in the codebase,Object<Metadata>must be copyable.In Move, copyable types support implicit copying on use. The
copykeyword is not required for reusable copyable values. The code intest_stake,test_unstake, and other test functions is valid as written and does not need modification.Likely an incorrect or invalid review comment.
sources/lock_staking.move (2)
219-270: Migration request flow (request_migration / undelegate_all / prepare_migration) looks consistentThe request-side migration logic hangs together well:
request_migration:
- Ensures there’s no pending migration (
!exists<MigrationInfo>(staking_account_addr)) before proceeding.- Uses
MigrationStore.metadataso the migratable LP type is globally configured.- Derives
unbond_time = curr_time + unbonding_periodand records the address inMigrationStore.infoskeyed by(unbond_time, addr).- Emits
RequestMigrationEventwith all relevant context.
undelegate_all:
- Safely skips validators with zero
total_shareortotal_locked_shareto avoid division by zero.- Builds
validator_ratio_mapastotal_share / total_locked_shareandexpected_total_lpas the sum of per‑validator LP amounts.- Sends
MsgUndelegatestargate messages for each validator.
prepare_migration:
- Uses
undelegate_allthen assertsexpected_total_lp > 0(and thatmigration_entriesis non‑empty) to prevent “empty” migrations.- Collects all delegation keys for the migrating LP metadata and withdraws each delegation once, computing per‑entry
weight = expected_lp / expected_total_lp.- Ensures all LP‑backed delegations for the given metadata are removed from on‑chain state before storing
MigrationInfo.The overall accounting and state‑clearing story looks solid, and the negative tests (
fail_test_request_migration_nothing, etc.) exercise the key guards.Also applies to: 900-1035
1863-1867: is_in_migration and get_migration_addrs semantics look correct for finalization schedulingTwo new views:
is_in_migration(addr)correctly checks:
- Existence of
MigrationInfoat the derived staking address, and!finalized, so finalized migrations no longer report as “in migration” even though the resource is kept.
get_migration_addrs(limit):
- Returns up to
limitaddresses whoseMigrationInfoKey.unbond_timeis strictly less than the current block time, because the iterator upper bound is(unbond_time = encode_u64(curr_time), addr = @0x0)andfinalize_migrationrequirescurr_time > migration_info.unbond_time.- Uses the
MigrationStore.infostable as the single source of truth for pending, not-yet-finalized migrations.This aligns nicely with the finalize guard and the tests (
test_migration_incentive,test_migration_init) that assertget_migration_addrs(1) == vector[addr]only after advancing the block time beyondunbond_time.Also applies to: 2307-2336
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
sources/lock_staking.move (2)
1118-1130: Consider dust handling from truncation.Line 1122 uses
bigdecimal::mul_by_u64_truncate(entry.weight, migration_amount)which may lose fractional amounts. If multiple entries exist, the sum of distributed amounts could be less thanmigration_amount, leaving dust tokens in the staking account.While this may be acceptable for small amounts, consider whether:
- The remaining balance should be distributed to the last entry, or
- Users should be explicitly informed about potential dust, or
- This is documented as expected behavior
298-302: Consider documenting or signaling when migration receives less than expected.Lines 299-302 cap
lp_amountat the minimum of actual balance andexpected_total_lp. If slashing occurs during the unbonding period, users may receive less thanexpected_total_lp.While this is correct behavior, consider:
- Emitting an event parameter showing the difference, or
- Adding a comment explaining this expected scenario, or
- Documenting this possibility in user-facing materials
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (1)
sources/lock_staking.move(38 hunks)
🔇 Additional comments (9)
sources/lock_staking.move (9)
205-208: LGTM! Critical issue from previous review has been addressed.The explicit assertion
signer::address_of(chain) == @vipnow ensures theMigrationStoreresource is only created at@vip, matching all subsequent access patterns throughout the module. This prevents the abort scenario whenchain != @vip.
1388-1458: LGTM! Migration hook correctly handles share distribution.The function properly validates migration state, calculates share deltas, and distributes shares proportionally using
bigdecimal::mul(line 1427) which maintains precision for fractional shares. The state cleanup (lines 1439-1446) and event emission (lines 1448-1455) are also correct.
458-458: LGTM! Correctly blocks asset withdrawal during migration.The assertion prevents users from withdrawing assets while migration is in progress, ensuring migration logic can safely assume assets remain in the staking account until finalized.
1866-1870: LGTM! Migration state check is correct.The function properly returns
trueonly whenMigrationInfoexists and is not yet finalized, accurately reflecting active migration state.
2310-2339: LGTM! Efficiently queries finalizable migrations.The function correctly uses table iteration with an upper bound of
curr_timeto return addresses whose unbonding period has completed, ready for migration finalization. The pagination withlimitparameter is also well-implemented.
2187-2189: LGTM! Centralizes init metadata reference.The helper function provides a single source of truth for the init token metadata, used consistently throughout migration flows. This improves maintainability.
321-384: LGTM! Incentive staking functions are correctly implemented.The three functions properly wrap the lock_incentive module's internal functions with appropriate token transfers and validation. The release time checks (lines 329-336) correctly enforce lock period constraints.
2356-2356: LGTM! Test function signatures correctly updated.The addition of
MigrationInfoto theacquiresclauses of test functions is a necessary mechanical change since the functions they call now acquire this resource. The changes are consistent throughout.Also applies to: 2414-2414, 2512-2512, 2658-2658, 2909-2909, 3295-3295, 3530-3530, 3659-3659, 3817-3817, 3959-3959, 4144-4144, 4332-4332, 4466-4466
4506-4927: LGTM! Comprehensive test coverage for migration flows.The test suite thoroughly covers:
- Both migration paths (to incentive and to init validator)
- Error scenarios (duplicate requests, empty migrations, premature finalization)
- Edge cases and state transitions
The tests properly set up prerequisites and validate state at each step.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
sources/lock_staking.move (5)
222-273: Consider adding a migration cancellation mechanism.Once
request_migrationis called,prepare_migrationimmediately undelegates all positions for the given metadata (line 236-241). The user's funds enter unbonding and cannot be accessed untilfinalize_migrationis called after the unbonding period.If the user changes their mind, or if finalization cannot be completed for any reason (e.g., validator issues, contract bugs), the funds remain stuck in an intermediate state with no way to revert to the original delegated state.
Consider adding:
- A
cancel_migrationfunction that allows reverting before unbond_time- Or at minimum, document that migration is irreversible once requested
298-302: Document partial migration behavior when slashing occurs.Lines 300-301 cap the migration amount at the minimum of actual balance and expected total LP. This handles slashing during unbonding, but users may not realize their migration completed with reduced funds.
Consider adding:
- An event field or log indicating if migration was partial (actual < expected)
- Documentation warning users that slashing during migration reduces final amounts
- Or validation that the difference is within acceptable bounds (e.g., < 10%)
1121-1133: Proportional distribution may lose value to truncation.Line 1125 uses
bigdecimal::mul_by_u64_truncate(entry.weight, migration_amount)to calculate each entry's proportional amount. The truncation can cause dust loss, especially with many small-weight entries.For example, if
migration_amount = 1000and there are 3 entries with weights[0.333, 0.333, 0.334], the amounts will be[333, 333, 334]totaling 1000. But with weights[0.2, 0.2, 0.2, 0.2, 0.2], amounts will be[200, 200, 200, 200, 200]also totaling 1000. However, with more entries or more complex weight distributions, truncation can cause the sum to be less than the migration amount, with the difference lost.Consider:
- Distributing the full amount for the last entry (amount = migration_amount - sum_so_far)
- Or documenting that small amounts may be lost to rounding
1061-1063: No slippage protection on liquidity withdrawal.Line 1061 calls
dex::withdraw_liquiditywithoption::none()for bothmin_coin_aandmin_coin_b, accepting any output amounts without slippage protection.While removing liquidity typically shouldn't have slippage (it's proportional to pool reserves), in edge cases like:
- Pool manipulation right before migration
- Pool in extreme imbalanced state
- DEX contract bugs
...the user could receive far less than expected on one or both sides.
Consider:
- Calculating expected amounts based on LP proportion and adding reasonable slippage tolerance (e.g., 1-5%)
- Or documenting that migrations accept any liquidity withdrawal outcome
- Or validating the received amounts are reasonable relative to expected_total_lp
1202-1205: Consider adding explanatory comment for metadata restriction.As confirmed in past reviews, this restriction is intentional: once the migration infrastructure (
MigrationStore) is deployed, users can only delegate init metadata, not LP tokens. This protocol-level decision encourages migration to the new system.Consider adding a comment to prevent future confusion:
// Once migration infrastructure is deployed, only init metadata delegation is allowed // to encourage all users to migrate from LP staking to init staking assert!( !exists<MigrationStore>(@vip) || metadata == get_init_metadata(), error::invalid_argument(ENOT_INIT_METADATA) );Based on learnings
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (1)
sources/lock_staking.move(38 hunks)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
sources/lock_staking.move (2)
203-230: Consider adding bounds validation forunbonding_period.The
unbonding_periodparameter is not validated, allowing it to be set to extreme values (e.g., 0 for immediate finalization, or u64::MAX for effectively infinite unbonding). While this may be intentional for flexibility, consider adding reasonable bounds:assert!( unbonding_period > 0 && unbonding_period <= MAX_UNBONDING_PERIOD, error::invalid_argument(EINVALID_UNBONDING_PERIOD) );This would prevent misconfiguration and make the system's invariants clearer.
1131-1143: Optional: Consider handling rounding dust inmigrate_to_incentive.Using
mul_by_u64_truncateon line 1135 truncates fractional parts, which can cause the sum of all staked amounts to be slightly less thanmigration_amount, leaving dust in the staking account.For example, with 100 tokens and 3 equal-weight entries (0.333... each), truncation gives [33, 33, 33] = 99, leaving 1 token.
To eliminate this:
let mut remaining = migration_amount; let entries_len = vector::length(&migration_info.entries); vector::enumerate_ref(&migration_info.entries, |i, entry| { let amount = if (i == entries_len - 1) { remaining // Use all remaining for last entry } else { let amt = bigdecimal::mul_by_u64_truncate(entry.weight, migration_amount); remaining = remaining - amt; amt }; lock_incentive::stake_internal(&staking_account_signer, migration_store.metadata, amount, entry.release_time); });Note: The current dust impact is minimal (< number of entries) and this refactor is purely for precision.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (1)
sources/lock_staking.move(38 hunks)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
sources/lock_staking.move (1)
916-978: Consider documenting the validator_ratio_map calculation.The
undelegate_allfunction calculates ratios astotal_share / total_locked_sharefor each validator. This ratio is later used inprepare_migrationto weight migration entries. While the math is correct, adding a comment explaining the purpose of this ratio (to account for share changes between delegation and migration due to rewards/slashing) would improve maintainability.// save ratio to simple map + // Ratio accounts for share changes (rewards/slashing) between original delegation and migration time let ratio = bigdecimal::div(total_share, total_locked_share); simple_map::add(&mut validator_ratio_map, validator, ratio);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (2)
sources/lock_incentive.move(1 hunks)sources/lock_staking.move(38 hunks)
🔇 Additional comments (7)
sources/lock_staking.move (7)
1053-1118: LGTM! Migration to init path is well-structured.The
migrate_to_initfunction correctly:
- Withdraws LP and converts to init + counterparty tokens
- Deposits init to staking account, counterparty to owner
- Delegates init to the specified validator
- Uses migration_hook to properly account for locked shares by release_time
The integration between LP withdrawal, token separation, delegation, and hook execution is clean.
1120-1163: LGTM! Migration to incentive path correctly distributes tokens.The function properly:
- Distributes
migration_amountacross entries according to pre-calculated weights- Maintains original release_time for each stake
- Cleans up migration state (sets finalized, removes from infos table)
- Emits the finalization event
The truncation in
mul_by_u64_truncateis acceptable since remaining dust amounts would be negligible.
331-387: LGTM! Incentive staking functions are well-integrated.The new entry points (
stake_incentive,unstake_incentive,claim_incentive) properly:
- Validate release_time constraints against ModuleStore limits
- Transfer tokens between user and staking account
- Delegate to the lock_incentive module for internal accounting
- Maintain separation of concerns (entry validation here, state management in lock_incentive)
468-471: Good safety check for migration in progress.Blocking withdrawal of migration metadata during active migration prevents users from accidentally withdrawing LP tokens that are needed to complete the migration flow. This ensures migration can finalize correctly once the unbonding period completes.
2328-2357: LGTM! Migration address query is well-designed.The
get_migration_addrsview function:
- Efficiently uses table iteration with an upper bound (curr_time) to find ready migrations
- Respects the caller-specified limit to control result size
- Returns addresses in order of unbond_time (earliest first)
This enables off-chain services to identify and finalize migrations in a gas-efficient manner.
4531-4702: Excellent test coverage for migration flows.The migration tests comprehensively cover:
- Both migration paths (incentive and init)
- State validation at each step (locked delegations, migration info, finalization)
- Integration with unbonding period timing
- Helper function behavior (get_migration_addrs, is_in_migration)
The tests validate the complex multi-step migration flow end-to-end.
4711-4908: LGTM! Failure mode tests are thorough.The negative test cases properly validate:
- Duplicate migration prevention (EMIGRATION_ALREADY_REQUESTED)
- Empty migration prevention (ENOTHING_TO_MIGRATE)
- Missing migration handling (EMIGRATION_NOT_FOUND)
- Double finalization prevention (EMIGRATION_ALREADY_FINALIZED)
- Premature finalization prevention (EMIGRATION_IN_PROGRESS)
These guard against common edge cases and ensure robust error handling.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
sources/vip.move (1)
1812-1835: Document deprecation for external integrators.The
_validator,_stakelisted_metadata, and_stakelisted_amountparameters are now deprecated but retained for backward compatibility. While the inline comments are helpful, external integrators should be notified:
- These parameters are ignored in the new implementation
- The staking model has migrated from validator delegation to incentive-based staking
- Integrators should update their calls to use the new functions directly
Consider adding migration notes in the repository documentation or changelog.
sources/lock_staking.move (2)
307-329: Finalize flow handles both init and incentive targets, but silently tolerates LP shortfall
finalize_migrationclamps the migrated LP amount tomin(balance, expected_total_lp)and then routes to eithermigrate_to_initormigrate_to_incentive. This is safe, but if slashing or other effects causebalance < expected_total_lp, the shortfall is silently accepted and no state or event records it.If you care about operational visibility into “under‑migrated” positions, consider recording when
lp_amount < expected_total_lp(e.g., via an event field, a counter inMigrationInfo, or logs) so off‑chain tooling can detect and act on migration losses.
916-1051: Undelegate‑all + prepare_migration weighting scheme is sound, with minor rounding caveat
undelegate_allandprepare_migrationtogether:
- Compute a per‑validator ratio
total_share / total_locked_shareand skip validators with zero share or zero locked share.- Fully undelegate all staking positions for the migration LP and sum their expected LP via
staking::share_to_amount.- Build
MigrationEntryweights asexpected_lp / expected_total_lp, with a final assertion that at least one entry exists.This keeps proportional release scheduling while normalizing against the total migrated LP. Note that due to integer/decimal truncation (
mul_by_u64_truncateand share/amount conversions) the weights may not sum exactly to 1, leaving a tiny residual LP undistributed. If you ever need exact conservation, consider pushing any remainder into the last entry or tracking a “dust” value separately.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (4)
sources/lock_incentive.move(1 hunks)sources/lock_staking.move(38 hunks)sources/test.move(17 hunks)sources/vip.move(17 hunks)
🔇 Additional comments (32)
sources/vip.move (9)
79-79: LGTM! New error constant for zero-stage validation.The
EZERO_STAGEconstant is properly defined and follows the established error code pattern.
610-616: Consistent refactoring to incentive-based staking.This stableswap variant follows the same refactoring pattern as
lock_stake, moving from validator delegation to incentive staking.
963-979: LGTM! Sequential claim validation prevents stage skipping.The enhanced validation logic ensures:
- Vesting store is auto-registered if needed (convenient for users)
- Sequential claiming is enforced:
start_stagemust equallast_claimed_stage + 1whenlast_claimed_stage != 0- First-time claims (when
last_claimed_stage == 0) are allowed from any stageThis prevents users from skipping stages and claiming out of order, which strengthens the reward distribution integrity.
1529-1530: LGTM! Zero-stage guard prevents invalid claims.The assertion prevents claiming from stage 0, which is appropriate since stage 0 represents the initial state before any reward distribution. This prevents underflow in line 1532 where
start_stage - 1is calculated.
3531-3531: LGTM! Test updated for new validation logic.The expected failure annotation correctly reflects the new
EINVALID_CLAIMABLE_STAGEerror from the enhanced sequential claim validation.
1862-1939: LGTM! Clean refactoring with backward compatibility.The new
batch_lock_incentive_scriptandbatch_stableswap_lock_incentive_scriptfunctions provide clean interfaces without deprecated parameters, while the old functions maintain backward compatibility by delegating to the new implementations.
2391-2483: LGTM! Test infrastructure updated for incentive modules.The test setup properly initializes the new
incentiveandlock_incentivemodules and simplifies the return type to only include necessary values.
4494-4580: LGTM! Comprehensive test coverage for incentive-based staking.The test properly verifies the new lock staking flow:
- LP tokens are deposited to the user's account
- Incentive staking is recorded correctly
- Locked incentive data (metadata, amount, release_time) is accurate
The assertions provide good coverage of the migration from validator delegation to incentive staking.
553-557: No action required — both validator delegation and incentive-based staking coexist.The
stake_incentivefunction (line 331) invokeslock_incentive::stake_internal, which is a separate staking mechanism from validator-baseddelegate_internal. Both patterns are supported concurrently in the module, and existing validator delegations continue to function. This is an addition of new staking functionality, not a breaking replacement of the delegation model.sources/lock_staking.move (10)
203-230: Migration params & MigrationStore placement look consistent now
update_migration_paramsnow hard-restricts the caller to@vipand always stores/updatesMigrationStoreat@vip, which matches all laterborrow_global[_mut]<MigrationStore>(@vip)call sites. The LP‑pair validation againstget_init_metadata()also prevents configuring a non‑INIT pool as the migration target.
232-283: Migration request flow and LP freeze semantics are coherentThe
request_migrationpath correctly:
- Borrows the single canonical
MigrationStoreat@vip.- Derives the staking account via
get_staking_account_signerand enforces a single activeMigrationInfoper staking account.- Uses
prepare_migrationto fully undelegate & remove all delegations for the migration LP metadata, and then recordsMigrationInfoplus an index key inMigrationStore.infos.- Emits a structured
RequestMigrationEventincluding expected LP total and optional validator.This gives a clear, auditable one‑shot migration request per staking account and neatly sets up later finalization.
461-496: New withdrawal guard correctly blocks LP drains during active migrationThe additional assertion in
withdraw_asset_for_staking_account:
- Uses
is_migration_metadatato target only the LP token configured inMigrationStore.- Uses
is_in_migration(owner)to gate by the owner’sMigrationInfofinalization status.This prevents users from withdrawing the LP being migrated while still allowing all other assets (including INIT rewards) to move freely, and once migration finalizes the guard no longer triggers. This is a solid safety net around the migration process.
1053-1163: Init‑path migration hook matches existing delegation accounting patterns
migrate_to_init+migration_hook:
- Reconstruct INIT + counterparty coins from LP, keep INIT on the staking account, send counterparty back to the owner.
- Delegate INIT to the chosen validator and use
share_before/share_afterto deriveshare_diff.- Feed
share_diffthroughshare_to_locked_shareand distribute the resulting locked share acrossmigration_info.entriesby weight.- Mark the migration finalized and clean up
MigrationStore.infosbefore emittingFinalizeMigrationEvent.This mirrors the existing
delegate_hook_internallogic and keeps thetotal_locked_shares/delegationsinvariants intact while reusing the same re‑entry guard pattern. The design looks correct and maintainable.
1120-1163: Incentive‑path migration finalization is straightforward and cleans up indicesFor the “to incentive” path,
migrate_to_incentive:
- Uses the stored weight vector to fan out
migration_amountinto per‑release‑timestake_internalcalls.- Marks
MigrationInfo.finalized = trueand removes the corresponding key fromMigrationStore.infos.- Emits a
FinalizeMigrationEventmirroring the init path.Given that
stake_internalis friend‑only and all writes stay within the staking account’s address, this is a clean and bounded transformation with no obvious leakage or invariants broken.
331-395: Incentive staking integration is minimal and well‑scoped
stake_incentive,unstake_incentive, andclaim_incentive:
- Reuse the existing
StakingAccountobject address viaget_staking_account_signer.- Enforce lock‑period bounds using
lock_staking’sModuleStorewhile delegating per‑key accounting and incentive mechanics tolock_incentive.- Keep all token movements local to the staking account address, avoiding surprises for callers.
Tests in this file and in
lock_incentive.movetogether cover basic success and timing behavior, so this new entry surface looks safe and coherent.
1879-1888: Public viewsis_migration_metadataandis_in_migrationare useful and cheapThese helpers expose exactly the bits off‑chain tooling or other modules need:
is_migration_metadatatells you whether a given LP metadata is the active migration target.is_in_migrationchecks if an address has a non‑finalizedMigrationInfo.Both read only a single resource each and align with how the rest of the module interprets migration state. Good low‑level primitives.
2328-2357:get_migration_addrsindexer is simple and seems to match tests
get_migration_addrs(limit):
- Early‑exits if there is no
MigrationStoreorlimit == 0.- Iterates
migration_store.infoswith an upper bound keyed by(unbond_time = encode_u64(curr_time), addr = @0x0)and stops oncelimitaddresses are collected.This appears to fulfill the “give me up to N matured migrations” use case, and the new tests (
test_migration_incentive/test_migration_init) validate at least the basic correctness. If table iteration ordering semantics ever change, this is one of the first places to re‑validate, but it looks fine for now.
2359-2363:get_locked_incentivesview provides a clean bridge tolock_incentiveBy deriving the staking account address once and delegating to
lock_incentive::get_locked_incentives, this view:
- Keeps
lock_stakingas the single source for the staking‑account address scheme.- Exposes incentive locks alongside delegation locks without duplicating accounting.
This is a good abstraction boundary between the two modules.
4530-4960: New migration & incentive tests give good end‑to‑end coverageThe added tests (
test_migration_incentive,test_migration_init, the variousfail_test_*_migration_*cases, andtest_incentive) exercise:
- Happy‑path flows for both migration targets.
- Error conditions around duplicate requests, nothing to migrate, missing/early/duplicate finalization.
- Basic incentive stake/unstake lifecycle.
This gives strong confidence in the new behavior and should catch most regressions around migration orchestration.
sources/test.move (7)
72-132: Updatedunpack_module_storetuple handling is consistentAll getters (
get_stage,get_stage_interval,get_vesting_period,get_challenge_period, ratios, TVL thresholds, and the newget_minimum_lock_staking_period) now unpack 10 fields fromvip::unpack_module_store(), with the minimum lock staking period occupying the last slot. This keeps existing tests aligned with the extended module store shape and centralizes the new period for reuse.
281-401: Initialization now fully wires incentive + lock_incentive modules
initializehas been extended to:
- Init and configure
incentiveandlock_incentive(epoch start, token pair registrations, LP registrations).- Increase DEX pool initial liquidity to
100000000units per side.- Pre‑fund both bridge addresses and call
vip::fund_reward_scriptonce, as before.This sets up a consistent environment for all tests that rely on incentive locking and ensures
lock_staking.stake_incentiveand related flows have their dependencies in place.
546-567: E2E test now validates incentive lock creation semanticsThe main
e2etest’s call tovip::batch_lock_incentive_scriptis followed by:
- Fetching locked incentives via
lock_staking::get_locked_incentives.- Unpacking and asserting metadata, amount, and
release_time == curr_time + get_minimum_lock_staking_period().This is a good high‑level check that the new script uses the expected LP metadata, locks the correct amount, and respects the configured minimum lock duration.
934-1110: Vesting lock‑stake tests correctly migrate to incentive-based flow
lock_stake_vesting_position,lock_stake_vesting_position_in_challenge_period, and their failure variant now:
- Use
batch_lock_incentive_scriptinstead of legacy lock‑staking scripts.- Pass stage indices as singleton vectors and use vesting remaining reward as lock amounts.
- Keep the original behavioral expectations about vesting position deletion and failure during challenge consistent.
These updates accurately reflect the new “lock to incentive” surface without weakening the original vesting invariants the tests were asserting.
1290-1317: Re-registered bridge reward tests exercise lock-incentive over multiple versionsIn
e2e_re_registered_bridge_reward, the two new calls tobatch_lock_incentive_script:
- Lock vesting rewards for both an old stage and a re‑registered (version+1) stage.
- Reuse
get_lp_metadata()and stage vectors[1]and[5]appropriately.This ensures lock‑staking from vesting positions behaves as expected across bridge deregistration and re‑registration, with incentive locks honoring the correct stage/version pairing.
1912-2025:test_batch_lock_stakethoroughly validates multi-stage incentive locking
test_batch_lock_stakenow:
- Claims rewards for stages 1–4, then for each stage computes remaining vesting and builds a
lock_staking_amountsvector.- Calls
batch_lock_incentive_scriptonce with all stages and amounts.- Asserts that all corresponding vesting positions have been removed.
This is a strong integration test for the “batch lock from many vesting stages” scenario and should catch most accounting regressions in the bridge → vesting → lock‑incentive pipeline.
2637-2760: Stable-swap batch lock test now also checks locked incentive release details
test_batch_stableswap_lock_stakehas been updated to usebatch_stableswap_lock_incentive_scriptand then:
- Asserts all relevant vesting positions are cleared.
- Fetches locked incentives from
lock_staking, unpacks one entry, and checks:
- Metadata is the stable LP.
- Amount equals
stakelisted_amount * 2.- Release time equals
curr_time + get_minimum_lock_staking_period().This end‑to‑end check ties together stableswap LP creation, vesting, and lock‑incentive behavior solidly.
sources/lock_incentive.move (6)
33-51: ModuleStore and keying model are simple and appropriate
ModuleStorekeeps a flat whitelistmetadatas: Table<Object<Metadata>, bool>, andStakingAccount.entriesuses a(metadata, release_time)composite key viaStakingKey. This is a straightforward design that:
- Avoids per‑metadata resources.
- Lets you track multiple release times per LP in a single table.
It should scale reasonably for the expected number of lock entries per user.
81-105: Whitelist management is correctly permissioned and validated
register/deregister:
- Rely on
utils::check_chain_permission(chain)for governance‑level authorization.- Assert against duplicates / missing entries before mutating the whitelist.
This ensures only approved metadata can be locked via
stake_internaland prevents accidental double‑registration or deregistration of unregistered assets.
107-144:stake_internalcorrectly enforces whitelist and aggregates per-key stakeIn
stake_internal:
- The metadata is verified against
ModuleStore.metadatasbefore proceeding.- Zero amounts are rejected early.
- A
StakingAccountresource is lazily created at the caller’s object address.- Stake amounts are accumulated per
(metadata, release_time)key, and then forwarded toincentive::stake.- A
StakeEventis emitted for observability.This keeps local accounting and the incentive module in sync and is restricted to friend modules, which is appropriate.
146-202:unstake_internalhandles timing, balance checks, and cleanup correctlyThe unstake path:
- Enforces that the current time is strictly greater than
release_timebefore allowing any unlock.- Requires the staking account and specific
(metadata, release_time)entry to exist.- Supports both partial and full unstake, with:
- Nonzero amount enforcement.
- “Not enough balance” guarding.
- Decrements the stored amount and removes the key when the total hits zero.
- Delegates to
incentive::unstakeand emits anUnstakeEvent.This gives a clear, predictable lifecycle for locked incentives and ensures no orphaned table entries remain when balances drop to zero.
204-249: View helpers expose exactly what callers need
get_whitelisted_metadatasandget_locked_incentives:
- Provide read‑only access to the current whitelist and all locks per staking account.
- Handle the “no staking account yet” case by returning an empty vector.
- Use
LockedIncentiveResponse+unpack_locked_incentiveto give a convenient tuple form.These are the right minimal surface for other modules and tests (e.g.,
vip::lock_stakingandvip::test) to introspect lock state.
262-524: Test suite gives strong coverage of all main behaviors and error pathsThe tests in this module cover:
- Registration/deregistration of whitelisted metadata.
- Single and repeated
stake_internalcalls aggregating per key.- Successful
unstake_internalpartial + full unlock, including cleanup of entries.- All major failure cases: early unstake, missing staking account, missing entry, zero amount, and insufficient balance.
This breadth of testing significantly reduces the risk of subtle accounting bugs or missed guard conditions in production.
2 participants
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores
Style
✏️ Tip: You can customize this high-level summary in your review settings.