refactor(cardano): move delta structs alongside entities#968
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 40 minutes and 59 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR splits the monolithic Cardano model into focused modules under Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 8
🧹 Nitpick comments (9)
crates/cardano/src/model/assets.rs (1)
60-67: Extract the asset key derivation into one helper.These two
key()impls hash the same(policy, asset)tuple. A tiny shared helper here would keep future asset deltas from drifting onto different keys.Also applies to: 111-118
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/cardano/src/model/assets.rs` around lines 60 - 67, Two identical key() implementations compute a hash over (policy, asset); extract that logic into a single helper (e.g., fn derive_asset_key_bytes(policy: &[u8], asset: &[u8]) -> [u8; N] or fn derive_asset_ns_key(policy: &[u8], asset: &[u8]) -> NsKey) and call it from both key() impls to avoid drift. Move the pallas::crypto::hash::Hasher::<256>::new(), hasher.input(...) and hasher.finalize() code into the helper, return either the raw key bytes or an NsKey constructed with (AssetState::NS, key), and update the two key() methods to delegate to that helper (referencing key(), AssetState::NS, and the new helper name).crates/cardano/src/model/proposals.rs (1)
113-129: Minor: Typo in doc comment.Line 113: "beign" should be "being" in the doc comment "if the proposal is still beign evaluated".
📝 Fix typo
- /// Returns true if the proposal is still beign evaluated. Not to confuse with `is_enacted`. + /// Returns true if the proposal is still being evaluated. Not to confuse with `is_enacted`.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/cardano/src/model/proposals.rs` around lines 113 - 129, Fix the typo in the is_active doc comment: change "beign" to "being" in the comment above the pub fn is_active(&self, current_epoch: Epoch) -> bool to read "Returns true if the proposal is still being evaluated. Not to confuse with `is_enacted`." This is purely a documentation spelling correction inside the proposals.rs comment for is_active.crates/cardano/src/model/epochs.rs (1)
445-497: EpochTransition relies on genesis being set.The
apply()method at line 489 usesself.genesis.as_ref().expect("genesis not set")when migrating pparams during an era transition. This will panic ifgenesisisNonebutera_transitionisSome.Since
genesisis marked with#[serde(skip)](line 451), it won't survive serialization/deserialization cycles. Ensure that code paths that deserializeEpochTransitionalways re-populate thegenesisfield before callingapply().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/cardano/src/model/epochs.rs` around lines 445 - 497, EpochTransition::apply() assumes genesis is present when era_transition is Some but genesis is #[serde(skip)] and may be None after deserialization; add a small safety and population flow: implement a setter like EpochTransition::set_genesis(&mut self, genesis: Arc<Genesis>) and ensure all code paths that deserialize or construct EpochTransition call that setter before invoking apply(), and also add a defensive debug_assert!(self.genesis.is_some() || self.era_transition.is_none(), "genesis must be populated for era transition") at the top of apply() to catch missed population earlier (avoid changing apply() signature).crates/cardano/src/model/accounts.rs (2)
54-59: Consider extracting the Conway protocol version as a named constant.The magic number
9for the Conway protocol version threshold could be extracted as a named constant (similar toSHELLEY_PROTOCOLinpparams.rs) for better maintainability.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/cardano/src/model/accounts.rs` around lines 54 - 59, Extract the magic number 9 into a named constant (e.g., CONWAY_PROTOCOL or CONWAY_PROTOCOL_VERSION) and use it in the comparison inside total_for_era; update the match arm in fn total_for_era(&self, era: EraProtocol) to compare against that constant instead of the literal 9, keeping existing calls to total_pre_conway() and total() unchanged so behavior remains identical.
694-697: Inconsistent overflow handling:+=used instead ofadd!macro.
PoolDepositRefundandProposalDepositRefunduse direct+=forrewards_sum, while other similar operations (TreasuryWithdrawal,AssignRewards) use theadd!macro for overflow protection. Consider usingadd!for consistency.♻️ Proposed fix for consistency
fn apply(&mut self, entity: &mut Option<Self::Entity>) { let entity = entity.as_mut().expect("existing account"); let stake = entity.stake.scheduled_or_default(); - stake.rewards_sum += self.pool_deposit; + stake.rewards_sum = add!(stake.rewards_sum, self.pool_deposit); }And similarly for
ProposalDepositRefund:fn apply(&mut self, entity: &mut Option<Self::Entity>) { let entity = entity.as_mut().expect("existing account"); tracing::debug!(cred=?self.account, deposit=%self.proposal_deposit, "applying proposal deposit refund"); let stake = entity.stake.scheduled_or_default(); - stake.rewards_sum += self.proposal_deposit; + stake.rewards_sum = add!(stake.rewards_sum, self.proposal_deposit); }Also applies to: 732-735
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/cardano/src/model/accounts.rs` around lines 694 - 697, The code inconsistently uses direct addition for rewards_sum (stake.rewards_sum += self.pool_deposit) in the PoolDepositRefund and ProposalDepositRefund handling; change these to use the existing add! overflow-safe macro (e.g., call add!(stake.rewards_sum, self.pool_deposit)) to match TreasuryWithdrawal/AssignRewards behavior and prevent overflow; update both occurrences where stake.rewards_sum is incremented in the PoolDepositRefund and ProposalDepositRefund code paths.crates/cardano/src/model/pending.rs (2)
222-226: Potential overflow inEnqueueMir::applyaccumulation path.The Alonzo+ accumulation path uses direct
+=which could overflow. Consider usingsaturating_addor theadd!macro.♻️ Proposed fix
} else if let Some(existing) = entity.as_mut() { // Alonzo+: accumulate - existing.from_reserves += self.from_reserves; - existing.from_treasury += self.from_treasury; + existing.from_reserves = existing.from_reserves.saturating_add(self.from_reserves); + existing.from_treasury = existing.from_treasury.saturating_add(self.from_treasury); } else {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/cardano/src/model/pending.rs` around lines 222 - 226, In EnqueueMir::apply, the Alonzo+ accumulation branch mutates existing.from_reserves and existing.from_treasury with direct += which can overflow; replace those uses of direct addition in the entity.as_mut() branch (where existing is modified) with saturating_add (or the project-wide add! macro) to safely accumulate values—update the lines that set existing.from_reserves and existing.from_treasury to use existing.from_reserves = existing.from_reserves.saturating_add(self.from_reserves) and similarly for from_treasury (or the equivalent add! invocation) so accumulation cannot overflow.
72-76: Potential overflow inPendingMirState::total_value().Direct addition without overflow checking could overflow for extremely large values. Consider using
saturating_addor anadd!macro for consistency with other parts of the codebase.♻️ Proposed fix
impl PendingMirState { pub fn total_value(&self) -> u64 { - self.from_reserves + self.from_treasury + self.from_reserves.saturating_add(self.from_treasury) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/cardano/src/model/pending.rs` around lines 72 - 76, PendingMirState::total_value currently adds self.from_reserves + self.from_treasury with plain + which can overflow; change the implementation to use checked/saturating addition (e.g., self.from_reserves.saturating_add(self.from_treasury)) so the sum cannot wrap, updating the PendingMirState::total_value method to return a saturated u64 instead of relying on unchecked addition.crates/cardano/src/model/mod.rs (1)
7-26: Minor: Lifetime elision in macro differs from trait definition.Line 10 uses
const NS: &strbut the trait at line 4 definesconst NS: &'static str. While Rust infers the static lifetime for constants, explicitly matching the trait definition improves clarity.♻️ Proposed fix for consistency
macro_rules! entity_boilerplate { ($type:ident, $ns:literal) => { impl super::FixedNamespace for $type { - const NS: &str = $ns; + const NS: &'static str = $ns; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/cardano/src/model/mod.rs` around lines 7 - 26, The impl generated by macro entity_boilerplate! declares the associated constant as `const NS: &str` which differs from the trait FixedNamespace signature; update the impl to use the exact type `const NS: &'static str` so it matches the FixedNamespace trait definition (adjust the occurrence in the macro where you implement super::FixedNamespace for $type and the NS constant declaration).crates/cardano/src/model/dreps.rs (1)
89-91:prev_depositundo field is never populated duringapply.The
prev_depositfield (line 90) exists for undo purposes, butapply()never saves the previous deposit value. If undo is implemented later, this data will be missing. Consider addingself.prev_deposit = Some(entity.deposit);before applying changes (similar to howDRepUnRegistrationsaves its undo data).♻️ Proposed fix to save undo data
fn apply(&mut self, entity: &mut Option<DRepState>) { let entity = entity.get_or_insert_with(|| DRepState::new(self.drep.clone())); + // save undo info + self.prev_deposit = Some(entity.deposit); + // apply changes entity.registered_at = Some((self.slot, self.txorder)); entity.voting_power = self.deposit; entity.deposit = self.deposit; }Also applies to: 119-126
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/cardano/src/model/dreps.rs` around lines 89 - 91, The prev_deposit undo field is never set before mutating the entity; in the apply() method of the struct that owns prev_deposit (e.g., DRepRegistration) assign self.prev_deposit = Some(entity.deposit) immediately before updating entity.deposit so the original value is preserved for undo; apply the same fix to the other apply() implementation mentioned (the similar struct around lines 119-126) so each place that mutates deposit first saves the prior deposit into prev_deposit (mirroring how DRepUnRegistration records undo data).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/cardano/src/ewrap/enactment.rs`:
- Around line 5-6: The enactment rollbacks currently leave enacted
protocol-parameter changes and treasury credits in place because
PParamsUpdate::undo() and TreasuryWithdrawal::undo() are stubs; implement these
undo methods to revert the applied changes: in PParamsUpdate::undo(), remove the
scheduled PParamsUpdate from any schedule structures and restore the previous
PParams values in whatever PParamsSet/AccountState field was mutated (ensure you
locate and update the same storage used by the enact path through
epochs.rs::(362-387)), and in TreasuryWithdrawal::undo(), subtract the
previously credited amount from the beneficiary's AccountState and restore the
treasury balance (match the accounting done in accounts.rs::(635-667)). Ensure
the undo implementations mirror the apply/enact logic exactly so rollbacks fully
revert state and maintain invariants.
In `@crates/cardano/src/model/accounts.rs`:
- Around line 581-584: In PoolDelegatorRetire::apply, avoid the panic-prone
unreachable! when destructuring self.prev_pool; replace it with a safe match or
use expect with a descriptive message referencing the expected
PoolDelegation::Pool variant so non-delegated cases (e.g., NotDelegated) are
handled explicitly; update the code around the let
Some(PoolDelegation::Pool(pool)) = self.prev_pool pattern so entity.retired_pool
is set only when a pool exists and otherwise return/handle the error condition
with a clear message.
In `@crates/cardano/src/model/assets.rs`:
- Around line 69-84: The undo methods currently no-op leaving persisted
AssetState mutated; implement undo to reverse what apply does: in the Asset
mint/asset-related delta impls (the apply/undo pair that mutates AssetState)
subtract self.quantity from AssetState.quantity_bytes, decrement mint_tx_count,
and restore initial_slot/initial_tx to their previous values (or None) instead
of leaving the first-mint markers—use any captured previous fields on the delta
(e.g., prev_initial_slot/prev_initial_tx or boolean flags) to restore state and
clear is_first_mint; likewise, for MetadataTxUpdate implement undo to set
AssetState.metadata_tx back to prev_metadata_tx (which is already captured).
Ensure undo checks for entity being Some and handles underflow (don’t decrement
below zero) and Option presence when restoring fields so rollback leaves
AssetState exactly as it was before apply.
In `@crates/cardano/src/model/dreps.rs`:
- Around line 241-242: The panic message in fn apply(&mut self, entity: &mut
Option<Self::Entity>) incorrectly says "existing account"; update the expect()
message to accurately reference the DRep entity (e.g., "existing drep" or
"existing DRepState") so the error reflects the actual type being unwrapped in
apply for DRepState.
In `@crates/cardano/src/model/eras.rs`:
- Around line 52-56: In the impl From<EntityKey> for EraProtocol (the from
function that calls value.as_ref()[..2].try_into().unwrap()), remove the unwrap
by extracting the two bytes deterministically — e.g. create a [u8;2] and
copy_from_slice(&value.as_ref()[..2]) or assign via [bytes0, bytes1] using
value.as_ref()[0] and [1] — and add a short comment stating EntityKey is always
32 bytes so the 2-byte slice is guaranteed; then use u16::from_be_bytes on that
array instead of relying on try_into().unwrap().
In `@crates/cardano/src/model/pparams.rs`:
- Around line 276-291: The era_transition method on EpochValue<PParamsSet>
currently calls unwrap_live(), which can panic; change it to access the live
value safely (e.g., use self.live() or equivalent) and return None if the live
value is missing. Specifically, in era_transition(), replace the
unwrap_live().protocol_major_or_default() call with an Option-aware sequence:
get the live PParamsSet via self.live(), map or and_then to obtain
protocol_major_or_default (or protocol_major if appropriate), and early-return
None when live is None; keep the existing use of self.next().and_then(|p|
p.protocol_major()) and the EraTransition construction using EraProtocol::from
only when both original and update are present and different.
In `@crates/cardano/src/roll/assets.rs`:
- Around line 9-10: The MetadataTxUpdate currently records prev_metadata_tx
inside its apply() but leaves undo() as a no-op, so asset.metadata_tx isn't
restored on reorg; modify MetadataTxUpdate (in
crates/cardano/src/model/assets.rs) so apply() saves the previous
AssetState.metadata_tx into a field (e.g., prev_metadata_tx) and undo() restores
AssetState.metadata_tx from that saved field, ensuring the shared-delta routing
still uses MetadataTxUpdate while preserving rollback behavior for CIP-25/CIP-68
metadata writes.
In `@crates/cardano/src/roll/datums.rs`:
- Around line 24-25: The rollback bug is that DatumRefIncrement::undo() and
DatumRefDecrement::undo() are currently no-ops despite apply() saving
was_new/prev_state, so update the undo implementations to reverse what apply()
did: use the stored prev_state and was_new to restore the previous datum row and
refcount (resurrect a deleted row when prev_state indicates it existed, or
remove a newly-created row when was_new is true), and adjust the shared refcount
appropriately; locate and modify DatumRefIncrement::undo,
DatumRefDecrement::undo, and ensure the logic mirrors apply() (using the saved
was_new/prev_state fields) in the visitor in crates/cardano/src/roll/datums.rs
so rollbacks correctly restore/delete rows and decrement/increment refcounts.
---
Nitpick comments:
In `@crates/cardano/src/model/accounts.rs`:
- Around line 54-59: Extract the magic number 9 into a named constant (e.g.,
CONWAY_PROTOCOL or CONWAY_PROTOCOL_VERSION) and use it in the comparison inside
total_for_era; update the match arm in fn total_for_era(&self, era: EraProtocol)
to compare against that constant instead of the literal 9, keeping existing
calls to total_pre_conway() and total() unchanged so behavior remains identical.
- Around line 694-697: The code inconsistently uses direct addition for
rewards_sum (stake.rewards_sum += self.pool_deposit) in the PoolDepositRefund
and ProposalDepositRefund handling; change these to use the existing add!
overflow-safe macro (e.g., call add!(stake.rewards_sum, self.pool_deposit)) to
match TreasuryWithdrawal/AssignRewards behavior and prevent overflow; update
both occurrences where stake.rewards_sum is incremented in the PoolDepositRefund
and ProposalDepositRefund code paths.
In `@crates/cardano/src/model/assets.rs`:
- Around line 60-67: Two identical key() implementations compute a hash over
(policy, asset); extract that logic into a single helper (e.g., fn
derive_asset_key_bytes(policy: &[u8], asset: &[u8]) -> [u8; N] or fn
derive_asset_ns_key(policy: &[u8], asset: &[u8]) -> NsKey) and call it from both
key() impls to avoid drift. Move the pallas::crypto::hash::Hasher::<256>::new(),
hasher.input(...) and hasher.finalize() code into the helper, return either the
raw key bytes or an NsKey constructed with (AssetState::NS, key), and update the
two key() methods to delegate to that helper (referencing key(), AssetState::NS,
and the new helper name).
In `@crates/cardano/src/model/dreps.rs`:
- Around line 89-91: The prev_deposit undo field is never set before mutating
the entity; in the apply() method of the struct that owns prev_deposit (e.g.,
DRepRegistration) assign self.prev_deposit = Some(entity.deposit) immediately
before updating entity.deposit so the original value is preserved for undo;
apply the same fix to the other apply() implementation mentioned (the similar
struct around lines 119-126) so each place that mutates deposit first saves the
prior deposit into prev_deposit (mirroring how DRepUnRegistration records undo
data).
In `@crates/cardano/src/model/epochs.rs`:
- Around line 445-497: EpochTransition::apply() assumes genesis is present when
era_transition is Some but genesis is #[serde(skip)] and may be None after
deserialization; add a small safety and population flow: implement a setter like
EpochTransition::set_genesis(&mut self, genesis: Arc<Genesis>) and ensure all
code paths that deserialize or construct EpochTransition call that setter before
invoking apply(), and also add a defensive debug_assert!(self.genesis.is_some()
|| self.era_transition.is_none(), "genesis must be populated for era
transition") at the top of apply() to catch missed population earlier (avoid
changing apply() signature).
In `@crates/cardano/src/model/mod.rs`:
- Around line 7-26: The impl generated by macro entity_boilerplate! declares the
associated constant as `const NS: &str` which differs from the trait
FixedNamespace signature; update the impl to use the exact type `const NS:
&'static str` so it matches the FixedNamespace trait definition (adjust the
occurrence in the macro where you implement super::FixedNamespace for $type and
the NS constant declaration).
In `@crates/cardano/src/model/pending.rs`:
- Around line 222-226: In EnqueueMir::apply, the Alonzo+ accumulation branch
mutates existing.from_reserves and existing.from_treasury with direct += which
can overflow; replace those uses of direct addition in the entity.as_mut()
branch (where existing is modified) with saturating_add (or the project-wide
add! macro) to safely accumulate values—update the lines that set
existing.from_reserves and existing.from_treasury to use existing.from_reserves
= existing.from_reserves.saturating_add(self.from_reserves) and similarly for
from_treasury (or the equivalent add! invocation) so accumulation cannot
overflow.
- Around line 72-76: PendingMirState::total_value currently adds
self.from_reserves + self.from_treasury with plain + which can overflow; change
the implementation to use checked/saturating addition (e.g.,
self.from_reserves.saturating_add(self.from_treasury)) so the sum cannot wrap,
updating the PendingMirState::total_value method to return a saturated u64
instead of relying on unchecked addition.
In `@crates/cardano/src/model/proposals.rs`:
- Around line 113-129: Fix the typo in the is_active doc comment: change "beign"
to "being" in the comment above the pub fn is_active(&self, current_epoch:
Epoch) -> bool to read "Returns true if the proposal is still being evaluated.
Not to confuse with `is_enacted`." This is purely a documentation spelling
correction inside the proposals.rs comment for is_active.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1056b007-fb2b-4750-9a8d-41fc848d3c5c
📒 Files selected for processing (30)
crates/cardano/src/estart/nonces.rscrates/cardano/src/estart/reset.rscrates/cardano/src/ewrap/drops.rscrates/cardano/src/ewrap/enactment.rscrates/cardano/src/ewrap/loading.rscrates/cardano/src/ewrap/refunds.rscrates/cardano/src/ewrap/rewards.rscrates/cardano/src/ewrap/wrapup.rscrates/cardano/src/model.rscrates/cardano/src/model/accounts.rscrates/cardano/src/model/assets.rscrates/cardano/src/model/datums.rscrates/cardano/src/model/dreps.rscrates/cardano/src/model/epoch_value.rscrates/cardano/src/model/epochs.rscrates/cardano/src/model/eras.rscrates/cardano/src/model/logs.rscrates/cardano/src/model/mod.rscrates/cardano/src/model/pending.rscrates/cardano/src/model/pools.rscrates/cardano/src/model/pparams.rscrates/cardano/src/model/proposals.rscrates/cardano/src/roll/accounts.rscrates/cardano/src/roll/assets.rscrates/cardano/src/roll/datums.rscrates/cardano/src/roll/dreps.rscrates/cardano/src/roll/epochs.rscrates/cardano/src/roll/pools.rscrates/cardano/src/roll/proposals.rscrates/cardano/src/rupd/deltas.rs
💤 Files with no reviewable changes (1)
- crates/cardano/src/model.rs
| AccountState, CardanoDelta, CardanoEntity, PParamValue, PParamsSet, PParamsUpdate, | ||
| ProposalAction, ProposalState, TreasuryWithdrawal, |
There was a problem hiding this comment.
The shared enactment deltas no longer undo applied state.
This import change routes proposal enactment through crates/cardano/src/model/epochs.rs:362-387 and crates/cardano/src/model/accounts.rs:635-667, but both PParamsUpdate::undo() and TreasuryWithdrawal::undo() are placeholders. On rollback, enacted protocol-parameter changes will stay scheduled and treasury withdrawals will stay credited to the account state.
Also applies to: 33-44
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/cardano/src/ewrap/enactment.rs` around lines 5 - 6, The enactment
rollbacks currently leave enacted protocol-parameter changes and treasury
credits in place because PParamsUpdate::undo() and TreasuryWithdrawal::undo()
are stubs; implement these undo methods to revert the applied changes: in
PParamsUpdate::undo(), remove the scheduled PParamsUpdate from any schedule
structures and restore the previous PParams values in whatever
PParamsSet/AccountState field was mutated (ensure you locate and update the same
storage used by the enact path through epochs.rs::(362-387)), and in
TreasuryWithdrawal::undo(), subtract the previously credited amount from the
beneficiary's AccountState and restore the treasury balance (match the
accounting done in accounts.rs::(635-667)). Ensure the undo implementations
mirror the apply/enact logic exactly so rollbacks fully revert state and
maintain invariants.
| let Some(PoolDelegation::Pool(pool)) = self.prev_pool else { | ||
| unreachable!("account delegated to pool") | ||
| }; | ||
| entity.retired_pool = Some(pool); |
There was a problem hiding this comment.
unreachable! will panic if called on non-delegated account.
If PoolDelegatorRetire::apply is called on an account that isn't delegated to a pool (e.g., NotDelegated), the unreachable! macro will panic. Consider using expect with a descriptive message or handling the case more gracefully if this is a potential runtime scenario.
💡 Alternative: Use expect for clearer error
- let Some(PoolDelegation::Pool(pool)) = self.prev_pool else {
- unreachable!("account delegated to pool")
- };
+ let Some(PoolDelegation::Pool(pool)) = self.prev_pool else {
+ panic!("PoolDelegatorRetire called on account not delegated to a pool: {:?}", self.delegator)
+ };
entity.retired_pool = Some(pool);📝 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 Some(PoolDelegation::Pool(pool)) = self.prev_pool else { | |
| unreachable!("account delegated to pool") | |
| }; | |
| entity.retired_pool = Some(pool); | |
| let Some(PoolDelegation::Pool(pool)) = self.prev_pool else { | |
| panic!("PoolDelegatorRetire called on account not delegated to a pool: {:?}", self.delegator) | |
| }; | |
| entity.retired_pool = Some(pool); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/cardano/src/model/accounts.rs` around lines 581 - 584, In
PoolDelegatorRetire::apply, avoid the panic-prone unreachable! when
destructuring self.prev_pool; replace it with a safe match or use expect with a
descriptive message referencing the expected PoolDelegation::Pool variant so
non-delegated cases (e.g., NotDelegated) are handled explicitly; update the code
around the let Some(PoolDelegation::Pool(pool)) = self.prev_pool pattern so
entity.retired_pool is set only when a pool exists and otherwise return/handle
the error condition with a clear message.
| fn apply(&mut self, entity: &mut Option<AssetState>) { | ||
| let entity = entity.get_or_insert_default(); | ||
|
|
||
| entity.add_quantity(self.quantity); | ||
| entity.mint_tx_count += 1; | ||
|
|
||
| if entity.initial_slot.unwrap_or(u64::MAX) > self.seen_in_slot { | ||
| entity.initial_slot = Some(self.seen_in_slot); | ||
| entity.initial_tx = Some(self.seen_in_tx); | ||
| self.is_first_mint = Some(true); | ||
| } | ||
| } | ||
|
|
||
| fn undo(&self, _entity: &mut Option<AssetState>) { | ||
| // no-op: undo not yet comprehensively implemented | ||
| } |
There was a problem hiding this comment.
Implement rollback for these asset deltas.
Both apply methods mutate persisted AssetState, but Lines 82-84 and 126-128 leave undo as a no-op. On a rollback, quantity_bytes, mint_tx_count, initial_*, and metadata_tx will remain mutated even after the source block is removed. MetadataTxUpdate already captures prev_metadata_tx, so this looks unfinished rather than intentional.
Also applies to: 120-128
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/cardano/src/model/assets.rs` around lines 69 - 84, The undo methods
currently no-op leaving persisted AssetState mutated; implement undo to reverse
what apply does: in the Asset mint/asset-related delta impls (the apply/undo
pair that mutates AssetState) subtract self.quantity from
AssetState.quantity_bytes, decrement mint_tx_count, and restore
initial_slot/initial_tx to their previous values (or None) instead of leaving
the first-mint markers—use any captured previous fields on the delta (e.g.,
prev_initial_slot/prev_initial_tx or boolean flags) to restore state and clear
is_first_mint; likewise, for MetadataTxUpdate implement undo to set
AssetState.metadata_tx back to prev_metadata_tx (which is already captured).
Ensure undo checks for entity being Some and handles underflow (don’t decrement
below zero) and Option presence when restoring fields so rollback leaves
AssetState exactly as it was before apply.
| impl From<EntityKey> for EraProtocol { | ||
| fn from(value: EntityKey) -> Self { | ||
| let bytes: [u8; 2] = value.as_ref()[..2].try_into().unwrap(); | ||
| Self(u16::from_be_bytes(bytes)) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect the new EntityKey -> EraProtocol conversion and any explicit call sites.
# Expected result: the conversion should validate exact key length instead of panicking/truncating.
sed -n '46,57p' crates/cardano/src/model/eras.rs
rg -n -C2 'EraProtocol::from\(' crates/cardano/srcRepository: txpipe/dolos
Length of output: 3132
🏁 Script executed:
rg -n "struct EntityKey|type EntityKey|pub EntityKey" --type rsRepository: txpipe/dolos
Length of output: 82
🏁 Script executed:
# Find EntityKey definition and check for size constraints
rg -B5 -A10 "pub struct EntityKey" --type rsRepository: txpipe/dolos
Length of output: 82
🏁 Script executed:
# Check if there are existing TryFrom implementations in the codebase for similar conversions
rg -n "impl TryFrom" crates/cardano/src --type rs | head -20Repository: txpipe/dolos
Length of output: 82
🏁 Script executed:
# Check if EntityKey has any length assertions or invariants
rg -n "EntityKey" crates/cardano/src/model/eras.rs -B2 -A2Repository: txpipe/dolos
Length of output: 576
🏁 Script executed:
# Find where EntityKey is defined
fd -name "*.rs" | xargs grep -l "pub struct EntityKey\|pub type EntityKey" | head -5Repository: txpipe/dolos
Length of output: 286
🏁 Script executed:
# Check if EntityKey is in dolos_core directory
find . -name "*.rs" -path "*/dolos_core*" | head -10Repository: txpipe/dolos
Length of output: 38
🏁 Script executed:
# Look for EntityKey implementation details
rg "impl.*EntityKey" -A5 | head -40Repository: txpipe/dolos
Length of output: 2035
🏁 Script executed:
# Check the as_ref() method return type to understand EntityKey storage
rg "EntityKey::from\(" -B2 -A2 | head -50Repository: txpipe/dolos
Length of output: 3074
🏁 Script executed:
# Find KEY_SIZE constant definition
rg "KEY_SIZE" crates/core/src/state.rs -B2 -A2Repository: txpipe/dolos
Length of output: 884
🏁 Script executed:
# Check the EntityKey struct definition
rg "pub struct EntityKey" crates/core/src/state.rs -A5Repository: txpipe/dolos
Length of output: 247
🏁 Script executed:
# Run clippy to see if this code triggers any warnings
cd crates/cardano && cargo clippy --all-targets --all-features 2>&1 | grep -A5 -B5 "EraProtocol\|unwrap"Repository: txpipe/dolos
Length of output: 38
🏁 Script executed:
# Try building with all features
cd crates/cardano && cargo build --all-targets --all-features 2>&1 | head -100Repository: txpipe/dolos
Length of output: 302
🏁 Script executed:
# Check if there are any warnings specifically from eras.rs
rg "unwrap" crates/cardano/src/model/eras.rs -n -B2 -A2Repository: txpipe/dolos
Length of output: 265
🏁 Script executed:
# Look at the broader context of how EraProtocol is used - is 2-byte truncation intentional?
rg "EraProtocol" crates/cardano/src --type-list | head -20Repository: txpipe/dolos
Length of output: 445
🏁 Script executed:
# Check if there are comments explaining the key structure
rg "era.*key\|key.*era" crates/cardano/src -i -B2 -A2 | head -50Repository: txpipe/dolos
Length of output: 38
🏁 Script executed:
# Check EraProtocol definition and usage
rg "enum EraProtocol|struct EraProtocol" crates/cardano/src -B2 -A5Repository: txpipe/dolos
Length of output: 521
🏁 Script executed:
# Look at the reverse conversion to understand the design intent
rg "impl From<EraProtocol> for EntityKey" crates/cardano/src -A10Repository: txpipe/dolos
Length of output: 751
🏁 Script executed:
# Check how era keys are created/stored in the database
rg "era.*key\|EntityKey.*era" crates/cardano/src -i | head -20Repository: txpipe/dolos
Length of output: 38
🏁 Script executed:
# Verify if try_into() on exact 2-byte slice can actually fail
python3 << 'EOF'
# Check if converting &[u8] of length 2 to [u8; 2] can fail
# In Rust: impl<const N: usize> TryFrom<&[u8]> for [u8; N]
# This returns Err if lengths don't match
# Since we explicitly slice [..2], the length is always 2
# So try_into() will always succeed
print("Conclusion: try_into() on [..2] slice to [u8; 2] will ALWAYS succeed")
print("The unwrap() is safe but represents poor error handling style")
EOFRepository: txpipe/dolos
Length of output: 186
Remove unnecessary unwrap() and clarify the 2-byte extraction.
Line 54 uses unwrap() on a try_into() that always succeeds because [..2] extracts exactly 2 bytes. While safe, this pattern is poor style. Consider removing the unwrap() with a direct array assignment or adding a comment explaining that the slice is guaranteed to be exactly 2 bytes. Note: EntityKey is always 32 bytes due to its padding implementation, so there is no risk of a short key panicking.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/cardano/src/model/eras.rs` around lines 52 - 56, In the impl
From<EntityKey> for EraProtocol (the from function that calls
value.as_ref()[..2].try_into().unwrap()), remove the unwrap by extracting the
two bytes deterministically — e.g. create a [u8;2] and
copy_from_slice(&value.as_ref()[..2]) or assign via [bytes0, bytes1] using
value.as_ref()[0] and [1] — and add a short comment stating EntityKey is always
32 bytes so the 2-byte slice is guaranteed; then use u16::from_be_bytes on that
array instead of relying on try_into().unwrap().
| impl EpochValue<PParamsSet> { | ||
| pub fn era_transition(&self) -> Option<EraTransition> { | ||
| let original = self.unwrap_live().protocol_major_or_default(); | ||
|
|
||
| let update = self.next().and_then(|p| p.protocol_major())?; | ||
|
|
||
| if original == update { | ||
| return None; | ||
| } | ||
|
|
||
| Some(EraTransition { | ||
| prev_version: EraProtocol::from(original), | ||
| new_version: EraProtocol::from(update), | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how unwrap_live is implemented and if it can panic
ast-grep --pattern 'fn unwrap_live($$$) {
$$$
}'Repository: txpipe/dolos
Length of output: 38
🏁 Script executed:
rg "unwrap_live" crates/cardano/src/model/pparams.rs -B 5 -A 5Repository: txpipe/dolos
Length of output: 352
🏁 Script executed:
rg "fn unwrap_live" --type rs -B 2 -A 10Repository: txpipe/dolos
Length of output: 82
🏁 Script executed:
rg "impl.*EpochValue" --type rs -B 2 -A 30Repository: txpipe/dolos
Length of output: 82
🏁 Script executed:
rg "fn unwrap_live" -B 2 -A 10Repository: txpipe/dolos
Length of output: 1111
🏁 Script executed:
fd "epoch" --type f -e rsRepository: txpipe/dolos
Length of output: 347
🏁 Script executed:
cd crates/cardano && cargo clippy --all-targets --all-features 2>&1 | head -100Repository: txpipe/dolos
Length of output: 302
🏁 Script executed:
rg "era_transition" -B 5 -A 5Repository: txpipe/dolos
Length of output: 7633
🏁 Script executed:
cat -n crates/cardano/src/model/epoch_value.rs | head -50Repository: txpipe/dolos
Length of output: 1614
🏁 Script executed:
cat -n crates/cardano/src/model/epoch_value.rs | sed -n '1,100p'Repository: txpipe/dolos
Length of output: 3361
🏁 Script executed:
rg "ended_state" crates/cardano/src/estart/ -A 2 | head -30Repository: txpipe/dolos
Length of output: 1921
Replace unwrap_live() call with safe handling to avoid panic on uninitialized live value.
On line 278, era_transition() calls unwrap_live() which panics via .expect() if the live PParamsSet is None. Since the method returns Option<EraTransition>, it should handle missing live values gracefully rather than panic. Consider using self.live() if available and wrapping the logic in an Option, or return None early if live is uninitialized.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/cardano/src/model/pparams.rs` around lines 276 - 291, The
era_transition method on EpochValue<PParamsSet> currently calls unwrap_live(),
which can panic; change it to access the live value safely (e.g., use
self.live() or equivalent) and return None if the live value is missing.
Specifically, in era_transition(), replace the
unwrap_live().protocol_major_or_default() call with an Option-aware sequence:
get the live PParamsSet via self.live(), map or and_then to obtain
protocol_major_or_default (or protocol_major if appropriate), and early-return
None when live is None; keep the existing use of self.next().and_then(|p|
p.protocol_major()) and the EraTransition construction using EraProtocol::from
only when both original and update are present and different.
| use crate::{MetadataTxUpdate, MintStatsUpdate}; | ||
|
|
There was a problem hiding this comment.
Restore rollback behavior before routing metadata updates through the shared delta.
After this import switch, these call sites now rely on crates/cardano/src/model/assets.rs:88-125, where MetadataTxUpdate records prev_metadata_tx in apply() but still leaves undo() as a no-op. A reorg that reverts a CIP-25/CIP-68 metadata write will keep AssetState.metadata_tx pointed at the reverted transaction instead of restoring the previous value.
Possible fix in crates/cardano/src/model/assets.rs
impl dolos_core::EntityDelta for MetadataTxUpdate {
type Entity = AssetState;
@@
- fn undo(&self, _entity: &mut Option<AssetState>) {
- // no-op: undo not yet comprehensively implemented
+ fn undo(&self, entity: &mut Option<AssetState>) {
+ if let Some(entity) = entity {
+ entity.metadata_tx = self.prev_metadata_tx;
+ }
}
}Also applies to: 35-42, 47-51
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/cardano/src/roll/assets.rs` around lines 9 - 10, The MetadataTxUpdate
currently records prev_metadata_tx inside its apply() but leaves undo() as a
no-op, so asset.metadata_tx isn't restored on reorg; modify MetadataTxUpdate (in
crates/cardano/src/model/assets.rs) so apply() saves the previous
AssetState.metadata_tx into a field (e.g., prev_metadata_tx) and undo() restores
AssetState.metadata_tx from that saved field, ensuring the shared-delta routing
still uses MetadataTxUpdate while preserving rollback behavior for CIP-25/CIP-68
metadata writes.
| use crate::{DatumRefDecrement, DatumRefIncrement}; | ||
|
|
There was a problem hiding this comment.
The shared datum deltas currently break rollback semantics.
crates/cardano/src/model/datums.rs:40-130 makes both DatumRefIncrement::undo() and DatumRefDecrement::undo() no-ops, even though apply() stores was_new/prev_state specifically for reversal. After switching this visitor to those shared deltas, rolling back a block with datum-hash outputs or inputs will leak refcounts and fail to resurrect deleted datum rows.
Possible fix in crates/cardano/src/model/datums.rs
impl dolos_core::EntityDelta for DatumRefIncrement {
@@
- fn undo(&self, _entity: &mut Option<DatumState>) {
- // no-op: undo not yet comprehensively implemented
+ fn undo(&self, entity: &mut Option<DatumState>) {
+ if self.was_new {
+ *entity = None;
+ } else if let Some(state) = entity {
+ state.refcount = state.refcount.saturating_sub(1);
+ }
}
}
@@
impl dolos_core::EntityDelta for DatumRefDecrement {
@@
- fn undo(&self, _entity: &mut Option<DatumState>) {
- // no-op: undo not yet comprehensively implemented
+ fn undo(&self, entity: &mut Option<DatumState>) {
+ *entity = self.prev_state.clone();
}
}Also applies to: 67-87
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/cardano/src/roll/datums.rs` around lines 24 - 25, The rollback bug is
that DatumRefIncrement::undo() and DatumRefDecrement::undo() are currently
no-ops despite apply() saving was_new/prev_state, so update the undo
implementations to reverse what apply() did: use the stored prev_state and
was_new to restore the previous datum row and refcount (resurrect a deleted row
when prev_state indicates it existed, or remove a newly-created row when was_new
is true), and adjust the shared refcount appropriately; locate and modify
DatumRefIncrement::undo, DatumRefDecrement::undo, and ensure the logic mirrors
apply() (using the saved was_new/prev_state fields) in the visitor in
crates/cardano/src/roll/datums.rs so rollbacks correctly restore/delete rows and
decrement/increment refcounts.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
crates/cardano/src/model/pparams.rs (1)
275-290:⚠️ Potential issue | 🟡 Minor
unwrap_live()can still panic inera_transition().Same concern raised previously:
era_transitioncallsself.unwrap_live()on line 277, which panics via.expect()when the livePParamsSethas not been initialized. Since the method returnsOption<EraTransition>, prefer returningNoneearly when live is missing rather than panicking.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/cardano/src/model/pparams.rs` around lines 275 - 290, The method EpochValue<PParamsSet>::era_transition currently calls self.unwrap_live() which can panic if the live PParamsSet is uninitialized; change it to safely return None when live is missing by checking for a live value first (e.g. use self.live() or an Option-returning accessor) to obtain the original protocol_major_or_default(), and only proceed to compute update via self.next().and_then(|p| p.protocol_major()) if original is present; keep the rest of the logic and return Some(EraTransition) only when both original and update exist and differ.crates/cardano/src/model/accounts.rs (1)
578-581:⚠️ Potential issue | 🟡 Minor
unreachable!can panic whenprev_poolisNoneorNotDelegated.Same concern as the previous review: this branch triggers a panic if
PoolDelegatorRetireis ever applied to an account that is not currently delegated to aPool(e.g.,NotDelegatedor not yet snapshotted). If this is a genuine invariant, anexpectwith context about the delegator would make debugging production panics easier; otherwise, handle the case gracefully.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/cardano/src/model/accounts.rs` around lines 578 - 581, Replace the panic-prone unreachable! when matching self.prev_pool in PoolDelegatorRetire: if the invariant is genuine, use an expect on the Option with contextual info (e.g., include the delegator identifier) when extracting Some(PoolDelegation::Pool(pool)) before setting entity.retired_pool = Some(pool); otherwise handle the non-pool cases explicitly (match Some(PoolDelegation::NotDelegated) | None => set entity.retired_pool = None or return/log gracefully) so PoolDelegation::Pool(pool), self.prev_pool and entity.retired_pool are handled without an unconditional panic.
🧹 Nitpick comments (2)
crates/cardano/src/model/pparams.rs (1)
292-317:pgetter!panic messages contain literal$kindinstead of the interpolated variant name.Inside the string literal
"pparam $kind doesn't match value", the macro metavariable is not substituted — if a mismatch ever occurs, the panic message will be the literal text. Usestringify!($kind)as already done inensure_pparam!at line 323.♻️ Proposed fix
- pub fn [<$kind:snake>](&self) -> Option<$ty> { - let value = self.get(PParamKind::$kind)?; - - let PParamValue::$kind(x) = value else { - panic!("pparam $kind doesn't match value"); - }; - - Some(x.clone()) - } - - - pub fn [<$kind:snake _or_default>](&self) -> $ty { - let value = self.get_or_default(PParamKind::$kind); - - let PParamValue::$kind(x) = value else { - panic!("pparam $kind doesn't match value"); - }; - - x - } + pub fn [<$kind:snake>](&self) -> Option<$ty> { + let value = self.get(PParamKind::$kind)?; + + let PParamValue::$kind(x) = value else { + panic!("pparam {} doesn't match value", stringify!($kind)); + }; + + Some(x.clone()) + } + + pub fn [<$kind:snake _or_default>](&self) -> $ty { + let value = self.get_or_default(PParamKind::$kind); + + let PParamValue::$kind(x) = value else { + panic!("pparam {} doesn't match value", stringify!($kind)); + }; + + x + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/cardano/src/model/pparams.rs` around lines 292 - 317, The panic messages in the pgetter! macro use the literal "$kind" instead of the variant name; update the two panic! calls inside pgetter! (the ones within the generated methods [<$kind:snake>] and [<$kind:snake _or_default>]) to include the interpolated variant name using stringify!($kind) (e.g., format the message or use panic!("pparam {} doesn't match value", stringify!($kind))) similar to ensure_pparam! so the panic shows the actual PParamKind.crates/cardano/src/model/mod.rs (1)
295-336: Match arm ordering differs acrosskey(),apply(), andundo().The
CardanoDeltaenum declaresDRepRegistration,DRepUnRegistration,DRepActivity,DRepExpiration(lines 188–191) andPoolDelegatorRetirebeforeDRepDelegatorDrop-adjacent variants, but thekey()implementation at lines 308–311 and 324–325 uses a different order.apply()andundo()mirror the declaration order. Not a functional bug, just makes future maintenance error-prone when adding/removing variants.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/cardano/src/model/mod.rs` around lines 295 - 336, The match arms in the CardanoDelta::key() method are out of order compared to the CardanoDelta enum and the apply()/undo() implementations; reorder the match arms in fn key(&self) -> NsKey so they follow the same variant order as the enum declaration (including the sequence DRepRegistration, DRepUnRegistration, DRepActivity, DRepExpiration and the placement of PoolDelegatorRetire vs DRepDelegatorDrop) to keep key(), apply(), and undo() consistent and reduce future maintenance errors.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/cardano/src/model/mod.rs`:
- Around line 218-240: The code silently drops an existing CardanoEntity when a
variant mismatch occurs in CardanoDelta::downcast_apply and
CardanoDelta::downcast_undo; to fix it, save whether the original entity was
Some (e.g., let had = entity.is_some()), then after converting with
entity.take().and_then(|x| x.into()) assert in debug builds that either there
was no original (had == false) or the conversion produced Some
(sub_entity.is_some()), using debug_assert! with a clear message referencing
delta and entity to catch variant-mismatch bugs during development.
In `@crates/cardano/src/model/pools.rs`:
- Around line 82-93: PoolSnapshot::next_value currently calls current.expect("no
prior pool snapshot") which will panic if no prior snapshot exists; change it to
return None instead of panicking so callers (e.g., PoolTransition::apply via
default_transition) can handle missing prior snapshots, or alternatively
clone/construct a safe default snapshot and return Some(default) if that better
matches invariants; update PoolSnapshot::next_value (impl TransitionDefault) to
check for None and either return None or a constructed sensible default
(preserving fields like params via clone when present), and ensure any callers
such as PoolTransition::apply or default_transition handle the Option properly.
---
Duplicate comments:
In `@crates/cardano/src/model/accounts.rs`:
- Around line 578-581: Replace the panic-prone unreachable! when matching
self.prev_pool in PoolDelegatorRetire: if the invariant is genuine, use an
expect on the Option with contextual info (e.g., include the delegator
identifier) when extracting Some(PoolDelegation::Pool(pool)) before setting
entity.retired_pool = Some(pool); otherwise handle the non-pool cases explicitly
(match Some(PoolDelegation::NotDelegated) | None => set entity.retired_pool =
None or return/log gracefully) so PoolDelegation::Pool(pool), self.prev_pool and
entity.retired_pool are handled without an unconditional panic.
In `@crates/cardano/src/model/pparams.rs`:
- Around line 275-290: The method EpochValue<PParamsSet>::era_transition
currently calls self.unwrap_live() which can panic if the live PParamsSet is
uninitialized; change it to safely return None when live is missing by checking
for a live value first (e.g. use self.live() or an Option-returning accessor) to
obtain the original protocol_major_or_default(), and only proceed to compute
update via self.next().and_then(|p| p.protocol_major()) if original is present;
keep the rest of the logic and return Some(EraTransition) only when both
original and update exist and differ.
---
Nitpick comments:
In `@crates/cardano/src/model/mod.rs`:
- Around line 295-336: The match arms in the CardanoDelta::key() method are out
of order compared to the CardanoDelta enum and the apply()/undo()
implementations; reorder the match arms in fn key(&self) -> NsKey so they follow
the same variant order as the enum declaration (including the sequence
DRepRegistration, DRepUnRegistration, DRepActivity, DRepExpiration and the
placement of PoolDelegatorRetire vs DRepDelegatorDrop) to keep key(), apply(),
and undo() consistent and reduce future maintenance errors.
In `@crates/cardano/src/model/pparams.rs`:
- Around line 292-317: The panic messages in the pgetter! macro use the literal
"$kind" instead of the variant name; update the two panic! calls inside pgetter!
(the ones within the generated methods [<$kind:snake>] and [<$kind:snake
_or_default>]) to include the interpolated variant name using stringify!($kind)
(e.g., format the message or use panic!("pparam {} doesn't match value",
stringify!($kind))) similar to ensure_pparam! so the panic shows the actual
PParamKind.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a67ec4a3-c184-485b-b703-19578292b07e
📒 Files selected for processing (17)
crates/cardano/src/estart/commit.rscrates/cardano/src/estart/work_unit.rscrates/cardano/src/ewrap/loading.rscrates/cardano/src/ewrap/work_unit.rscrates/cardano/src/ewrap/wrapup.rscrates/cardano/src/genesis/mod.rscrates/cardano/src/lib.rscrates/cardano/src/model/accounts.rscrates/cardano/src/model/logs.rscrates/cardano/src/model/mod.rscrates/cardano/src/model/pools.rscrates/cardano/src/model/pparams.rscrates/cardano/src/model/proposals.rscrates/cardano/src/roll/accounts.rscrates/cardano/src/roll/batch.rscrates/cardano/src/roll/epochs.rscrates/cardano/src/roll/proposals.rs
✅ Files skipped from review due to trivial changes (4)
- crates/cardano/src/roll/batch.rs
- crates/cardano/src/estart/work_unit.rs
- crates/cardano/src/ewrap/work_unit.rs
- crates/cardano/src/lib.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- crates/cardano/src/ewrap/loading.rs
- crates/cardano/src/ewrap/wrapup.rs
- crates/cardano/src/roll/proposals.rs
| impl CardanoDelta { | ||
| pub fn downcast_apply<T, D>(delta: &mut D, entity: &mut Option<CardanoEntity>) | ||
| where | ||
| Option<T>: From<CardanoEntity>, | ||
| D: dolos_core::EntityDelta<Entity = T>, | ||
| T: Into<CardanoEntity>, | ||
| { | ||
| let mut sub_entity = entity.take().and_then(|x| x.into()); | ||
| delta.apply(&mut sub_entity); | ||
| *entity = sub_entity.map(|x| x.into()); | ||
| } | ||
|
|
||
| pub fn downcast_undo<T, D>(delta: &D, entity: &mut Option<CardanoEntity>) | ||
| where | ||
| Option<T>: From<CardanoEntity>, | ||
| D: dolos_core::EntityDelta<Entity = T>, | ||
| T: Into<CardanoEntity>, | ||
| { | ||
| let mut sub_entity = entity.take().and_then(|x| x.into()); | ||
| delta.undo(&mut sub_entity); | ||
| *entity = sub_entity.map(|x| x.into()); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Enumerate all delta target entity types to confirm no surprise variants slip through downcast.
rg -nP --type=rust 'type\s+Entity\s*=\s*\w+' -g 'crates/cardano/src/**'Repository: txpipe/dolos
Length of output: 2950
🏁 Script executed:
# First, let's examine the downcast methods and variant_boilerplate macro
head -250 crates/cardano/src/model/mod.rs | tail -50Repository: txpipe/dolos
Length of output: 1694
🏁 Script executed:
# Check for the variant_boilerplate macro definition
rg -n "variant_boilerplate" crates/cardano/src/model/mod.rsRepository: txpipe/dolos
Length of output: 655
🏁 Script executed:
# Look for the variant_boilerplate macro implementation
rg -n "macro_rules\!\s+variant_boilerplate" -A 30Repository: txpipe/dolos
Length of output: 168
🏁 Script executed:
# Read the variant_boilerplate macro definition (lines 77-95)
sed -n '77,95p' crates/cardano/src/model/mod.rsRepository: txpipe/dolos
Length of output: 574
🏁 Script executed:
# Look at how CardanoEntity is defined to understand the enum structure
sed -n '1,110p' crates/cardano/src/model/mod.rsRepository: txpipe/dolos
Length of output: 3109
🏁 Script executed:
# Find where downcast_apply and downcast_undo are called
rg -n "downcast_apply|downcast_undo" crates/cardano/src/model/mod.rs -B 2 -A 2Repository: txpipe/dolos
Length of output: 7301
🏁 Script executed:
# Look at the apply and undo methods in CardanoDelta to understand the dispatch
sed -n '290,400p' crates/cardano/src/model/mod.rsRepository: txpipe/dolos
Length of output: 6848
🏁 Script executed:
# Check a few specific delta implementations to see what Entity types they target
rg -n "impl.*EntityDelta.*for (ControlledAmountInc|StakeRegistration)" -A 3 crates/cardano/src/model/Repository: txpipe/dolos
Length of output: 618
🏁 Script executed:
# Look at ControlledAmountInc to see what entity type it targets
rg -n "impl EntityDelta for ControlledAmountInc" -A 10 crates/cardano/src/model/Repository: txpipe/dolos
Length of output: 38
🏁 Script executed:
# Try to see what entity types various deltas use
rg -n "type Entity = " crates/cardano/src/model/accounts.rs | head -5Repository: txpipe/dolos
Length of output: 235
🏁 Script executed:
# Check how entities are loaded/routed to deltas in the actual application
# Look for where apply/undo is called with entities
rg -n "\.apply\(|\.undo\(" crates/cardano/src/ -B 3 -A 1 | head -50Repository: txpipe/dolos
Length of output: 2966
🏁 Script executed:
# Check if different delta types could have the same key
# List all delta types and what Entity types they target
cat > /tmp/check_deltas.sh << 'EOF'
#!/bin/bash
# For each model file, extract delta types and their entity types
for file in crates/cardano/src/model/*.rs; do
echo "=== $(basename $file) ==="
rg -n "impl.*EntityDelta for (\w+)" -A 1 "$file" | grep -E "(impl|type Entity)" | paste - - | sed 's/--//' | head -20
done
EOF
bash /tmp/check_deltas.shRepository: txpipe/dolos
Length of output: 1024
🏁 Script executed:
# Now test if the suggested debug_assert compiles and passes clippy
cat > /tmp/test_debug_assert.rs << 'EOF'
fn test_downcast<T, D>(entity: &mut Option<T>)
where
Option<T>: From<String>,
{
let taken = entity.take();
let had_entity = taken.is_some();
let mut sub_entity: Option<T> = taken.and_then(|x| {
// Simulate the conversion
None::<T>
});
debug_assert!(
!had_entity || sub_entity.is_some(),
"CardanoEntity variant did not match delta target type",
);
}
fn main() {}
EOF
rustc --crate-type lib /tmp/test_debug_assert.rs 2>&1Repository: txpipe/dolos
Length of output: 102
🏁 Script executed:
# Map each entity type to the deltas that target it
cat > /tmp/analyze_deltas.py << 'EOF'
import re
import subprocess
# Get all delta type -> entity type mappings
result = subprocess.run(
['rg', '-n', 'impl.*EntityDelta for (\\w+)', '-A', '1', 'crates/cardano/src/model/'],
capture_output=True,
text=True
)
deltas = {}
for line in result.stdout.split('\n'):
match = re.search(r'for (\w+)\)', line)
if match:
current_delta = match.group(1)
match = re.search(r'type Entity = (\w+)', line)
if match:
entity = match.group(1)
if entity not in deltas:
deltas[entity] = []
deltas[entity].append(current_delta)
# Print summary
for entity, delta_list in sorted(deltas.items()):
print(f"{entity}: {len(delta_list)} deltas")
for d in sorted(set(delta_list)):
print(f" - {d}")
EOF
python3 /tmp/analyze_deltas.pyRepository: txpipe/dolos
Length of output: 272
🏁 Script executed:
# Check if any two deltas targeting different entities could have the same key
# Look at key implementations for a sample of deltas
rg -n "impl EntityDelta for (AccountTransition|PoolTransition|EpochTransition)" -A 10 crates/cardano/src/model/ | grep -E "(impl|fn key|NsKey)"Repository: txpipe/dolos
Length of output: 38
🏁 Script executed:
# Verify: do the deltas in CardanoDelta match what we found in the model files?
# Count the number of delta arms in apply() method
sed -n '340,380p' crates/cardano/src/model/mod.rs | grep -c "Self::"Repository: txpipe/dolos
Length of output: 58
🏁 Script executed:
# Better approach: extract all deltas and their entity types
rg -n 'impl.*EntityDelta for (\w+)' -A 1 crates/cardano/src/model/ | rg 'impl|type Entity'Repository: txpipe/dolos
Length of output: 6513
🏁 Script executed:
# Look at the key() implementations to understand if there could be namespace collisions
# Check AccountTransition specifically
rg -n "impl EntityDelta for AccountTransition" -A 15 crates/cardano/src/model/Repository: txpipe/dolos
Length of output: 38
🏁 Script executed:
# Check if different entity types could ever be stored at the same key
# Look at what key/namespace each delta produces
rg -n "fn key\(&self\)" -A 3 crates/cardano/src/model/accounts.rs | head -30Repository: txpipe/dolos
Length of output: 991
🏁 Script executed:
# Verify the key/namespace pattern holds across all deltas
# Check a few deltas from different entity types
rg -n "impl EntityDelta for (PoolRegistration|DRepRegistration|DatumRefIncrement)" -A 10 crates/cardano/src/model/ | grep -E "(impl|type Entity|fn key|NS)"Repository: txpipe/dolos
Length of output: 38
🏁 Script executed:
# Check if cargo clippy would pass on the file
cd crates/cardano && cargo clippy --lib 2>&1 | head -50Repository: txpipe/dolos
Length of output: 302
Add a debug assertion to catch silent entity drops on variant mismatch.
The entity.take().and_then(|x| x.into()) pattern relies on Option<T>::from(CardanoEntity) returning None for non-matching variants. If a CardanoEntity with the wrong variant ever reaches downcast_apply or downcast_undo—due to database corruption, a refactor breaking the entity-to-namespace mapping, or a bug in the loader—the original entity is silently dropped without error.
In normal operation, the namespace/key system strongly enforces that each delta's namespace maps to exactly one entity variant, preventing such mismatches. However, this invariant is load-bearing and invisible in the code. A debug_assert! would catch violations during development:
Suggested defensive check
pub fn downcast_apply<T, D>(delta: &mut D, entity: &mut Option<CardanoEntity>)
where
Option<T>: From<CardanoEntity>,
D: dolos_core::EntityDelta<Entity = T>,
T: Into<CardanoEntity>,
{
- let mut sub_entity = entity.take().and_then(|x| x.into());
+ let taken = entity.take();
+ let had_entity = taken.is_some();
+ let mut sub_entity: Option<T> = taken.and_then(|x| x.into());
+ debug_assert!(
+ !had_entity || sub_entity.is_some(),
+ "CardanoEntity variant did not match delta target type",
+ );
delta.apply(&mut sub_entity);
*entity = sub_entity.map(|x| x.into());
}Apply the same pattern to downcast_undo.
📝 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.
| impl CardanoDelta { | |
| pub fn downcast_apply<T, D>(delta: &mut D, entity: &mut Option<CardanoEntity>) | |
| where | |
| Option<T>: From<CardanoEntity>, | |
| D: dolos_core::EntityDelta<Entity = T>, | |
| T: Into<CardanoEntity>, | |
| { | |
| let mut sub_entity = entity.take().and_then(|x| x.into()); | |
| delta.apply(&mut sub_entity); | |
| *entity = sub_entity.map(|x| x.into()); | |
| } | |
| pub fn downcast_undo<T, D>(delta: &D, entity: &mut Option<CardanoEntity>) | |
| where | |
| Option<T>: From<CardanoEntity>, | |
| D: dolos_core::EntityDelta<Entity = T>, | |
| T: Into<CardanoEntity>, | |
| { | |
| let mut sub_entity = entity.take().and_then(|x| x.into()); | |
| delta.undo(&mut sub_entity); | |
| *entity = sub_entity.map(|x| x.into()); | |
| } | |
| } | |
| impl CardanoDelta { | |
| pub fn downcast_apply<T, D>(delta: &mut D, entity: &mut Option<CardanoEntity>) | |
| where | |
| Option<T>: From<CardanoEntity>, | |
| D: dolos_core::EntityDelta<Entity = T>, | |
| T: Into<CardanoEntity>, | |
| { | |
| let taken = entity.take(); | |
| let had_entity = taken.is_some(); | |
| let mut sub_entity: Option<T> = taken.and_then(|x| x.into()); | |
| debug_assert!( | |
| !had_entity || sub_entity.is_some(), | |
| "CardanoEntity variant did not match delta target type", | |
| ); | |
| delta.apply(&mut sub_entity); | |
| *entity = sub_entity.map(|x| x.into()); | |
| } | |
| pub fn downcast_undo<T, D>(delta: &D, entity: &mut Option<CardanoEntity>) | |
| where | |
| Option<T>: From<CardanoEntity>, | |
| D: dolos_core::EntityDelta<Entity = T>, | |
| T: Into<CardanoEntity>, | |
| { | |
| let mut sub_entity = entity.take().and_then(|x| x.into()); | |
| delta.undo(&mut sub_entity); | |
| *entity = sub_entity.map(|x| x.into()); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/cardano/src/model/mod.rs` around lines 218 - 240, The code silently
drops an existing CardanoEntity when a variant mismatch occurs in
CardanoDelta::downcast_apply and CardanoDelta::downcast_undo; to fix it, save
whether the original entity was Some (e.g., let had = entity.is_some()), then
after converting with entity.take().and_then(|x| x.into()) assert in debug
builds that either there was no original (had == false) or the conversion
produced Some (sub_entity.is_some()), using debug_assert! with a clear message
referencing delta and entity to catch variant-mismatch bugs during development.
| impl TransitionDefault for PoolSnapshot { | ||
| fn next_value(current: Option<&Self>) -> Option<Self> { | ||
| let current = current.expect("no prior pool snapshot"); | ||
|
|
||
| Some(PoolSnapshot { | ||
| is_retired: current.is_retired, | ||
| params: current.params.clone(), | ||
| blocks_minted: 0, | ||
| is_new: false, | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
PoolSnapshot::next_value panics when no prior snapshot exists.
current.expect("no prior pool snapshot") panics if the EpochValue<PoolSnapshot> is transitioned without a prior value. This is consumed via default_transition in PoolTransition::apply (line 367), which in turn is driven by epoch boundary wiring. If an edge case ever routes a PoolTransition to a pool without a live snapshot (e.g., a recently-registered pool whose scheduled snapshot was never materialized), the whole boundary processing will panic. Consider returning None (or cloning a sensible default) and letting the caller decide, or asserting this invariant at the call site with a more actionable message.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/cardano/src/model/pools.rs` around lines 82 - 93,
PoolSnapshot::next_value currently calls current.expect("no prior pool
snapshot") which will panic if no prior snapshot exists; change it to return
None instead of panicking so callers (e.g., PoolTransition::apply via
default_transition) can handle missing prior snapshots, or alternatively
clone/construct a safe default snapshot and return Some(default) if that better
matches invariants; update PoolSnapshot::next_value (impl TransitionDefault) to
check for None and either return None or a constructed sensible default
(preserving fields like params via clone when present), and ensure any callers
such as PoolTransition::apply or default_transition handle the Option properly.
1 participant
Summary by CodeRabbit