fix(cardano): Handle pool re-registrations #822
Conversation
WalkthroughThis PR introduces pool delegator retirement tracking by adding a new Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/cardano/src/ewrap/drops.rs (1)
88-91: Potential panic if invariant is violated.The
unreachable!macro will panic ifprev_poolis notSome(PoolDelegation::Pool(_)). While this should be guaranteed by the check invisit_account(line 161-162), defensive coding might be preferable to avoid panics in edge cases.Consider using a warning log instead:
🔎 Proposed defensive handling
- let Some(PoolDelegation::Pool(pool)) = self.prev_pool else { - unreachable!("account delegated to pool") - }; - entity.retired_pool = Some(pool); + if let Some(PoolDelegation::Pool(pool)) = self.prev_pool { + entity.retired_pool = Some(pool); + } else { + warn!(delegator=%self.delegator, "expected pool delegation but found {:?}", self.prev_pool); + }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
crates/cardano/src/ewrap/drops.rscrates/cardano/src/genesis/staking.rscrates/cardano/src/model.rscrates/cardano/src/roll/accounts.rscrates/minibf/src/routes/accounts.rs
🧰 Additional context used
🧬 Code graph analysis (2)
crates/cardano/src/roll/accounts.rs (1)
crates/cardano/src/model.rs (1)
undo(1974-2009)
crates/cardano/src/ewrap/drops.rs (1)
crates/cardano/src/roll/accounts.rs (24)
new(28-34)new(119-129)new(177-185)new(233-247)new(293-304)new(381-388)apply(53-64)apply(86-96)apply(140-155)apply(196-210)apply(258-268)apply(315-337)apply(359-363)apply(399-412)apply(437-448)undo(66-68)undo(98-102)undo(157-162)undo(212-217)undo(270-275)undo(339-342)undo(365-369)undo(414-419)undo(450-452)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Check Build
🔇 Additional comments (7)
crates/minibf/src/routes/accounts.rs (1)
96-101: LGTM! Appropriate fallback for retired pool delegation.The fallback to
retired_poolwhendelegated_pool_at(current_epoch)returnsNonecorrectly preserves pool information for accounts whose delegated pool has retired. This ensures API consumers can still identify the pool the account was delegated to.crates/cardano/src/roll/accounts.rs (1)
165-218: LGTM! Proper undo/redo semantics for retired pool state.The
StakeDelegationdelta correctly:
- Saves
retired_poolbefore applying (line 203)- Clears
retired_poolwhen applying a new delegation (line 209) - this is correct because a new delegation supersedes the retired state- Restores
retired_poolon undo (line 216)This ensures that chain rollbacks properly restore the retired pool tracking state.
crates/cardano/src/genesis/staking.rs (1)
101-111: LGTM! Correct initialization for genesis accounts.Initializing
retired_pool: Nonefor genesis delegations is correct since these accounts are bootstrapped with active pools, not retired ones.crates/cardano/src/ewrap/drops.rs (1)
161-165: LGTM! Boundary processing correctly detects retiring pools.The logic properly checks if the account's delegated pool is in the
retiring_poolsset and emits aPoolDelegatorRetiredelta accordingly.crates/cardano/src/model.rs (3)
536-539: LGTM! Appropriate field addition with correct CBOR tagging.The
retired_poolfield is properly tagged with#[n(7)]following the existing sequential CBOR encoding pattern, and theOption<PoolHash>type correctly represents the optional retired pool state.
543-555: LGTM! Correct initialization in constructor.The
AccountState::newproperly initializesretired_pool: None, consistent with the genesis initialization instaking.rs.
1823-1823: LGTM! Complete delta integration.The
PoolDelegatorRetiredelta is fully wired into theCardanoDeltaenum with:
- Enum variant (line 1823)
delta_from!macro (line 1891)key()dispatch (line 1928)apply()dispatch (line 1966)undo()dispatch (line 2003)Also applies to: 1891-1891, 1928-1928, 1966-1966, 2003-2003
| fn undo(&self, _entity: &mut Option<AccountState>) { | ||
| // todo!() | ||
| // Placeholder undo logic. Ensure this does not panic. | ||
| } |
There was a problem hiding this comment.
Placeholder undo logic may cause issues during chain rollbacks.
The undo method is not implemented, which could lead to incorrect state after rollbacks. Unlike some other deltas where undo is less critical, PoolDelegatorRetire::apply modifies both entity.pool (via schedule) and entity.retired_pool. If a rollback occurs, these changes won't be reversed.
🔎 Proposed undo implementation
fn undo(&self, _entity: &mut Option<AccountState>) {
- // todo!()
- // Placeholder undo logic. Ensure this does not panic.
+ let Some(entity) = _entity.as_mut() else {
+ warn!(delegator=%self.delegator, "missing account during undo");
+ return;
+ };
+
+ // Restore previous pool delegation
+ entity.pool.reset(self.prev_pool.clone());
+ // Clear the retired_pool since we're undoing the retirement
+ entity.retired_pool = None;
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In crates/cardano/src/ewrap/drops.rs around lines 94 to 97, implement undo to
reverse the changes made by PoolDelegatorRetire::apply: remove the scheduled
retirement entry from entity.pool.schedule that matches this delta (epoch and
pool id) and restore entity.retired_pool to its previous state (clear it or set
it back if it equals the pool id and epoch scheduled by this delta). Ensure you
safely handle None entity, only mutate when the pool and retired_pool match the
values set by apply, and avoid panics.
| pub credential: StakeCredential, | ||
|
|
||
| #[n(7)] | ||
| pub retired_pool: Option<PoolHash>, |
There was a problem hiding this comment.
let's put a cbor default attribute here so that previous snapshots still work regardless of the existence of the new value.
Summary by CodeRabbit
New Features
✏️ Tip: You can customize this high-level summary in your review settings.