fix(cardano): fix multiple accounting details - #767
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThis PR adds collateral-aware fee computation for Alonzo-era transactions (UTXO lookup), tightens reward application to require spendability and changes missing-account handling to a panic, removes two public EpochValue clearing methods, switches deregistration to explicit replace calls, and simplifies account dump output. Changes
Sequence Diagram(s)sequenceDiagram
participant DeltaBuilder as DeltaBuilder (crawler)
participant BlockVisitor as BlockVisitor::visit_tx
participant FeeLogic as define_tx_fees / compute_alonzo_collateral
participant UtxoMap as UTXO Map
DeltaBuilder->>BlockVisitor: visit_tx(block, tx, utxos)
BlockVisitor->>FeeLogic: define_tx_fees(tx, utxos)
alt tx.fee is valid
FeeLogic-->>BlockVisitor: return explicit fee
else tx.fee invalid
FeeLogic->>UtxoMap: lookup collateral inputs
alt collateral inputs found
FeeLogic-->>BlockVisitor: return sum(collateral)
else collateral inputs missing
FeeLogic->>FeeLogic: compute_alonzo_collateral(tx)
FeeLogic-->>BlockVisitor: return computed collateral fee
end
end
sequenceDiagram
participant RewardApplier as AssignRewards::apply
participant RewardMap as RewardMap::take_for_apply
participant AccountState as AccountState (lookup)
RewardApplier->>AccountState: lookup account for reward.target
alt account exists
RewardMap->>RewardMap: check registered && reward.spendable
alt both true
RewardMap-->>RewardApplier: Some(reward) (applied_effective++)
else
RewardMap-->>RewardApplier: None (applied_unspendable++)
end
else account missing
RewardApplier->>RewardApplier: panic ("existing account")
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
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 (3)
crates/cardano/src/rupd/loading.rs (1)
131-131: Fix typo in TODO comment.The comment contains "ths" which should be "this".
- // TODO: check if we really need to make ths check. It might be adding noise to the data. + // TODO: check if we really need to make this check. It might be adding noise to the data.crates/cardano/src/roll/epochs.rs (2)
217-230: Collapseelse { if .. }and keep logic identical.Minor readability fix flagged by static analysis. No behavior change.
- } else { - if let Some(collateral) = tx.total_collateral() { - Ok(collateral) - } else { - compute_alonzo_collateral(tx, utxos) - } - } + } else if let Some(collateral) = tx.total_collateral() { + Ok(collateral) + } else { + compute_alonzo_collateral(tx, utxos) + }
193-215: Use ofconsumes()for invalid Alonzo collateral is correct; apply optional style/safety improvements.The
consumes()accessor correctly returns only collateral inputs for invalid transactions in Alonzo, so the semantic logic is sound. The code is safe as-is.Consider these optional style and safety improvements:
- let mut total = 0; + let mut total: u64 = 0; for input in tx.consumes() { let utxo = utxos .get(&TxoRef::from(&input)) .ok_or(ChainError::BrokenInvariant(BrokenInvariant::MissingUtxo( TxoRef::from(&input), )))?; utxo.with_dependent(|_, utxo| { - total += utxo.value().coin(); + total = total.saturating_add(utxo.value().coin()); }); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
crates/cardano/src/estart/rewards.rs(3 hunks)crates/cardano/src/model.rs(0 hunks)crates/cardano/src/pallas_extras.rs(1 hunks)crates/cardano/src/rewards/mod.rs(1 hunks)crates/cardano/src/roll/accounts.rs(1 hunks)crates/cardano/src/roll/epochs.rs(3 hunks)crates/cardano/src/roll/mod.rs(2 hunks)crates/cardano/src/roll/txs.rs(3 hunks)crates/cardano/src/rupd/loading.rs(1 hunks)src/bin/dolos/data/dump_state.rs(1 hunks)
💤 Files with no reviewable changes (1)
- crates/cardano/src/model.rs
🧰 Additional context used
🧬 Code graph analysis (4)
crates/cardano/src/roll/mod.rs (2)
crates/cardano/src/roll/epochs.rs (1)
visit_tx(261-273)crates/cardano/src/roll/txs.rs (2)
visit_tx(100-114)tx(109-109)
crates/cardano/src/estart/rewards.rs (1)
crates/cardano/src/rewards/mod.rs (2)
total_value(38-40)total_value(131-136)
src/bin/dolos/data/dump_state.rs (2)
crates/cardano/src/rupd/loading.rs (2)
state(115-115)state(125-125)crates/cardano/src/ewrap/loading.rs (6)
state(38-38)state(77-77)state(95-95)state(113-113)state(123-123)state(133-133)
crates/cardano/src/roll/epochs.rs (2)
crates/core/src/lib.rs (8)
hash(498-498)from(77-79)from(83-85)from(89-91)from(122-124)from(128-132)from(139-141)from(145-147)crates/cardano/src/owned.rs (1)
hash(34-36)
🪛 GitHub Actions: CI
crates/cardano/src/estart/rewards.rs
[error] 3-3: unused import: pallas::ledger::primitives::StakeCredential
🪛 GitHub Check: Check Build
crates/cardano/src/estart/rewards.rs
[failure] 3-3:
unused import: pallas::ledger::primitives::StakeCredential
crates/cardano/src/roll/epochs.rs
[failure] 223-223:
this else { if .. } block can be collapsed
🔇 Additional comments (11)
crates/cardano/src/pallas_extras.rs (1)
114-114: LGTM!Adding
Debugderive improves debugging capabilities forMultiEraStakeDelegation.src/bin/dolos/data/dump_state.rs (1)
307-307: LGTM!The change to use the generic
dump_statefunction aligns with the existing pattern and reduces code duplication.crates/cardano/src/roll/accounts.rs (1)
312-316: LGTM!The explicit
replacecalls make the deregistration logic clearer by showing exactly what state is being set (NotDelegated and None respectively) instead of using genericclearmethods.crates/cardano/src/estart/rewards.rs (2)
72-74: LGTM!The enhanced warning message with the reward amount provides better debugging information.
24-25: The change is correct and theexpect()cannot panic.The verification confirms that accounts are guaranteed to exist when
AssignRewards::apply()is called because:
visit_accountiterates only over existingAccountStateentities loaded directly from the state storeAssignRewardsdeltas are created only when the reward exists (line 71 check)- During commit, the entity is initialized as
Some(entity)and no prior deltas remove or delete accounts- The
expect("existing account")is safe and appropriateThe upstream checks in the state store iteration provide sufficient guarantee that accounts exist at this stage. The change from warning + early return to
expect()is justified.crates/cardano/src/roll/mod.rs (2)
250-250: LGTM!The call site correctly passes the
utxosreference tovisit_tx.
54-62: AllBlockVisitortrait implementations properly updated.Verification confirms that both
visit_tximplementations (inTxLogVisitorandEpochStateVisitor) have been updated with the newutxosparameter.EpochStateVisitoractively uses it for collateral-aware fee computation viadefine_tx_fees(tx, utxos). Other implementations inherit the default trait implementation.crates/cardano/src/roll/txs.rs (2)
1-2: LGTM!The
HashMapimport is necessary to support the updatedvisit_txsignature.
100-114: LGTM!The
visit_txsignature has been correctly updated to match theBlockVisitortrait. The unusedutxosparameter is appropriately marked with_since this implementation doesn't require UTXO lookups.crates/cardano/src/rewards/mod.rs (1)
242-256: Disregard this review comment — code logic is correct and not a recent change.The review describes a scenario that doesn't match the current implementation. The code already checks both registration and spendability at application time, which is the correct design given Cardano's reward timing model. Registration changes take effect over a 2-4 epoch lag, so an account's registration status can differ between when rewards are earned (snapshot time) and when they're applied (2+ epochs later).
The claim about a previous "Some(None) path" doesn't match the current code—there are only
Some(reward)andNonebranches. The TODO comment already documents this architectural decision and the acknowledged uncertainty. Unspendable rewards are tracked separately and can be drained via dedicated methods, and a warning in estart/rewards.rs logs mismatches for monitoring. The current logic is stable and intentional, not a recent "tightening."Likely an incorrect or invalid review comment.
crates/cardano/src/roll/epochs.rs (1)
266-269: Allvisit_tximplementations properly updated—no further action needed.Verification confirms that all
visit_txmethod definitions have been updated with the newutxos: &HashMap<TxoRef, OwnedMultiEraOutput>parameter:
- Trait definition (mod.rs): includes parameter ✓
- TxLogVisitor (txs.rs): includes parameter ✓
- EpochStateVisitor (epochs.rs): includes parameter ✓
The breaking trait signature change has been consistently propagated across the trait definition and all implementing types.
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Breaking Changes