Feat/delisted vesting #57
Conversation
WalkthroughRefactors snapshot loading to copy semantics, adds deregistered-bridge handling and stricter stage validation, exposes new constants and an update_bridge_address entry, adjusts reward/vesting flows to handle deregistered state, updates tests, and changes development addresses in Move.toml. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant VIP as vip::module
participant ModuleStore
participant Bridge
participant Vesting
Client->>VIP: submit_claim(request with bridge_id, stage)
VIP->>ModuleStore: load_snapshot_copy(stage, bridge_id, version)
ModuleStore-->>VIP: Snapshot or mock (if deregistered)
VIP->>Bridge: query bridge registration/state
Bridge-->>VIP: registered? / last_version
alt bridge deregistered
VIP->>Vesting: compute vesting with is_deregistered = true
Vesting-->>VIP: vesting events (UserVestingCreate/Finalized)
else bridge active
VIP->>Vesting: compute vesting with is_deregistered = false
Vesting-->>VIP: vesting events and fund transfers
end
VIP-->>Client: claim result / emitted events
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
sources/vip.move (2)
1499-1517: Move the zero-stage guard before claimability checks.
check_user_reward_claimablecan abort withESTAGE_DATA_NOT_FOUNDfor stage 0, so the newEZERO_STAGEis unlikely to surface. Validatingstart_stage != 0first also avoids unnecessary snapshot lookups.🛠️ Proposed change
- check_user_reward_claimable( - module_store, - account, - bridge_id, - version, - start_stage, - end_stage - ); - let vesting_period = module_store.vesting_period; - let minimum_score_ratio = module_store.minimum_score_ratio; - - add_tvl_snapshot_internal(module_store); - - // ensure start stage is not 0 - assert!(start_stage != 0, error::invalid_argument(EZERO_STAGE)); + // ensure start stage is not 0 + assert!(start_stage != 0, error::invalid_argument(EZERO_STAGE)); + check_user_reward_claimable( + module_store, + account, + bridge_id, + version, + start_stage, + end_stage + ); + let vesting_period = module_store.vesting_period; + let minimum_score_ratio = module_store.minimum_score_ratio; + + add_tvl_snapshot_internal(module_store);
1532-1557: Skip merkle-proof verification for mock snapshots.
When a deregistered bridge returns a mock snapshot (emptymerkle_root), a non‑zerol2_scorewill always fail proof validation before you override it toU64_MAX. Gating the proof check on!is_deregisteredmakes the API more resilient to caller input.🛠️ Proposed change
- // check merkle proof - let target_hash = - score_hash( - bridge_id, - *stage, - account_addr, - *l2_score, - snapshot.total_l2_score - ); - if (*l2_score != 0) { - assert_merkle_proofs( - *merkle_proof, - snapshot.merkle_root, - target_hash - ); - }; - - // if deregistered and snapshot is not submitted, update score to u64 max to reach minimum score - let is_deregistered = snapshot.merkle_root.length() == 0; + let is_deregistered = snapshot.merkle_root.length() == 0; + if (!is_deregistered) { + let target_hash = + score_hash( + bridge_id, + *stage, + account_addr, + *l2_score, + snapshot.total_l2_score + ); + if (*l2_score != 0) { + assert_merkle_proofs( + *merkle_proof, + snapshot.merkle_root, + target_hash + ); + }; + }; + + // if deregistered and snapshot is not submitted, update score to u64 max to reach minimum score if (is_deregistered) { l2_score = &U64_MAX };
🧹 Nitpick comments (1)
sources/test.move (1)
1338-1341: Fix the test name typo (calim→claim).
Improves readability and makes test output/search more consistent.📝 Proposed rename
- fun calim_deregistered_bridge_reward( + fun claim_deregistered_bridge_reward(
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@sources/vip.move`:
- Around line 1282-1299: The update_bridge_address function should explicitly
validate the bridge exists: after calling get_last_bridge_version (used in
update_bridge_address) check the is_registered boolean and if false abort with a
descriptive error (e.g., "bridge not found") instead of proceeding; do this
before constructing BridgeInfoKey and calling table::borrow_mut on
module_store.bridges, so the ModuleStore borrow and table access only occur for
registered bridges and return a clear error rather than a generic table abort.
- Around line 949-965: In check_user_reward_claimable, when last_claimed_stage
== 0 (first-time claimers) add an explicit assertion that start_stage >=
init_stage to mirror the deregistered-bridge check and prevent claims before
init_stage; locate the block that computes last_claimed_stage via
vesting::get_user_last_claimed_stage(account_addr, bridge_id, version) and
insert an assert for start_stage >= init_stage (use the same error code used for
invalid stages, e.g. EINVALID_CLAIMABLE_STAGE) before proceeding with snapshot
validation or other checks.
🧹 Nitpick comments (2)
sources/vip.move (2)
454-456: Fix typos in the comment.The comment contains spelling errors: "Dereigstered" should be "Deregistered" and "submitt" should be "submit".
📝 Suggested fix
- // Dereigstered rollup cannot submitt snapshot + // Deregistered rollup cannot submit snapshot
2062-2086: Consider using a more specific error code for the stage progression check.The assertion at lines 2070-2075 validates that the next stage has started before allowing claims on a deregistered bridge. Using
ESTAGE_DATA_NOT_FOUNDfor this check may be confusing since the actual intent is to ensure the stage is "complete" before claiming. Consider introducing a dedicated error constant likeESTAGE_NOT_FINALIZEDfor clarity.Also, the mock snapshot returns
create_time: 0which causesis_after_challenge_periodto returntrueimmediately. This appears intentional for deregistered bridges but deserves a comment explaining this design choice.
2 participants
Summary by CodeRabbit
Bug Fixes
New Features
Tests
Chores