refactor(cardano): use deltas for the sweep process - #717
Conversation
WalkthroughAdds visitor-driven, domain-based boundary processing producing undoable Cardano deltas (retirements, expirations, rewards, transitions); extends model enums/fields; consolidates commit to apply whole namespaces; replaces per-entity sweep storage with WorkDeltas; renames a pallas helper and adds pool reward account helper. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Runner as Sweep Runner
participant Domain
participant Boundary as BoundaryWork
participant Visitors as BoundaryVisitors
participant Deltas as WorkDeltas
participant Commit as commit()
Runner->>Boundary: new BoundaryWork(...)
Runner->>Boundary: compute(domain)
Boundary->>Domain: state() iter Pool/DRep/Account
loop per entity
Boundary->>Visitors: visit_pool / visit_drep / visit_account
Visitors->>Deltas: push CardanoDelta (retire/expire/drop/reward/transition)
end
Boundary->>Visitors: flush()
Visitors->>Deltas: finalize pending deltas
Runner->>Boundary: commit(domain)
Boundary->>Commit: apply_whole_namespace for Account/Pool/DRep
Commit->>Domain: writer()
Commit->>Domain: write updated entities (apply deltas)
Commit->>Boundary: run epoch lifecycle (drop/promote)
sequenceDiagram
autonumber
participant BW as BoundaryWork
participant Rewards as RewardsVisitor
participant Pools as PoolState
participant Delegs as DelegatorMap
participant Deltas as WorkDeltas
BW->>Rewards: visit_pool(pool)
Rewards->>Pools: read pool params & stake
Rewards->>Rewards: compute_pool_reward()
alt pool reward account present
Rewards->>Deltas: AssignPoolRewards(pool, account, operator_share)
else
Note right of Rewards: warn missing reward account
end
loop each delegator
Rewards->>Rewards: compute_delegator_reward()
Rewards->>Deltas: AssignDelegatorRewards(account, reward)
end
sequenceDiagram
autonumber
participant BW as BoundaryWork
participant Retires as RetiresVisitor
participant Pool as PoolState
participant Accounts as AccountState
participant Deltas as WorkDeltas
BW->>Retires: visit_pool(pool)
Retires->>Retires: should_retire_pool()
alt retire now
Retires->>Deltas: PoolDeRegistration / PoolDelegatorDrop (per delegator)
end
BW->>Retires: visit_drep(drep)
Retires->>Retires: should_expire_drep()
alt expire now
Retires->>Deltas: DRepExpiration / DRepDelegatorDrop (per delegator)
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
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: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
crates/minibf/src/mapping.rs (1)
592-596: Removedbg!calls from deposit calculation.Leaking cert data and deposit amounts to stderr in production is noisy and can bloat logs.
Apply this diff:
- if pallas_extras::cert_as_stake_registration(x).is_some() { - dbg!(x); - return Some(dbg!(key_deposit)); - } + if pallas_extras::cert_as_stake_registration(x).is_some() { + return Some(key_deposit); + }crates/core/src/batch.rs (1)
12-16: DerivingDebughere imposesC::Delta: Debugand may break existingChainLogicimplementors.
#[derive(Debug)]onWorkDeltas<C>requiresC::Delta: Debug. If anyDeltadoesn’t implementDebug, this won’t compile. Prefer a manualDebugimpl that avoids extra bounds.Apply this diff to drop the derive:
-#[derive(Debug)] pub struct WorkDeltas<C: ChainLogic> { pub entities: HashMap<NsKey, Vec<C::Delta>>, pub slot: SlotTags, }Add this manual
Debugimpl elsewhere in the file:impl<C: ChainLogic> std::fmt::Debug for WorkDeltas<C> { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { f.debug_struct("WorkDeltas") .field("entities_len", &self.entities.len()) .field("slot", &self.slot) .finish() } }crates/cardano/src/roll/epochs.rs (3)
40-46: Bug: stake deregistration uses pool deposit instead of key deposit.Decayed deposits from stake deregistration should reference the key deposit, not the pool deposit.
Apply this diff:
- entity.decayed_deposits += - self.stake_deregistration_count * entity.pparams.pool_deposit_or_default(); + entity.decayed_deposits += + self.stake_deregistration_count * entity.pparams.key_deposit_or_default();Also fix the undo accordingly (Lines 60-62):
- entity.decayed_deposits -= - self.stake_deregistration_count * entity.pparams.pool_deposit_or_default(); + entity.decayed_deposits -= + self.stake_deregistration_count * entity.pparams.key_deposit_or_default();
1101-1121: Off-by-one in delegation active epoch.You compute
active_epochasepoch + 1upstream and add+1again here, resulting inepoch + 2.Apply this diff:
- active_epoch: active_epoch + 1, + active_epoch,
260-269: Replace the first duplicate mapping with the correct getter
Change line 260 fromall_proposed_ada_per_utxo_byte => MinUtxoValue,to
all_proposed_min_utxo_value => MinUtxoValue,and keep only the existing
all_proposed_ada_per_utxo_byte => AdaPerUtxoByteentry.
🧹 Nitpick comments (6)
crates/cardano/src/sweep/commit.rs (1)
88-119: Consider optimizing namespace iteration to avoid removing all entities upfront.The current implementation iterates through all entities in a namespace and removes matching deltas from the map. For namespaces with many entities but few deltas, this could be inefficient. Consider iterating over deltas first and loading only the entities that need updates.
crates/cardano/src/sweep/compute.rs (1)
298-340: Consider extracting visitor orchestration into a helper method.The repetitive pattern of visitor initialization, entity iteration, and flush could be extracted into a reusable helper to reduce code duplication and improve maintainability.
Example refactor:
fn apply_visitors<D: Domain>( &mut self, domain: &D, visitors: &mut [&mut dyn BoundaryVisitor], ) -> Result<(), ChainError> { // Apply to pools let pools = domain.state().iter_entities_typed::<PoolState>(PoolState::NS, None)?; for pool in pools { let (pool_id, pool) = pool?; for visitor in visitors.iter_mut() { visitor.visit_pool(self, &pool_id, &pool)?; } } // Similar for dreps and accounts... // Then flush all for visitor in visitors.iter_mut() { visitor.flush(self)?; } Ok(()) }crates/cardano/src/model.rs (1)
1035-1035: Clean up commented code or add explanation.The commented-out
AssignEpochRewardsvariant appears in multiple places. Either remove it completely or add a comment explaining why it's kept for future use.Also applies to: 1098-1098
crates/cardano/src/sweep/rewards.rs (1)
199-212: Consider batching delegator reward deltas.Creating individual deltas for each delegator could be memory-intensive for large pools. Consider batching or streaming the deltas.
Instead of collecting all deltas in a vector first, apply them directly:
-let mut delegators = vec![]; - for (delegator, stake) in ctx.active_snapshot.accounts_by_pool.iter_delegators(id) { let reward = compute_delegator_reward(total_pool_reward, pool_stake, *stake); - - delegators.push(AssignDelegatorRewards { + ctx.add_delta(AssignDelegatorRewards { account: delegator.clone(), reward, }); } - -for delta in delegators { - ctx.add_delta(delta); -}crates/cardano/src/sweep/mod.rs (1)
80-95: Consider documenting the EntityKey change rationale.The change from
PoolIdtoEntityKeyinDelegatorMapenables handling both pools and DReps uniformly, which is a good architectural improvement. However, this breaking change may impact external consumers of the API.Consider adding a doc comment explaining the generic nature of the map:
#[derive(Debug, Default, Clone)] +/// Maps entities (pools or DReps) to their delegators and stake amounts pub struct DelegatorMap(HashMap<EntityKey, HashMap<AccountId, u64>>);crates/cardano/src/sweep/retires.rs (1)
89-121: Consolidate duplicate DRepExpiration implementation
Extract theDRepExpirationstruct and itsEntityDeltaimpl (including unified warn/debug logging) into a shared module, then import it in bothretires.rsandtransition.rs.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
crates/cardano/src/model.rs(10 hunks)crates/cardano/src/pallas_extras.rs(2 hunks)crates/cardano/src/roll/epochs.rs(1 hunks)crates/cardano/src/roll/pools.rs(6 hunks)crates/cardano/src/sweep/commit.rs(2 hunks)crates/cardano/src/sweep/compute.rs(5 hunks)crates/cardano/src/sweep/loading.rs(2 hunks)crates/cardano/src/sweep/mod.rs(5 hunks)crates/cardano/src/sweep/retires.rs(1 hunks)crates/cardano/src/sweep/rewards.rs(1 hunks)crates/cardano/src/sweep/transition.rs(1 hunks)crates/core/src/batch.rs(1 hunks)crates/minibf/src/mapping.rs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (10)
crates/minibf/src/mapping.rs (1)
crates/cardano/src/pallas_extras.rs (1)
cert_as_pool_registration(34-86)
crates/cardano/src/roll/epochs.rs (1)
crates/cardano/src/pallas_extras.rs (1)
cert_as_pool_registration(34-86)
crates/cardano/src/sweep/retires.rs (5)
crates/cardano/src/model.rs (8)
sweep(727-737)key(1105-1135)from(866-868)from(872-874)from(878-880)from(884-887)apply(1137-1167)undo(1169-1199)crates/cardano/src/sweep/mod.rs (4)
sweep(161-177)visit_pool(33-40)visit_drep(53-60)flush(63-65)crates/cardano/src/roll/pools.rs (12)
key(39-42)key(73-75)key(99-101)key(138-141)apply(44-57)apply(77-81)apply(103-107)apply(143-158)undo(59-61)undo(83-87)undo(109-113)undo(160-164)crates/cardano/src/sweep/rewards.rs (7)
key(63-68)key(102-104)apply(70-79)apply(106-115)undo(81-90)undo(117-126)visit_pool(160-215)crates/cardano/src/sweep/transition.rs (14)
key(23-25)key(65-67)key(97-99)key(125-127)apply(27-36)apply(69-77)apply(101-105)apply(129-137)undo(38-50)undo(79-86)undo(107-111)undo(139-143)visit_pool(176-194)flush(215-221)
crates/cardano/src/sweep/rewards.rs (7)
crates/cardano/src/model.rs (10)
sweep(727-737)k(618-620)a0(622-624)key(1105-1135)from(866-868)from(872-874)from(878-880)from(884-887)apply(1137-1167)undo(1169-1199)crates/cardano/src/sweep/mod.rs (2)
sweep(161-177)visit_pool(33-40)crates/cardano/src/roll/pools.rs (11)
key(39-42)key(73-75)key(99-101)key(138-141)apply(44-57)apply(77-81)apply(103-107)undo(59-61)undo(83-87)undo(109-113)undo(160-164)crates/cardano/src/sweep/retires.rs (3)
key(19-21)undo(34-43)visit_pool(195-220)crates/cardano/src/roll/accounts.rs (21)
key(44-47)key(69-72)key(109-112)key(151-154)key(193-196)key(239-242)key(274-277)apply(49-52)apply(74-79)apply(114-121)apply(156-163)apply(198-205)apply(244-255)apply(279-282)undo(54-57)undo(81-84)undo(123-126)undo(165-168)undo(207-210)undo(257-262)undo(284-287)crates/cardano/src/sweep/transition.rs (1)
visit_pool(176-194)crates/cardano/src/pallas_extras.rs (1)
pool_reward_account(288-291)
crates/cardano/src/sweep/transition.rs (3)
crates/cardano/src/model.rs (4)
sweep(727-737)key(1105-1135)apply(1137-1167)undo(1169-1199)crates/cardano/src/sweep/mod.rs (4)
sweep(161-177)visit_pool(33-40)visit_account(43-50)flush(63-65)crates/cardano/src/sweep/retires.rs (15)
key(19-21)key(57-59)key(96-98)key(134-136)apply(23-32)apply(61-74)apply(100-109)apply(138-151)undo(34-43)undo(76-85)undo(111-120)undo(153-162)should_retire_pool(165-175)visit_pool(195-220)flush(254-260)
crates/cardano/src/sweep/mod.rs (6)
crates/cardano/src/sweep/commit.rs (2)
commit(122-148)domain(97-97)crates/cardano/src/sweep/compute.rs (4)
compute(294-349)domain(302-304)domain(314-316)domain(326-328)crates/cardano/src/sweep/retires.rs (3)
visit_pool(195-220)visit_drep(222-252)flush(254-260)crates/cardano/src/sweep/rewards.rs (1)
visit_pool(160-215)crates/cardano/src/sweep/transition.rs (3)
visit_pool(176-194)visit_account(196-213)flush(215-221)crates/cardano/src/roll/epochs.rs (1)
flush(291-301)
crates/cardano/src/sweep/commit.rs (4)
crates/cardano/src/sweep/mod.rs (1)
sweep(161-177)crates/cardano/src/sweep/compute.rs (3)
domain(302-304)domain(314-316)domain(326-328)crates/cardano/src/sweep/loading.rs (1)
domain(53-55)crates/core/src/state.rs (2)
commit(270-270)commit(498-500)
crates/cardano/src/roll/pools.rs (3)
crates/cardano/src/model.rs (11)
roll(706-724)new(228-249)new(556-561)new(836-845)key(1105-1135)from(866-868)from(872-874)from(878-880)from(884-887)apply(1137-1167)undo(1169-1199)crates/cardano/src/roll/accounts.rs (26)
new(26-32)new(97-103)new(139-145)new(181-187)new(225-233)key(44-47)key(69-72)key(109-112)key(151-154)key(193-196)key(239-242)key(274-277)apply(49-52)apply(74-79)apply(114-121)apply(156-163)apply(198-205)apply(244-255)apply(279-282)undo(54-57)undo(81-84)undo(123-126)undo(165-168)undo(207-210)undo(257-262)undo(284-287)crates/cardano/src/pallas_extras.rs (2)
cert_as_pool_registration(34-86)pool_reward_account(288-291)
crates/cardano/src/sweep/loading.rs (5)
crates/cardano/src/model.rs (2)
drep_to_entity_key(801-811)sweep(727-737)crates/cardano/src/eras.rs (1)
load_active_era(182-197)crates/cardano/src/utils.rs (1)
mutable_slots(12-15)crates/cardano/src/sweep/mod.rs (1)
sweep(161-177)crates/core/src/batch.rs (1)
default(19-24)
crates/cardano/src/sweep/compute.rs (9)
crates/cardano/src/model.rs (2)
sweep(727-737)rewards(788-792)crates/cardano/src/sweep/mod.rs (1)
sweep(161-177)crates/cardano/src/sweep/commit.rs (1)
domain(97-97)crates/cardano/src/sweep/loading.rs (1)
domain(53-55)crates/minibf/src/routes/network.rs (1)
domain(191-200)crates/minibf/src/routes/pools.rs (2)
domain(83-84)domain(150-151)crates/minibf/src/routes/epochs/mod.rs (1)
domain(26-28)crates/cardano/src/eras.rs (1)
domain(183-185)crates/cardano/src/lib.rs (3)
domain(136-138)domain(156-158)domain(164-166)
🔇 Additional comments (20)
crates/minibf/src/mapping.rs (1)
597-599: Switch tocert_as_pool_registrationlooks correct.This aligns minibf’s deposit logic with the renamed helper and preserves semantics across eras.
crates/cardano/src/roll/epochs.rs (1)
231-233: Use ofcert_as_pool_registrationis consistent with the API rename.Matches the updated helper and keeps the counting logic straightforward.
crates/cardano/src/sweep/loading.rs (1)
103-104: InitializedeltaswithWorkDeltas::default()— good.This matches the new delta-driven sweep flow.
crates/cardano/src/pallas_extras.rs (2)
34-86: Renamed helpercert_as_pool_registrationis well-formed.Covers both Alonzo-compatible and Conway PoolRegistration variants and normalizes fields.
288-291: Verify reward account bytes represent full address in all era variants
Confirm that everyreward_accountinMultiEraPoolRegistrationencodes a complete stake/reward address (including network tag and credential) soAddress::from_bytes(...).ok()?andaddress_as_stake_credalways yield the correctStakeCredential.crates/cardano/src/roll/pools.rs (3)
91-91: LGTM: Renaming aligns with blockchain terminology standards.The rename from
PoolRetirementtoPoolDeRegistrationbetter matches the terminology used in the codebase where "registration/deregistration" pairs are consistent (e.g.,StakeRegistration/StakeDeregistration).
125-133: LGTM: Pool account initialization ensures rewards can be tracked.The new
PoolAccountDetecteddelta properly handles pool reward account initialization, which is essential since pool reward accounts don't go through standard stake registration. This ensures reward distribution works correctly for pool operators.Also applies to: 189-199
148-150: Fix incorrect entity key derivation.The function creates an
EntityKeyfromaccount_keybytes but then wraps it again withEntityKey::from(). This double-wrapping is redundant sinceaccount_keyis already anEntityKey.Apply this fix:
let account_key = minicbor::to_vec(&self.reward_account).unwrap(); -let account_key = EntityKey::from(account_key); -debug!(operator=%self.operator, account_key=%account_key, "initializing pool account"); +debug!(operator=%self.operator, account_key=%hex::encode(&account_key), "initializing pool account");Likely an incorrect or invalid review comment.
crates/cardano/src/model.rs (1)
211-212: LGTM: Boolean flags for retirement state tracking.Adding
is_retiredtoPoolStateandexpiredtoDRepStateprovides clear state tracking for entity lifecycle management.Also applies to: 831-832
crates/cardano/src/sweep/mod.rs (4)
3-10: LGTM!The imports are properly organized and the new
WorkDeltastype aligns well with the delta-driven architecture being introduced.
31-66: Well-designed visitor pattern implementation.The
BoundaryVisitortrait provides a clean abstraction for the sweep process with sensible defaults. The#[allow(unused_variables)]pragmas are appropriate for trait methods with default implementations.
150-158: Clean helper methods for BoundaryWork.The
starting_epoch_no()andadd_delta()methods provide good encapsulation. The delta addition pattern is consistent with the new architecture.
166-166: compute retains genesis accessThe new signature still calls
domain.genesis()indefine_era_transition, so all required genesis data remains available.crates/cardano/src/sweep/retires.rs (4)
23-32: Good defensive programming with entity checks.The pattern of checking for entity existence before applying changes and using appropriate logging levels (warn for missing, debug for operations) is well implemented.
207-217: Consider populating prev_pool_id for undo support.The
PoolDelegatorDropinstances are created withprev_pool_id: None, which means undo operations won't restore the previous pool delegation. This might be intentional for retirements, but consider whether preservation of undo information is needed.- self.deltas.push( - PoolDelegatorDrop { - delegator: delegator.clone(), - prev_pool_id: None, - } - .into(), - ); + // For retirements, we intentionally don't preserve prev_pool_id + // as retired pools shouldn't be restored through undo + self.deltas.push( + PoolDelegatorDrop { + delegator: delegator.clone(), + prev_pool_id: None, + } + .into(), + );
177-187: Verify DRep inactivity period calculation
ctx.valid_drep_inactivity_period()delegates toPParams::ensure_drep_inactivity_period()in chain-libs; manually confirm it returns the exact on-chain drep inactivity period (correct units and defaults).
11-14: Naming distinction is intentional:PoolDeRegistrationmodels the on-chain de-registration certificate in the roll pipeline, whilePoolRetirementrepresents the subsequent sweep-level retirement event. No change required.Likely an incorrect or invalid review comment.
crates/cardano/src/sweep/transition.rs (3)
89-144: Remove duplicate delta types.As noted in the retires.rs review,
DRepExpirationandDRepDelegatorDropare duplicated between this file andretires.rs. This duplication should be resolved.
182-182: Confirmed get_pool_stake returns 0 for missing pools. No additional checks needed.
27-36: Document and verify AccountTransition.apply order
Incrates/cardano/src/sweep/transition.rs’sAccountTransition::apply, add a// order matterscomment (as inPoolTransition) and confirm that the sequence—
active_pool = latest_poolactive_drep = latest_drepactive_stake = wait_stakewait_stake = live_stake()
—accurately reflects Cardano’s epoch boundary rules.
|
|
||
| let genesis = | ||
| Genesis::from_file_paths(byron, shelley, alonzo, conway, force_protocol).unwrap(); | ||
| let domain = todo!(); |
There was a problem hiding this comment.
Replace todo!() with proper test setup.
Using todo!() in tests will cause panics when the test runs. This should be replaced with a proper mock or test domain implementation.
The test needs a proper domain implementation. Consider creating a test-specific domain mock or using a test helper to create a domain instance.
🤖 Prompt for AI Agents
In crates/cardano/src/sweep/compute.rs around line 447, replace the placeholder
let domain = todo!() with a real test domain setup: create or reuse a
test-domain/mock that implements the same trait(s) the sweep code expects,
construct it with the minimal state needed for this test (UTxO set, script
contexts, keys, or config fields), import the test helper or mock module, and
instantiate domain before use; ensure the mock implements any async or trait
methods called by the code and populate fields so the test runs
deterministically instead of panicking.
| fn compute_delegator_reward( | ||
| available_rewards: u64, | ||
| total_delegated: u64, | ||
| delegator_stake: u64, | ||
| ) -> u64 { | ||
| let share = (delegator_stake as f64 / total_delegated as f64) * available_rewards as f64; | ||
| share.round() as u64 | ||
| } |
There was a problem hiding this comment.
Potential precision loss in reward calculations.
Using floating-point arithmetic for financial calculations can lead to rounding errors. Consider using fixed-point arithmetic or the num-rational crate for precise calculations.
Consider using rational arithmetic similar to other parts of the codebase:
use num_rational::Rational64;
fn compute_delegator_reward(
available_rewards: u64,
total_delegated: u64,
delegator_stake: u64,
) -> u64 {
let numerator = (available_rewards as i128) * (delegator_stake as i128);
let denominator = total_delegated as i128;
((numerator + denominator / 2) / denominator) as u64 // Round to nearest
}🤖 Prompt for AI Agents
In crates/cardano/src/sweep/rewards.rs around lines 19 to 26, the current
implementation uses floating-point arithmetic which can lose precision for
financial reward calculations; replace the float-based computation with integer
(or rational) arithmetic to avoid rounding errors — compute numerator =
available_rewards * delegator_stake as a 128-bit integer, compute denominator =
total_delegated as 128-bit, then perform integer division with rounding to
nearest (add denominator/2 before dividing) and cast the result back to u64;
ensure you handle potential zero/overflow edge cases (e.g., total_delegated ==
0) and use signed 128-bit temporaries to match the example in the review
comment.
| let z0 = 1.0 / k as f64; | ||
| let sigma = pool_stake as f64 / total_active_stake as f64; | ||
| let s = pool.declared_pledge as f64 / total_active_stake as f64; | ||
| let sigma_prime = sigma.min(z0); | ||
|
|
||
| let r = total_rewards as f64; | ||
| let a0 = a0.numerator as f64 / a0.denominator as f64; | ||
| let r_pool = r * (sigma_prime + s.min(sigma) * a0 * (sigma_prime - sigma)); | ||
|
|
||
| let r_pool_u64 = r_pool.round() as u64; | ||
| let after_cost = r_pool_u64.saturating_sub(pool.fixed_cost); | ||
| let pool_margin_cost = pool.margin_cost.numerator as f64 / pool.margin_cost.denominator as f64; | ||
| let operator_share = pool.fixed_cost + ((after_cost as f64) * pool_margin_cost).round() as u64; | ||
|
|
||
| (r_pool_u64, operator_share) |
There was a problem hiding this comment.
Add validation for protocol parameters.
The function should validate that k > 0 and a0 denominator is non-zero to prevent division by zero.
Add validation at the beginning of the function:
if k == 0 {
return (0, 0);
}
if a0.denominator == 0 {
return (0, 0);
}🤖 Prompt for AI Agents
In crates/cardano/src/sweep/rewards.rs around lines 36 to 50, the code divides
by k and by a0.denominator without validating them, which can cause
division-by-zero panics; add early checks at the start of the function that if k
== 0 or a0.denominator == 0 then immediately return (0, 0), ensuring no further
calculations occur with invalid protocol params.
| fn undo(&self, entity: &mut Option<AccountState>) { | ||
| let Some(entity) = entity else { | ||
| return; | ||
| }; | ||
|
|
||
| entity.latest_drep = entity.active_drep.clone(); | ||
| entity.latest_pool = entity.active_pool.clone(); | ||
| entity.wait_stake = entity.active_stake; | ||
|
|
||
| entity.active_pool = self.prev_pool.clone(); | ||
| entity.active_drep = self.prev_drep.clone(); | ||
| entity.active_stake = self.prev_stake.unwrap_or(0); | ||
| } |
There was a problem hiding this comment.
Potential data loss in undo if prev values aren't captured.
The undo implementation relies on prev_pool, prev_drep, and prev_stake being set, but the apply method doesn't populate these fields. This could lead to incorrect state restoration.
The apply method should capture the previous values before modifying the entity:
fn apply(&mut self, entity: &mut Option<AccountState>) {
let Some(entity) = entity else {
return;
};
+ // Save current state for undo
+ self.prev_pool = entity.active_pool.clone();
+ self.prev_drep = entity.active_drep.clone();
+ self.prev_stake = Some(entity.active_stake);
+
entity.active_pool = entity.latest_pool.clone();
entity.active_drep = entity.latest_drep.clone();
entity.active_stake = entity.wait_stake;
entity.wait_stake = entity.live_stake();
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| fn undo(&self, entity: &mut Option<AccountState>) { | |
| let Some(entity) = entity else { | |
| return; | |
| }; | |
| entity.latest_drep = entity.active_drep.clone(); | |
| entity.latest_pool = entity.active_pool.clone(); | |
| entity.wait_stake = entity.active_stake; | |
| entity.active_pool = self.prev_pool.clone(); | |
| entity.active_drep = self.prev_drep.clone(); | |
| entity.active_stake = self.prev_stake.unwrap_or(0); | |
| } | |
| fn apply(&mut self, entity: &mut Option<AccountState>) { | |
| let Some(entity) = entity else { | |
| return; | |
| }; | |
| // Save current state for undo | |
| self.prev_pool = entity.active_pool.clone(); | |
| self.prev_drep = entity.active_drep.clone(); | |
| self.prev_stake = Some(entity.active_stake); | |
| entity.active_pool = entity.latest_pool.clone(); | |
| entity.active_drep = entity.latest_drep.clone(); | |
| entity.active_stake = entity.wait_stake; | |
| entity.wait_stake = entity.live_stake(); | |
| } |
🤖 Prompt for AI Agents
In crates/cardano/src/sweep/transition.rs around lines 38 to 50, the undo method
assumes prev_pool, prev_drep and prev_stake were populated but apply does not
capture them, risking incorrect restoration; update the apply implementation to
read the entity's current active_pool, active_drep and active_stake and assign
them into self.prev_pool, self.prev_drep and self.prev_stake (wrap the stake in
Some), before mutating the entity fields, and ensure prev_* are Option<T>
semantics are respected so undo can reliably restore the prior values.
| fn should_retire_pool(ctx: &mut BoundaryWork, pool: &PoolState) -> bool { | ||
| if pool.is_retired { | ||
| return false; | ||
| } | ||
|
|
||
| let Some(retiring_epoch) = pool.retiring_epoch else { | ||
| return false; | ||
| }; | ||
|
|
||
| retiring_epoch <= ctx.starting_epoch_no() | ||
| } | ||
|
|
||
| fn should_expire_drep(ctx: &mut BoundaryWork, drep: &DRepState) -> Result<bool, ChainError> { | ||
| let last_activity_slot = drep | ||
| .last_active_slot | ||
| .unwrap_or(drep.initial_slot.unwrap_or_default()); | ||
|
|
||
| let (last_activity_epoch, _) = ctx.active_era.slot_epoch(last_activity_slot); | ||
|
|
||
| let expiring_epoch = last_activity_epoch as u64 + ctx.valid_drep_inactivity_period()?; | ||
|
|
||
| Ok(expiring_epoch <= ctx.starting_epoch_no()) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider extracting shared helper functions.
The should_retire_pool and should_expire_drep functions are duplicated from retires.rs. Consider moving these to a shared module or the parent mod.rs.
Move these functions to crates/cardano/src/sweep/mod.rs as they're used by multiple visitor implementations:
-fn should_retire_pool(ctx: &mut BoundaryWork, pool: &PoolState) -> bool {
- // implementation
-}
-
-fn should_expire_drep(ctx: &mut BoundaryWork, drep: &DRepState) -> Result<bool, ChainError> {
- // implementation
-}
+// Import from parent module
+use super::{should_retire_pool, should_expire_drep};Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
crates/cardano/src/sweep/commit.rs(2 hunks)crates/cardano/src/sweep/rewards.rs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
crates/cardano/src/sweep/rewards.rs (5)
crates/cardano/src/sweep/mod.rs (2)
sweep(161-177)visit_pool(33-40)crates/cardano/src/model.rs (10)
sweep(727-737)k(618-620)a0(622-624)key(1105-1135)from(866-868)from(872-874)from(878-880)from(884-887)apply(1137-1167)undo(1169-1199)crates/cardano/src/roll/pools.rs (11)
key(39-42)key(73-75)key(99-101)key(138-141)apply(44-57)apply(77-81)apply(103-107)undo(59-61)undo(83-87)undo(109-113)undo(160-164)crates/cardano/src/sweep/transition.rs (1)
visit_pool(176-194)crates/cardano/src/pallas_extras.rs (1)
pool_reward_account(288-291)
crates/cardano/src/sweep/commit.rs (3)
crates/cardano/src/sweep/mod.rs (1)
sweep(161-177)crates/cardano/src/sweep/loading.rs (1)
domain(53-55)crates/cardano/src/sweep/compute.rs (3)
domain(302-304)domain(314-316)domain(326-328)
🔇 Additional comments (1)
crates/cardano/src/sweep/commit.rs (1)
131-133: Do not silently drop leftover deltas.When
self.deltas.entitiesstays non-empty we skip applying those deltas and still commit, so we lose state updates. Please surface this as an error (e.g., introduce aBrokenInvariant::UncommittedDeltasand return it) instead of logging a warning.- if !self.deltas.entities.is_empty() { - warn!(quantity = %self.deltas.entities.len(), "uncommitted deltas"); - } + if !self.deltas.entities.is_empty() { + return Err(ChainError::from(BrokenInvariant::UncommittedDeltas)); + }
| let (total_pool_reward, operator_share) = compute_pool_reward( | ||
| pot_delta.available_rewards, | ||
| ctx.active_snapshot.total_stake, | ||
| pool, | ||
| pool_stake, | ||
| ctx.valid_k()?, | ||
| &ctx.valid_a0()?, | ||
| ); | ||
|
|
||
| self.total_rewards += total_pool_reward; | ||
|
|
||
| if let Some(pool_reward_account) = pallas_extras::pool_reward_account(&pool.reward_account) | ||
| { | ||
| debug!(pool=%id, "should assign pool rewards"); | ||
|
|
||
| ctx.add_delta(AssignPoolRewards { | ||
| pool: id.clone(), | ||
| pool_reward_account, | ||
| operator_share, | ||
| }); | ||
| } else { | ||
| warn!(pool=%id, "missing pool reward account"); | ||
| } | ||
|
|
||
| let mut delegators = vec![]; | ||
|
|
||
| for (delegator, stake) in ctx.active_snapshot.accounts_by_pool.iter_delegators(id) { | ||
| let reward = compute_delegator_reward(total_pool_reward, pool_stake, *stake); | ||
|
|
||
| delegators.push(AssignDelegatorRewards { | ||
| account: delegator.clone(), | ||
| reward, | ||
| }); |
There was a problem hiding this comment.
Avoid double-counting rewards.
total_pool_reward already contains the operator’s share. We add that share to the pool account and then distribute the full total_pool_reward again to delegators, minting extra ADA. Clamp the operator share to the pool total and subtract it before computing delegator rewards.
- let (total_pool_reward, operator_share) = compute_pool_reward(
+ let (total_pool_reward, mut operator_share) = compute_pool_reward(
pot_delta.available_rewards,
ctx.active_snapshot.total_stake,
pool,
pool_stake,
ctx.valid_k()?,
&ctx.valid_a0()?,
);
self.total_rewards += total_pool_reward;
- if let Some(pool_reward_account) = pallas_extras::pool_reward_account(&pool.reward_account)
+ operator_share = operator_share.min(total_pool_reward);
+ let delegators_budget = total_pool_reward.saturating_sub(operator_share);
+
+ if let Some(pool_reward_account) = pallas_extras::pool_reward_account(&pool.reward_account)
{
debug!(pool=%id, "should assign pool rewards");
ctx.add_delta(AssignPoolRewards {
pool: id.clone(),
pool_reward_account,
operator_share,
});
} else {
warn!(pool=%id, "missing pool reward account");
}
+ if pool_stake == 0 || delegators_budget == 0 {
+ return Ok(());
+ }
+
let mut delegators = vec![];
for (delegator, stake) in ctx.active_snapshot.accounts_by_pool.iter_delegators(id) {
- let reward = compute_delegator_reward(total_pool_reward, pool_stake, *stake);
+ let reward = compute_delegator_reward(delegators_budget, pool_stake, *stake);
delegators.push(AssignDelegatorRewards {
account: delegator.clone(),
reward,
});
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let (total_pool_reward, operator_share) = compute_pool_reward( | |
| pot_delta.available_rewards, | |
| ctx.active_snapshot.total_stake, | |
| pool, | |
| pool_stake, | |
| ctx.valid_k()?, | |
| &ctx.valid_a0()?, | |
| ); | |
| self.total_rewards += total_pool_reward; | |
| if let Some(pool_reward_account) = pallas_extras::pool_reward_account(&pool.reward_account) | |
| { | |
| debug!(pool=%id, "should assign pool rewards"); | |
| ctx.add_delta(AssignPoolRewards { | |
| pool: id.clone(), | |
| pool_reward_account, | |
| operator_share, | |
| }); | |
| } else { | |
| warn!(pool=%id, "missing pool reward account"); | |
| } | |
| let mut delegators = vec![]; | |
| for (delegator, stake) in ctx.active_snapshot.accounts_by_pool.iter_delegators(id) { | |
| let reward = compute_delegator_reward(total_pool_reward, pool_stake, *stake); | |
| delegators.push(AssignDelegatorRewards { | |
| account: delegator.clone(), | |
| reward, | |
| }); | |
| let (total_pool_reward, mut operator_share) = compute_pool_reward( | |
| pot_delta.available_rewards, | |
| ctx.active_snapshot.total_stake, | |
| pool, | |
| pool_stake, | |
| ctx.valid_k()?, | |
| &ctx.valid_a0()?, | |
| ); | |
| self.total_rewards += total_pool_reward; | |
| // Clamp operator share and compute the budget for delegators | |
| operator_share = operator_share.min(total_pool_reward); | |
| let delegators_budget = total_pool_reward.saturating_sub(operator_share); | |
| if let Some(pool_reward_account) = pallas_extras::pool_reward_account(&pool.reward_account) | |
| { | |
| debug!(pool=%id, "should assign pool rewards"); | |
| ctx.add_delta(AssignPoolRewards { | |
| pool: id.clone(), | |
| pool_reward_account, | |
| operator_share, | |
| }); | |
| } else { | |
| warn!(pool=%id, "missing pool reward account"); | |
| } | |
| // If there's no stake or no budget left, skip delegator distribution | |
| if pool_stake == 0 || delegators_budget == 0 { | |
| return Ok(()); | |
| } | |
| let mut delegators = vec![]; | |
| for (delegator, stake) in ctx.active_snapshot.accounts_by_pool.iter_delegators(id) { | |
| let reward = compute_delegator_reward(delegators_budget, pool_stake, *stake); | |
| delegators.push(AssignDelegatorRewards { | |
| account: delegator.clone(), | |
| reward, | |
| }); | |
| } |
🤖 Prompt for AI Agents
In crates/cardano/src/sweep/rewards.rs around lines 173 to 205,
total_pool_reward currently includes the operator's share but is later used
intact to compute delegator rewards, causing double-counting; clamp
operator_share to at most total_pool_reward (operator_share =
operator_share.min(total_pool_reward)) and subtract it from the distributable
pool before computing delegator rewards (let distributable = total_pool_reward -
operator_share) then pass distributable (not total_pool_reward) into
compute_delegator_reward for each delegator; keep adding operator_share to the
pool account as before and ensure subtraction uses a saturating/min-safe
operation to avoid underflow.
7ae517a to
d11b76c
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/cardano/src/roll/pools.rs (1)
90-114: Remove stalePoolRetiremententity delta
- crates/cardano/src/sweep/retires.rs: rename or remove
struct PoolRetirementand itsEntityDeltaimpl- crates/cardano/src/model.rs: update import, enum variant,
delta_from!, and apply/undo dispatch fromPoolRetirementtoPoolDeRegistrationcrates/cardano/src/sweep/compute.rs (1)
252-256: Prevent u64 underflow when computing largest_stable_slotepoch_start - nonce_stability_window can underflow for small epochs/windows. Use saturating_sub.
- largest_stable_slot: self - .active_era - .epoch_start(self.ending_state.number as u64 + 2) - - nonce_stability_window(self.active_protocol.into(), genesis), + largest_stable_slot: { + let start = self + .active_era + .epoch_start(self.ending_state.number as u64 + 2); + start.saturating_sub(nonce_stability_window(self.active_protocol.into(), genesis)) + },
🧹 Nitpick comments (4)
crates/cardano/src/sweep/loading.rs (1)
47-78: Account-driven snapshot load — LGTMIterating AccountState and populating ending/active snapshots looks correct.
Consider a unit test to assert snapshot tallies (pool/drep/total stake) for mixed accounts. I can draft one if useful.
crates/cardano/src/roll/pools.rs (1)
143-153: Prefer Debug for EntityKey in tracing to avoid Display boundUse ? for account_key to avoid requiring Display.
- debug!(operator=%self.operator, account_key=%account_key, "initializing pool account"); + debug!(operator=%self.operator, account_key=?account_key, "initializing pool account");crates/cardano/src/sweep/compute.rs (1)
13-27: Consider widening ratio math to i128 to avoid intermediate overflowreserves/fees can be large; Rational64 keeps headroom tight. Using Ratio for as_ratio!/into_ratio! and casting back at the end is safer.
If you prefer to keep i64, please confirm max inputs (reserves + fees + decayed_deposits) always fit within i63 during all epochs.
Also applies to: 35-56
crates/cardano/src/sweep/mod.rs (1)
79-96: Iter ergonomics: prefer flat_map over flatten (optional)Option::into_iter().flatten() works; flat_map(|m| m.iter()) can be a tad clearer to some readers.
- ) -> impl Iterator<Item = (&AccountId, &u64)> { - self.0.get(entity_id).into_iter().flatten() + ) -> impl Iterator<Item = (&AccountId, &u64)> { + self.0 + .get(entity_id) + .into_iter() + .flat_map(|m| m.iter()) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
crates/cardano/src/model.rs(10 hunks)crates/cardano/src/pallas_extras.rs(2 hunks)crates/cardano/src/roll/epochs.rs(1 hunks)crates/cardano/src/roll/pools.rs(6 hunks)crates/cardano/src/sweep/commit.rs(2 hunks)crates/cardano/src/sweep/compute.rs(5 hunks)crates/cardano/src/sweep/loading.rs(2 hunks)crates/cardano/src/sweep/mod.rs(5 hunks)crates/cardano/src/sweep/retires.rs(1 hunks)crates/cardano/src/sweep/rewards.rs(1 hunks)crates/cardano/src/sweep/transition.rs(1 hunks)crates/core/src/batch.rs(1 hunks)crates/minibf/src/mapping.rs(1 hunks)crates/redb3/src/state/utxoset.rs(1 hunks)crates/test-vectors/src/build.rs(9 hunks)crates/testing/src/toy_domain.rs(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- crates/cardano/src/pallas_extras.rs
- crates/cardano/src/roll/epochs.rs
- crates/core/src/batch.rs
- crates/minibf/src/mapping.rs
- crates/cardano/src/sweep/retires.rs
- crates/cardano/src/sweep/rewards.rs
🧰 Additional context used
🧬 Code graph analysis (9)
crates/testing/src/toy_domain.rs (3)
crates/core/src/lib.rs (5)
state(622-622)archive(623-623)storage_config(617-617)wal(621-621)default(370-381)src/adapters.rs (6)
state(73-75)archive(77-79)storage_config(85-87)wal(69-71)wal(102-102)default(14-16)crates/cardano/src/model.rs (1)
build_schema(1004-1015)
crates/cardano/src/model.rs (1)
crates/cardano/src/sweep/mod.rs (1)
sweep(160-176)
crates/cardano/src/roll/pools.rs (4)
crates/cardano/src/roll/accounts.rs (21)
key(44-47)key(69-72)key(109-112)key(151-154)key(193-196)key(239-242)key(274-277)apply(49-52)apply(74-79)apply(114-121)apply(156-163)apply(198-205)apply(244-255)apply(279-282)undo(54-57)undo(81-84)undo(123-126)undo(165-168)undo(207-210)undo(257-262)undo(284-287)crates/cardano/src/sweep/rewards.rs (2)
key(63-66)undo(79-88)crates/core/src/state.rs (5)
from(16-21)from(25-27)from(31-33)from(58-63)from(77-79)crates/cardano/src/pallas_extras.rs (2)
cert_as_pool_registration(34-86)pool_reward_account(288-291)
crates/test-vectors/src/build.rs (1)
crates/cardano/src/sweep/commit.rs (1)
state(39-39)
crates/cardano/src/sweep/compute.rs (7)
crates/cardano/src/sweep/mod.rs (1)
sweep(160-176)crates/cardano/src/model.rs (2)
sweep(733-743)rewards(794-798)crates/core/src/lib.rs (1)
genesis(618-618)crates/cardano/src/sweep/commit.rs (1)
domain(97-97)crates/cardano/src/sweep/loading.rs (1)
domain(51-53)crates/cardano/src/eras.rs (1)
domain(201-203)crates/cardano/src/lib.rs (3)
domain(144-146)domain(164-166)domain(172-174)
crates/cardano/src/sweep/transition.rs (5)
crates/cardano/src/sweep/mod.rs (4)
sweep(160-176)visit_pool(33-40)visit_account(43-50)flush(63-65)crates/cardano/src/model.rs (11)
sweep(733-743)new(234-255)new(562-567)new(842-851)key(1113-1143)from(872-874)from(878-880)from(884-886)from(890-893)apply(1145-1175)undo(1177-1207)crates/cardano/src/roll/pools.rs (14)
new(27-33)new(126-132)key(39-42)key(73-75)key(99-101)key(138-141)apply(44-57)apply(77-81)apply(103-107)apply(143-158)undo(59-61)undo(83-87)undo(109-113)undo(160-164)crates/cardano/src/sweep/rewards.rs (7)
key(63-66)key(100-102)apply(68-77)apply(104-113)undo(79-88)undo(115-124)visit_pool(158-213)crates/cardano/src/sweep/retires.rs (13)
key(19-21)key(57-59)key(96-98)key(134-136)apply(23-32)apply(61-74)apply(100-109)undo(34-43)undo(76-85)undo(111-120)undo(153-162)visit_pool(195-220)flush(254-260)
crates/cardano/src/sweep/mod.rs (7)
crates/cardano/src/sweep/commit.rs (2)
commit(122-144)domain(97-97)crates/cardano/src/sweep/compute.rs (4)
compute(300-355)domain(308-310)domain(320-322)domain(332-334)crates/cardano/src/sweep/transition.rs (3)
visit_pool(125-137)visit_account(139-148)flush(150-156)crates/cardano/src/sweep/rewards.rs (1)
visit_pool(158-213)crates/cardano/src/sweep/retires.rs (3)
visit_pool(195-220)visit_drep(222-252)flush(254-260)crates/cardano/src/roll/epochs.rs (1)
flush(291-301)crates/cardano/src/sweep/loading.rs (1)
domain(51-53)
crates/cardano/src/sweep/commit.rs (3)
crates/cardano/src/sweep/mod.rs (1)
sweep(160-176)crates/cardano/src/sweep/loading.rs (1)
domain(51-53)crates/cardano/src/sweep/compute.rs (3)
domain(308-310)domain(320-322)domain(332-334)
crates/cardano/src/sweep/loading.rs (3)
crates/cardano/src/model.rs (1)
drep_to_entity_key(807-817)crates/cardano/src/eras.rs (1)
load_active_era(200-215)crates/core/src/batch.rs (1)
default(19-24)
⏰ 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 (26)
crates/test-vectors/src/build.rs (9)
167-169: Signature updated to dolos_redb3::state::StateStore — LGTMMatches the refactor.
262-264: Duplicate: state param type update OKSame change as in handle_account_state.
333-334: Duplicate: state param type update OKSame change as in handle_account_state.
445-446: Duplicate: state param type update OKSame change as in handle_account_state.
645-646: Duplicate: state param type update OKSame change as in handle_account_state.
813-814: Duplicate: state param type update OKSame change as in handle_account_state.
586-589: PoolState.is_retired properly defined and updated
PoolState declaresis_retired: bool(defaultfalse), andsweep/retires.rssets it totrueon retire and back tofalseon undo.
118-123: Approve code changes – no remaining references todolos_redb3::StateStorefound.
870-873: No action needed: DRepState.expired default correct
Defaultexpired: boolinitializes tofalse, and the sweep logic insweep/retires.rssets it totrueon expiration as expected.crates/testing/src/toy_domain.rs (5)
8-10: Seed helper now uses dolos_redb3::state::StateStore::in_memory — LGTM
78-84: ToyDomain storage types updated (state/archive) — LGTMMatches the new domain-oriented storage split.
93-95: State initialization path updated — LGTM
104-107: Archive initialized via dolos_redb3::archive::ArchiveStore — LGTM
141-146: No lingering old associated-type definitions detected — LGTM
Ran grep fordolos_redb3::StateStoreanddolos_redb3::archive::ChainStore; no occurrences found.crates/cardano/src/roll/pools.rs (1)
116-165: New PoolAccountDetected delta — LGTMKeying by minicbor-encoded StakeCredential matches existing account key convention; undo path is correct.
crates/redb3/src/state/utxoset.rs (1)
442-442: Approve import path alignment
No lingering old imports forStateStore; usingcrate::state::StateStoreis consistent.crates/cardano/src/sweep/loading.rs (1)
80-105: Approve code changes — no residual references to removed per-entity fieldscrates/cardano/src/sweep/compute.rs (1)
452-455: Replace todo!() with a real Domain test setup (duplicate)This test will panic. Provide a minimal test domain/mock implementing Domain used by compute().
I can sketch a minimal in-memory Domain mock for this test if helpful.
crates/cardano/src/model.rs (5)
1019-1046: LGTM: CardanoDelta variants extended coherentlyNew variants align with the delta-driven sweep design.
Please confirm external routing (apply/undo) is updated anywhere else these variants are matched over (if any).
1082-1109: LGTM: delta_from mappings are complete and consistentAll new variants are covered, avoiding construction friction.
1113-1142: LGTM: key() routing includes new variantsKey computation appears exhaustive for the expanded enum.
1145-1174: LGTM: apply() routing updatedDowncast apply paths wired for new variants.
1177-1206: LGTM: undo() routing updatedDowncast undo paths present for all new variants.
crates/cardano/src/sweep/mod.rs (3)
31-66: LGTM: BoundaryVisitor abstractionClear extensibility point; default no-ops keep visitor wiring lightweight.
149-157: LGTM: deltas batching helpersstarting_epoch_no and add_delta keep call sites clean; aligns with WorkDeltas usage.
165-168: LGTM: domain-centric compute/commit integrationSwitching to compute(domain) and commit(domain) matches the new visitor/deltas flow.
| #[n(14)] | ||
| pub is_retired: bool, | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Backwards-compat risk: adding non-optional field to CBOR struct PoolState
New field is_retired: bool may break decoding older states unless minicbor supplies a default. If minicbor doesn’t default missing fields, consider making it Option (unwrap_or(false)) or annotate with a decode-default attribute.
Also applies to: 253-254
🌐 Web query:
minicbor derive: how to provide a default when a struct field is missing during decoding (e.g., attribute to default absent fields)?
💡 Result:
Use the derive attributes provided by minicbor_derive:
-
#[cbor(default)] on a field — when the field is missing during decoding the field is set to Default::default() for its type. Field type must implement Default. [1]
-
#[cbor(skip)] on a field — that field is not encoded and is initialised with Default::default() when decoding. Field type must implement Default. [1]
-
Alternatively make the field Option; absent -> None (optional fields default to None). [2]
-
For custom logic, use #[cbor(decode_with = "")] to run your own decode function for that field. [1]
References: minicbor_derive docs (attributes: default, skip, decode_with; optional fields behavior). [1] [2]
Annotate new is_retired fields with #[cbor(default)]
Missing fields will default to false on decode; apply to the is_retired: bool definitions at lines 217–219 and 253–254.
🤖 Prompt for AI Agents
In crates/cardano/src/model.rs around lines 217–219 and 253–254, the new boolean
field is_retired must be annotated with #[cbor(default)] so missing fields
decode to false; update both is_retired: bool declarations to add the
#[cbor(default)] attribute immediately above the field (same style as other cbor
defaults) so decoding absent values yields false.
| #[n(5)] | ||
| pub expired: bool, | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Backwards-compat risk: DRepState.expired added as non-optional
Same concern as PoolState. Verify minicbor’s behavior or switch to Option/decode default to avoid breaking previously persisted states.
Also applies to: 849-850
🌐 Web query:
With minicbor's #[derive(Decode)], how to mark a field optional or give it a default when absent in the encoded map/array?
💡 Result:
Use Option for optional fields (missing → None), or annotate a field with #[cbor(default)] / #[cbor(skip)] to get Default::default() when the entry is absent.
Examples:
- Optional: field: Option — if missing during Decode it becomes None. [1]
- Default value: #[cbor(default)] field: String — if missing it is set to String::default() (same for any type implementing Default). [1][2]
- Skip encoding + default on decode: #[cbor(skip)] field: T (T: Default) — not encoded; decoded as Default::default(). [1]
For custom decoding/default logic use #[cbor(decode_with = "")] and implement your own decoder. [1]
Sources: minicbor derive docs. [1][2]
References
[1] minicbor-derive docs (attributes: default, skip, decode_with).
[2] minicbor-derive overview (Option defaults to None; behavior for maps/arrays).
Annotate new expired fields as optional or provide a default
In both DRepState (lines 837–839) and PoolState (849–850), add #[cbor(default)] to expired: bool (or change its type to Option<bool>) so missing entries decode to false (or None) and avoid breaking existing persisted data.
🤖 Prompt for AI Agents
In crates/cardano/src/model.rs around lines 837–839 (DRepState.expired) and
849–850 (PoolState.expired), the new expired: bool field will break decoding of
existing persisted data; add the serde-cbor default annotation by applying
#[cbor(default)] to both expired fields so missing entries decode to false
(alternatively change the field type to Option<bool> if you prefer explicit None
handling).
| if let Some(cert) = pallas_extras::cert_as_pool_registration(cert) { | ||
| deltas.add_for_entity(PoolRegistration::new(block.slot(), cert.clone())); | ||
|
|
||
| // Reward accounts for pool don't need to go through the standard stake | ||
| // registration process. This is why we need to track the account directly on | ||
| // pool registration. | ||
|
|
||
| let cred = pallas_extras::pool_reward_account(&cert.reward_account).unwrap(); | ||
| deltas.add_for_entity(PoolAccountDetected::new(cert.operator, cred)); | ||
| } |
There was a problem hiding this comment.
Avoid panic on invalid reward account; handle None from pool_reward_account
unwrap() can crash on malformed on-chain data. Guard and warn instead.
Apply:
- let cred = pallas_extras::pool_reward_account(&cert.reward_account).unwrap();
- deltas.add_for_entity(PoolAccountDetected::new(cert.operator, cred));
+ if let Some(cred) = pallas_extras::pool_reward_account(&cert.reward_account) {
+ deltas.add_for_entity(PoolAccountDetected::new(cert.operator, cred));
+ } else {
+ warn!(operator=%cert.operator, "invalid pool reward account; skipping PoolAccountDetected");
+ }And add warn to imports:
-use tracing::{debug, trace};
+use tracing::{debug, trace, warn};🤖 Prompt for AI Agents
In crates/cardano/src/roll/pools.rs around lines 191 to 200, the code calls
pallas_extras::pool_reward_account(...).unwrap(), which may panic on malformed
on-chain data; change this to handle the Option return value safely by matching
or using if let Some(cred) =
pallas_extras::pool_reward_account(&cert.reward_account) {
deltas.add_for_entity(PoolAccountDetected::new(cert.operator, cred)); } else {
log a warning including the problematic cert or reward_account bytes/context and
skip adding the PoolAccountDetected }, and add the warn log import (e.g., use
tracing::warn or whatever logging crate is used) to the imports.
| if !self.deltas.entities.is_empty() { | ||
| warn!(quantity = %self.deltas.entities.len(), "uncommitted deltas"); | ||
| } |
There was a problem hiding this comment.
Leftover deltas are discarded instead of applied.
After the namespace walk we can exit with entries still sitting in self.deltas.entities. Those correspond to keys that didn’t exist in the store (e.g. pool reward accounts) and never get applied—we simply log and drop them, losing state updates. We must consume and persist those deltas (or fail hard) rather than continue.
Please extend apply_whole_namespace so every remaining entry for that namespace is applied against None (creating the entity when needed) and removed from the map, or return an error if anything is left. Leaving this as a warning will corrupt ledger state.
🤖 Prompt for AI Agents
In crates/cardano/src/sweep/commit.rs around lines 131 to 133, leftover entries
in self.deltas.entities are only warned-about and dropped, which loses state;
change apply_whole_namespace so after the namespace walk you iterate over every
remaining entry for that namespace, call the existing apply logic with a None
base (so the entity is created when needed), persist the result and remove the
entry from self.deltas.entities; if any apply/persist fails return an Err
instead of continuing, ensuring no deltas are silently discarded.
| self.prev_pool = entity.latest_pool.clone(); | ||
| self.prev_drep = entity.latest_drep.clone(); | ||
| self.prev_stake = Some(entity.active_stake); |
There was a problem hiding this comment.
Undo currently restores the latest delegation instead of the prior active one.
We’re snapshotting latest_* before mutating, so undo puts the account back into the latest delegation instead of the real pre-rotation active delegation/stake. That breaks rollback correctness. Snapshot the active fields instead.
- self.prev_pool = entity.latest_pool.clone();
- self.prev_drep = entity.latest_drep.clone();
+ self.prev_pool = entity.active_pool.clone();
+ self.prev_drep = entity.active_drep.clone();
self.prev_stake = Some(entity.active_stake);🤖 Prompt for AI Agents
In crates/cardano/src/sweep/transition.rs around lines 44 to 46, the code
snapshots entity.latest_pool/latest_drep so undo restores the latest delegation
instead of the prior active one; change the snapshots to use
entity.active_pool.clone() and entity.active_drep.clone() (keep prev_stake =
Some(entity.active_stake) as-is), ensuring you clone the active fields so undo
will restore the true pre-rotation active delegation/stake.
1 participant
Summary by CodeRabbit
New Features
Refactor
Bug Fixes