chore: Fix drep handling - #703
Conversation
WalkthroughAdds DRepActivity as a new Cardano delta and wires it into CardanoDelta. Refactors DRep identity to use explicit drep_id (Vec) instead of StakeCredential, updates registration/unregistration deltas, keying, and BlockVisitor to emit DRepActivity and appropriate reg/unreg deltas derived from certificates. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Chain
participant Visitor as BlockVisitor
participant Cert as Certificate
participant Mapper as cert_to_id / drep_to_id
participant Store as Delta System
Chain->>Visitor: visit_cert(cert, slot)
Visitor->>Mapper: derive drep_id from cert
Mapper-->>Visitor: drep_id (Vec<u8>)
Note over Visitor,Store: New/changed flow: always emit activity
Visitor->>Store: emit DRepActivity{ drep_id, slot }
alt RegDRepCert
Visitor->>Store: emit DRepRegistration{ drep_id, slot, deposit, anchor }
else UnRegDRepCert
Visitor->>Store: emit DRepUnRegistration{ drep_id, deposit }
else Other cert
Note over Visitor: No reg/unreg delta
end
Store->>Store: apply deltas (update last_active_slot, reg/unreg states)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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: 1
🧹 Nitpick comments (1)
crates/cardano/src/roll/dreps.rs (1)
35-48: Consider adding documentation for the cert_to_id function.This function is central to DRep identity resolution but lacks documentation explaining its purpose and the None return cases.
+/// Extracts the DRep ID from a certificate if it contains DRep-related information. +/// Returns None for certificates that don't involve DReps. fn cert_to_id(cert: &MultiEraCert) -> Option<Vec<u8>> {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
crates/cardano/src/model.rs(6 hunks)crates/cardano/src/roll/dreps.rs(7 hunks)
⏰ 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 (5)
crates/cardano/src/model.rs (1)
35-35: LGTM! DRepActivity integration looks complete.The DRepActivity delta has been properly integrated into the CardanoDelta enum with all the necessary boilerplate:
- Import added at line 35
- Enum variant added at line 961
- From trait implementation via delta_from! macro at line 1013
- Key, apply, and undo delegation properly handled in lines 1034, 1056, 1077
Also applies to: 961-961, 1013-1013, 1034-1034, 1056-1056, 1077-1077
crates/cardano/src/roll/dreps.rs (4)
144-183: LGTM! Clean implementation of DRepActivity delta.The DRepActivity struct properly tracks last active slot changes with appropriate undo capability. The implementation follows the same pattern as other deltas in the codebase.
195-213: Efficient activity tracking for all DRep-related certificates.Good design choice to emit DRepActivity for all certificates that reference a DRep, ensuring activity is tracked even for non-registration operations like voting delegations.
15-17: Reserve a distinct prefix for invented DRep IDs — avoid vec![0]/vec![1]crates/cardano/src/roll/dreps.rs maps Abstain => vec![0] and NoConfidence => vec![1] (see test-vectors: crates/test-vectors/src/build.rs:220–221). Verify that real DRep IDs can never be the single bytes 0x00/0x01 (i.e., are always encoded with DREP_KEY_PREFIX/DREP_SCRIPT_PREFIX + key bytes); if not, switch these to a reserved prefix/marker (e.g., 0xFF or DREP_RESERVED_PREFIX) to prevent collisions.
52-52: Verify migration/compatibility for drep_id key change
- Breaking change: replacing credential-based keys with direct drep_id (drep_id: Vec) will affect persisted data/serialization.
- Repo scan results: no migration scripts or DRepState (de)serialization impls were found; matches for "migration|upgrade|backward" were in src/serve/o7s_unix/chainsync.rs, src/relay/chainsync.rs, src/bin/dolos/init.rs, crates/test-vectors/src/epochs.sql, crates/minibf/src/routes/health.rs (appear unrelated). ast-grep found no impl for DRepState; no tests for DRepState/DRepRegistration surfaced.
- Action required: add a migration/upgrade path or compatibility layer, update serialization/deserialization and tests, or explicitly confirm and document that no persisted data depends on the previous credential-based key format.
Affected: crates/cardano/src/roll/dreps.rs — drep_id: Vec (lines ~52, 62-64, 77).
| fn apply(&mut self, entity: &mut Option<DRepState>) { | ||
| let entity = entity.get_or_insert_default(); | ||
|
|
||
| // save undo info | ||
| self.previous_last_active_slot = entity.last_active_slot; | ||
|
|
||
| // apply changes | ||
| entity.last_active_slot = Some(self.slot); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
🧩 Analysis chain
Consider initializing drep_id in the entity.
When applying DRepActivity, the entity is created with default values if it doesn't exist. However, the drep_id field in the entity is not being set from the delta's drep_id.
🏁 Script executed:
#!/bin/bash
# Check if drep_id is set elsewhere when creating DRepState
echo "Checking where drep_id is initialized in DRepState..."
ast-grep --pattern 'impl $_ for DRepRegistration {
$$$
fn apply($$$) {
$$$
}
$$$
}'
# Check the DRepState structure definition
echo -e "\nChecking DRepState structure..."
rg -A 10 "struct DRepState" --type rustLength of output: 2922
Initialize entity.drep_id when creating a DRepState
Both DRepRegistration::apply (crates/cardano/src/roll/dreps.rs:76–90) and DRepActivity::apply (crates/cardano/src/roll/dreps.rs:168–176) call entity.get_or_insert_default() but never set entity.drep_id; DRepState defines pub drep_id: Vec in crates/cardano/src/model.rs — set it when inserting the default (e.g. entity.drep_id = self.drep_id.clone()).
🤖 Prompt for AI Agents
In crates/cardano/src/roll/dreps.rs around lines 76–90 and 168–176, both
DRepRegistration::apply and DRepActivity::apply call
entity.get_or_insert_default() but never populate the DRepState.drep_id field;
update each apply to set entity.drep_id = self.drep_id.clone() immediately after
inserting the default (i.e., when get_or_insert_default() returns the newly
inserted state) so the drep_id is initialized correctly on creation.
2 participants
Summary by CodeRabbit