chore: remove protocol v9 specifics #620
Conversation
Signed-off-by: jeluard <jeluard@users.noreply.github.com>
WalkthroughThis PR strips out Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
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: 0
🧹 Nitpick comments (1)
crates/amaru-ledger/src/governance/ratification/stake_pools.rs (1)
119-121: Comment could use a refresh, cobber!The comment mentions "Starting from v10" but there's no actual version check in the code anymore. Since this PR removes v9 specifics, the comment could be simplified to just describe the behavior without the version reference. Something like "If no explicit vote, the fallback is given to the DRep..." would be clearer now that we're always on v10+. Just a bit of polish to keep things crisp! 🎯
🔎 Proposed comment update
- // Starting from v10, the fallback is given to the DRep chosen by the pool's - // reward account (?!), if any. If there's no drep, then the vote is considered - // to be "no" by default. + // If no explicit vote, the fallback is given to the DRep chosen by the pool's + // reward account, if any. If there's no DRep, then the vote is considered + // to be "no" by default.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
crates/amaru-ledger/src/governance/ratification.rscrates/amaru-ledger/src/governance/ratification/constitutional_committee.rscrates/amaru-ledger/src/governance/ratification/dreps.rscrates/amaru-ledger/src/governance/ratification/stake_pools.rscrates/amaru-ledger/src/rules/transaction/certificates.rscrates/amaru-plutus/src/script_context/v3.rscrates/amaru-stores/src/rocksdb/ledger/columns/accounts.rscrates/amaru-stores/src/rocksdb/ledger/columns/dreps.rscrates/amaru-stores/src/rocksdb/mod.rs
💤 Files with no reviewable changes (1)
- crates/amaru-plutus/src/script_context/v3.rs
🧰 Additional context used
🧠 Learnings (10)
📚 Learning: 2025-08-20T13:02:25.763Z
Learnt from: jeluard
Repo: pragma-org/amaru PR: 387
File: crates/amaru-stores/src/lib.rs:40-40
Timestamp: 2025-08-20T13:02:25.763Z
Learning: In the amaru-stores crate, amaru_slot_arithmetic types like Epoch and EraHistory are used throughout the main crate code in modules like in_memory/mod.rs, rocksdb/consensus.rs, and rocksdb/ledger/columns/, not just in tests. This means amaru-slot-arithmetic should be a regular dependency, not a dev-dependency.
Applied to files:
crates/amaru-ledger/src/rules/transaction/certificates.rscrates/amaru-stores/src/rocksdb/ledger/columns/dreps.rscrates/amaru-stores/src/rocksdb/ledger/columns/accounts.rscrates/amaru-stores/src/rocksdb/mod.rs
📚 Learning: 2025-09-29T16:44:14.807Z
Learnt from: rkuhn
Repo: pragma-org/amaru PR: 471
File: crates/amaru-network/src/protocol.rs:94-106
Timestamp: 2025-09-29T16:44:14.807Z
Learning: In the amaru-network crate protocol.rs file, the correct Cardano mini-protocol ID assignments are: PROTO_N2N_KEEP_ALIVE = 8 and PROTO_N2N_PEER_SHARE = 10, as verified against the network specification by the maintainer.
Applied to files:
crates/amaru-ledger/src/rules/transaction/certificates.rs
📚 Learning: 2025-12-16T21:32:37.668Z
Learnt from: rkuhn
Repo: pragma-org/amaru PR: 584
File: crates/amaru-network/src/handshake/tests.rs:40-47
Timestamp: 2025-12-16T21:32:37.668Z
Learning: In Rust, shadowing a binding with a new let does not drop the previous binding until the end of the scope. All shadowed bindings in a scope are dropped in reverse-declaration order when the scope ends. Therefore, multiple let _guard = register_*() calls will keep all guards alive until the end of the function (or the surrounding scope). When reviewing code, be mindful that resources tied to shadowed bindings persist longer than the most recent binding; to release early, constrain the lifetime in an inner block or explicitly drop guards when appropriate.
Applied to files:
crates/amaru-ledger/src/rules/transaction/certificates.rscrates/amaru-ledger/src/governance/ratification/dreps.rscrates/amaru-stores/src/rocksdb/ledger/columns/dreps.rscrates/amaru-ledger/src/governance/ratification/constitutional_committee.rscrates/amaru-ledger/src/governance/ratification.rscrates/amaru-stores/src/rocksdb/ledger/columns/accounts.rscrates/amaru-ledger/src/governance/ratification/stake_pools.rscrates/amaru-stores/src/rocksdb/mod.rs
📚 Learning: 2025-05-21T18:58:48.631Z
Learnt from: abailly
Repo: pragma-org/amaru PR: 228
File: crates/amaru-stores/src/rocksdb/consensus.rs:89-128
Timestamp: 2025-05-21T18:58:48.631Z
Learning: The InMemConsensusStore implementation in crates/amaru-stores/src/rocksdb/consensus.rs will be fleshed out incrementally on a by-need basis, driven by test requirements rather than implementing all functionality upfront.
Applied to files:
crates/amaru-stores/src/rocksdb/ledger/columns/dreps.rscrates/amaru-stores/src/rocksdb/ledger/columns/accounts.rscrates/amaru-stores/src/rocksdb/mod.rs
📚 Learning: 2025-01-21T15:32:17.911Z
Learnt from: jeluard
Repo: pragma-org/amaru PR: 69
File: crates/amaru/src/ledger/state/diff_epoch_reg.rs:112-117
Timestamp: 2025-01-21T15:32:17.911Z
Learning: When suggesting code changes in Rust, always verify that the types align correctly, especially when dealing with references and Options. The `Fold::Registered` variant in `diff_epoch_reg.rs` expects a reference `&'a V`, so unwrapping an `Option<&V>` requires only a single `.expect()`.
Applied to files:
crates/amaru-ledger/src/governance/ratification/constitutional_committee.rs
📚 Learning: 2025-04-20T17:57:23.233Z
Learnt from: rkuhn
Repo: pragma-org/amaru PR: 149
File: crates/amaru/src/stages/consensus/chain_forward/test_infra.rs:272-285
Timestamp: 2025-04-20T17:57:23.233Z
Learning: In test infrastructure code, rkuhn prefers explicit panics (using .unwrap() or similar) over returning Result types, as test failures should be immediate and obvious.
Applied to files:
crates/amaru-ledger/src/governance/ratification/constitutional_committee.rs
📚 Learning: 2025-08-18T08:10:32.640Z
Learnt from: KtorZ
Repo: pragma-org/amaru PR: 374
File: crates/amaru-stores/src/in_memory/mod.rs:427-433
Timestamp: 2025-08-18T08:10:32.640Z
Learning: The MemoryStore in crates/amaru-stores/src/in_memory/mod.rs is planned for a major revamp, so unimplemented methods like set_proposals_roots and set_constitution are intentionally left as placeholders until the revamp is complete.
Applied to files:
crates/amaru-stores/src/rocksdb/ledger/columns/accounts.rscrates/amaru-stores/src/rocksdb/mod.rs
📚 Learning: 2025-08-18T08:10:35.849Z
Learnt from: KtorZ
Repo: pragma-org/amaru PR: 374
File: crates/amaru-stores/src/in_memory/mod.rs:431-433
Timestamp: 2025-08-18T08:10:35.849Z
Learning: The MemoryStore in crates/amaru-stores/src/in_memory/mod.rs is planned for a major revamp soon, so unimplemented methods like set_constitution, set_proposals_roots are intentionally left aside until the revamp is complete.
Applied to files:
crates/amaru-stores/src/rocksdb/ledger/columns/accounts.rscrates/amaru-stores/src/rocksdb/mod.rs
📚 Learning: 2025-08-19T09:54:04.412Z
Learnt from: KtorZ
Repo: pragma-org/amaru PR: 374
File: crates/amaru-ledger/src/summary/stake_distribution.rs:219-225
Timestamp: 2025-08-19T09:54:04.412Z
Learning: In stake distribution calculations for governance voting, pool deposit refunds are intentionally handled asymmetrically: DRep voting stake includes the refund (`dreps_voting_stake += account.lovelace + drep_deposits + refund`) while Pool voting stake does not include the refund (`let delta = account.lovelace + pool_deposits`). This behavior matches the Haskell Cardano ledger reference implementation and is by design, not a bug.
Applied to files:
crates/amaru-ledger/src/governance/ratification/stake_pools.rs
📚 Learning: 2025-02-03T11:15:22.640Z
Learnt from: abailly
Repo: pragma-org/amaru PR: 75
File: crates/amaru/src/consensus/mod.rs:164-165
Timestamp: 2025-02-03T11:15:22.640Z
Learning: In the Amaru project, chain selection operations (roll_forward and rollback) should use separate result types to leverage the type system for preventing impossible states, rather than using runtime checks or panics.
Applied to files:
crates/amaru-stores/src/rocksdb/mod.rs
🧬 Code graph analysis (5)
crates/amaru-ledger/src/governance/ratification/dreps.rs (1)
crates/amaru-kernel/src/vote.rs (1)
any_vote_ref(28-30)
crates/amaru-stores/src/rocksdb/ledger/columns/dreps.rs (1)
crates/amaru-stores/src/rocksdb/common.rs (2)
as_key(25-27)as_value(30-32)
crates/amaru-ledger/src/governance/ratification/constitutional_committee.rs (1)
crates/amaru-ledger/src/summary/mod.rs (1)
into_safe_ratio(129-134)
crates/amaru-ledger/src/governance/ratification/stake_pools.rs (2)
crates/amaru-kernel/src/lib.rs (1)
expect_stake_credential(551-554)crates/amaru-ledger/src/governance/ratification/dreps.rs (1)
any_tally(293-311)
crates/amaru-stores/src/rocksdb/mod.rs (4)
crates/amaru-ledger/src/state.rs (1)
protocol_parameters(230-232)crates/amaru-ledger/src/store.rs (1)
protocol_parameters(166-166)crates/amaru-stores/src/in_memory/mod.rs (2)
protocol_parameters(101-103)protocol_parameters(367-369)crates/amaru-stores/src/in_memory/ledger/columns/accounts.rs (1)
add(23-50)
⏰ 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). (12)
- GitHub Check: End-to-end snapshot tests (preview, 2, 680, true, 10.5.3)
- GitHub Check: End-to-end snapshot tests (preprod, 1, 182, 10.5.3)
- GitHub Check: Build aarch64/linux
- GitHub Check: Build aarch64/macos
- GitHub Check: Build riscv32
- GitHub Check: Build wasm32
- GitHub Check: Test coverage
- GitHub Check: Build x86_64/linux
- GitHub Check: Build x86_64/windows
- GitHub Check: Test the CLI
- GitHub Check: Benches
- GitHub Check: Analyze (rust)
🔇 Additional comments (16)
crates/amaru-ledger/src/rules/transaction/certificates.rs (2)
24-25: Crikey, clean removal of the v9 baggage, mate!Removing
PROTOCOL_VERSION_9from the imports is spot on for this PR's mission. The code doesn't reference it anywhere below, so we're all sorted here.
215-217: The slot_to_epoch usage here is spot on—no double-dip needed!Your eyes picked up on something real, but it turns out the API's designed exactly this way. The
slot_to_epochfunction signature takes two parameters—slotandtip—so passingpointer.slot()twice isn't a mishap; it's the intended pattern. You'll see this same pattern throughout the codebase (rocksdb, praos, state, even the test suites), so the code here is bang on.The removal of the protocol version conditional is also intentional—the git history confirms this is part of cleaning up protocol v9 specifics. The refactor stands up solid.
crates/amaru-ledger/src/governance/ratification/dreps.rs (2)
33-37: Clean signature simplification, mate!The removal of the
protocol_versionparameter is spot on. The function signature is now more straightforward, and since the AI summary mentions that version gating has been removed from the function body, this is a solid cleanup. No drama here! 🎬
231-234: Test imports cleaned up nicely!Good to see the test imports aligned with the new reality. Removing the protocol version constants and keeping things tidy. This is cleaner than a fresh pair of Dunlop Volleys! 👟
crates/amaru-ledger/src/governance/ratification/constitutional_committee.rs (2)
122-127: Voting threshold signature looking fresh!Nice work stripping out the
protocol_versionparameter. The method signature is cleaner than a freshly wiped hard drive after a zombie apocalypse (looking at you, The Last of Us). 🧟
137-146: Committee size check is proper!The active member count check looks good without the version branching. The warning message at lines 138-142 is a beaut for debugging when the committee's running short-staffed. This is cleaner than a Melbourne café's pour-over setup! ☕
crates/amaru-ledger/src/governance/ratification.rs (1)
474-474: Tally call simplified beautifully!The updated
stake_pools::tallycall is as clean as a whistle. Dropping theprotocol_versionparameter keeps things tidy and aligns perfectly with the changes instake_pools.rs. This is smoother than butter on a hot pavlova! 🍰crates/amaru-ledger/src/governance/ratification/stake_pools.rs (2)
97-101: Tally signature is mint!The simplified
tallyfunction signature is looking sharp. Removing theprotocol_versionparameter streamlines the API nicely. This is as clean as a fresh fade from your local barber! 💈
324-341: Test helper updated like a boss!The
any_tallyhelper is now returning a cleaner tuple without theprotocol_version. The prop_map at lines 337-339 is straightforward and matches the new signature perfectly. This is tighter than a drum solo in a Rush song! 🥁crates/amaru-stores/src/rocksdb/ledger/columns/dreps.rs (2)
15-16: G'day! Import cleanup looks crisp as.The removal of
PROTOCOL_VERSION_9,ProtocolVersion,dreps_delegations, andaccountsfrom the imports aligns perfectly with the PR's mission to strip out v9-specific bits. Clean as a whistle, mate.
152-175: Refactoring checks out, mate.The simplified
dreps::removesignature is all good—all the call sites in both RocksDB and in-memory implementations have been updated to the new 2-parameter version. The delegation reset logic hasn't been orphaned somewhere collecting digital dust either; it looks like it's been cleanly separated into thedreps_delegationsmodule, which handles that tracking independently. No downstream code is expecting those delegation resets to happen throughdreps::removeanymore, so you're not breaking anything downstream.crates/amaru-stores/src/rocksdb/ledger/columns/accounts.rs (2)
15-23: Bonza import cleanup!Stripped out
DRep,CertificatePointer,PROTOCOL_VERSION_9, andProtocolVersion- the imports are now lean and mean, matching the simplified logic below. No complaints here, champion.
31-74: Function signature overhaul is solid, but let's check for stragglers.Right, so the
addfunction's been streamlined beautifully:
- Dropped the
protocol_versionparameter ✓- Changed return from
Result<Vec<...>, StoreError>toResult<(), StoreError>✓- The
_drepunderscore prefix on line 35 is textbook Rust - tells the compiler "yeah nah, not using this value" ✓The logic itself looks sound - it's updating or creating account rows without tracking previous delegations anymore. Much simpler, like going from a complicated flat white order to just asking for a coffee.
This is indeed a breaking API change, but the good news: both implementations (rocksdb and in_memory) have been updated consistently, and all call sites use the
?operator without capturing the return value. No code's actually expecting that oldVecback, so you're all sweet here.crates/amaru-stores/src/rocksdb/mod.rs (3)
19-20: Import changes track with the rest of the PR.The imports still pull in
ProtocolParametersbut dropped thePROTOCOL_VERSION_9constant. Makes sense - we're keeping the parameters but ditching the version-specific branching logic. Sweet as.
781-781: Updated call to accounts::add matches the new signature perfectly.Line 781 now calls
accounts::add(&self.db, add.accounts)?without the protocol_version parameter, which syncs up nicely with the signature change we saw in accounts.rs. The return value is no longer captured, which is exactly what we'd expect since it now returns().
796-796: All dreps::remove and accounts::add call sites have been properly updated—no stragglers with the old protocol_version signature.The search came up clean, mate. Both
accounts::addanddreps::removeare showing up with the correct 2-parameter signatures across the board:
- rocksdb/mod.rs lines 781 and 796: correct signatures ✓
- in_memory/mod.rs lines 704 and 714: correct signatures ✓
And no sneaky protocol_version references lurking about either. You've nailed it—like landing a perfect headshot in a no-scoping competition. All the function calls have been consistently refactored across both implementations.
|
This looks good to me, but I want to make sure we don't merge this until we update documentation and makefile commands to start from protocol version 10 blocks. |
|
Right, we need new snapshots before those changes can be applied. |
Summary by CodeRabbit
Release Notes
Refactor
Tests
✏️ Tip: You can customize this high-level summary in your review settings.