pedantic conway txbody #628
Conversation
WalkthroughAdds CBOR-backed generic pseudo-types for blocks, transactions, transaction bodies, and witness sets; introduces NonEmptySet and NonEmptyKeyValuePairs; adds conversions from minted/raw (KeepRaw) forms to owned types; updates kernel re-exports and migrates many ledger/plutus call sites to use TransactionBody/id() and memoized outputs. Changes
Sequence Diagram(s)(omitted — changes are type/API additions and conversion mappings; no new multi-actor sequential flow to diagram) Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
rrruko
force-pushed
the
rrruko/pedantic-txbody-2
branch
from
4e1bd3b to
e3fbb2f
Compare
Signed-off-by: ruko <rukokarasu@gmail.com>
rrruko
force-pushed
the
rrruko/pedantic-txbody-2
branch
from
e3fbb2f to
2c7e12b
Compare
![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 (4)
crates/amaru-ledger/src/rules/transaction/phase_one/collateral.rs (1)
331-336: G'day mate! Time to say goodbye to this commented-out code.Right, so I see you've left this test case commented out like a save file you're not sure you'll need again. While the explanation on line 331 is absolutely brilliant documentation (seriously, it's clearer than the plot of Inception), keeping the commented test code itself is a bit like leaving debug
console.logs in production – it's clutter that doesn't serve us anymore.Since Conway transactions now enforce non-empty collateral at the CBOR decode level, this test scenario literally can't happen for Conway era. The validation logic at lines 210-212 is still needed for backwards compatibility with earlier eras (Alonzo, Babbage), but this specific test is kaput.
Recommendation: Keep the explanatory comment (line 331) but remove the commented test code (lines 332-336). If you want to preserve this for historical context, a proper doc comment or commit message would be more appropriate than zombie code.
♻️ Proposed cleanup
- // This tx now fails to decode because conway txes cannot have empty collateral - //#[test_case( - // fixture!("3b13b5c319249407028632579ee584edc38eaeb062dac5156437a627d126fbb1", "no-collateral") => - // matches Err(InvalidCollateral::NoCollateral); - // "no collateral" - //)] + // Note: The "no-collateral" test case was removed because Conway transactions + // cannot have empty collateral (enforced at CBOR decode time). The NoCollateral + // error variant is still used for pre-Conway eras (Alonzo, Babbage).crates/amaru-kernel/src/non_empty_set.rs (1)
10-15: Consider enforcing set semantics or clarifying the doc comment, mate.The doc comment on line 13 says the vec "should contain no duplicates" but there's no validation in
from_vec(line 22) orTryFrom<Vec<T>>(line 38) to enforce this. It's like calling something a "set" but letting duplicates slide through like a bouncer on smoko.If set semantics (no duplicates) are critical for correctness, consider adding validation. This would require
T: Eq + Hashbounds and checking during construction. Alternatively, if duplicates are acceptable in practice and the caller is responsible for ensuring uniqueness, update the doc to say "may contain duplicates" or "caller must ensure no duplicates" to avoid confusion.♻️ Option 1: Add duplicate validation (requires additional bounds)
+use std::collections::HashSet; + /// Non-empty Set /// -/// Optional 258 tag (until era after Conway, at which point is it required) -/// with a vec of items which should contain no duplicates +/// Optional 258 tag (until era after Conway, at which point is it required) +/// with a vec of items which must contain no duplicates #[derive(Debug, PartialEq, Eq, Clone, PartialOrd, Serialize, Deserialize)] pub struct NonEmptySet<T>(Vec<T>); -impl<T> NonEmptySet<T> { +impl<T: Eq + Hash> NonEmptySet<T> { pub fn from_vec(x: Vec<T>) -> Option<Self> { - if x.is_empty() { None } else { Some(Self(x)) } + if x.is_empty() { + None + } else { + let set: HashSet<_> = x.iter().collect(); + if set.len() != x.len() { + None // or return Result with descriptive error + } else { + Some(Self(x)) + } + } } }♻️ Option 2: Clarify documentation if duplicates are caller's responsibility
/// Non-empty Set /// /// Optional 258 tag (until era after Conway, at which point is it required) -/// with a vec of items which should contain no duplicates +/// with a vec of items. Callers are responsible for ensuring no duplicates. #[derive(Debug, PartialEq, Eq, Clone, PartialOrd, Serialize, Deserialize)] pub struct NonEmptySet<T>(Vec<T>);crates/amaru-kernel/src/block.rs (1)
49-64: Consider usinginto_iter()to avoid extra clonesThe current pattern
.iter().cloned().map(|x| x.unwrap())creates intermediate clones before consuming. Since you're taking ownership ofxanyway, you could use.into_iter()directly—like skipping the tutorial and going straight to the action, ya know?♻️ Optional simplification
Block { header: x.header.unwrap().into(), transaction_bodies: pallas_primitives::MaybeIndefArray::Def( x.transaction_bodies - .iter() - .cloned() + .into_iter() .map(|x| x.unwrap()) .map(TransactionBody::from) .collect(), ), transaction_witness_sets: pallas_primitives::MaybeIndefArray::Def( x.transaction_witness_sets - .iter() - .cloned() + .into_iter() .map(|x| x.unwrap()) .map(WitnessSet::from) .collect(), ),crates/amaru-kernel/src/transaction_body.rs (1)
361-387: Minor: Redundant processing of required fieldsThe loop on lines 361-387 processes all entries including keys 0, 1, 2 (inputs, outputs, fee), which were already extracted on lines 346-354 to create the basic tx body. This means
set_tx_body_fieldis called again for these fields, overwriting with the same values.This isn't buggy—it's just a wee bit redundant, like watching the intro cutscene you can't skip. Could add a filter to skip keys 0, 1, 2 in the loop if you wanted to tidy it up.
♻️ Optional: Skip already-processed fields
- for (key, val) in entries { + for (key, val) in entries.into_iter().filter(|(k, _)| !matches!(k, 0 | 1 | 2)) {
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
crates/amaru-kernel/src/block.rscrates/amaru-kernel/src/lib.rscrates/amaru-kernel/src/memoized/transaction_output.rscrates/amaru-kernel/src/non_empty_key_value_pairs.rscrates/amaru-kernel/src/non_empty_set.rscrates/amaru-kernel/src/transaction.rscrates/amaru-kernel/src/transaction_body.rscrates/amaru-kernel/src/witness_set.rscrates/amaru-ledger/src/rules/block.rscrates/amaru-ledger/src/rules/transaction/phase_one/collateral.rscrates/amaru-ledger/src/rules/transaction/phase_one/mod.rscrates/amaru-ledger/src/rules/transaction/phase_one/proposals.rscrates/amaru-ledger/src/rules/transaction/phase_one/vkey_witness.rscrates/amaru-ledger/src/rules/transaction/phase_two/mod.rscrates/amaru-plutus/src/script_context/mod.rscrates/amaru-plutus/src/script_context/v1.rscrates/amaru-plutus/src/script_context/v3.rs
🧰 Additional context used
🧠 Learnings (11)
📓 Common learnings
Learnt from: KtorZ
Repo: pragma-org/amaru PR: 370
File: crates/amaru-kernel/src/transaction_pointer.rs:36-44
Timestamp: 2025-08-08T14:35:35.562Z
Learning: In the amaru project, when decoding CBOR arrays, prefer using minicbor_extra::heterogenous_array with the expected length to validate definite-length arrays and correctly handle indefinite-length arrays. Example: crates/amaru-kernel/src/transaction_pointer.rs Decode should use heterogenous_array(d, 2, …) instead of ignoring the length from d.array().
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.
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.
📚 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/phase_one/collateral.rscrates/amaru-plutus/src/script_context/v3.rscrates/amaru-ledger/src/rules/transaction/phase_one/vkey_witness.rscrates/amaru-kernel/src/block.rscrates/amaru-ledger/src/rules/block.rscrates/amaru-kernel/src/non_empty_key_value_pairs.rscrates/amaru-plutus/src/script_context/v1.rscrates/amaru-kernel/src/transaction.rscrates/amaru-kernel/src/transaction_body.rscrates/amaru-kernel/src/witness_set.rscrates/amaru-plutus/src/script_context/mod.rscrates/amaru-kernel/src/memoized/transaction_output.rscrates/amaru-ledger/src/rules/transaction/phase_one/mod.rscrates/amaru-ledger/src/rules/transaction/phase_two/mod.rscrates/amaru-kernel/src/non_empty_set.rscrates/amaru-ledger/src/rules/transaction/phase_one/proposals.rscrates/amaru-kernel/src/lib.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-plutus/src/script_context/v3.rscrates/amaru-ledger/src/rules/block.rs
📚 Learning: 2025-08-23T15:44:44.318Z
Learnt from: rkuhn
Repo: pragma-org/amaru PR: 392
File: crates/ouroboros-traits/src/is_header/fake.rs:52-88
Timestamp: 2025-08-23T15:44:44.318Z
Learning: In the Amaru project, FakeHeader in crates/ouroboros-traits/src/is_header/fake.rs is specifically designed for testing purposes, and panic behavior (like copy_from_slice() panicking on malformed hex) is the preferred approach rather than graceful error handling, as it helps identify test data issues quickly during development.
Applied to files:
crates/amaru-ledger/src/rules/transaction/phase_one/vkey_witness.rscrates/amaru-ledger/src/rules/transaction/phase_one/proposals.rs
📚 Learning: 2025-08-08T14:43:20.218Z
Learnt from: KtorZ
Repo: pragma-org/amaru PR: 370
File: crates/amaru-kernel/src/point.rs:45-52
Timestamp: 2025-08-08T14:43:20.218Z
Learning: In crates/amaru-kernel/src/point.rs, Point::Specific stores a header hash that is always exactly 32 bytes by project invariant. Therefore, converting it with `impl From<&Point> for Hash<32>` without a runtime length check is acceptable; future reviews should not request an error-returning check inside this `From` impl. If validation is ever desired, it should be done via `TryFrom` or upstream constructors.
Applied to files:
crates/amaru-ledger/src/rules/transaction/phase_one/vkey_witness.rs
📚 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/block.rscrates/amaru-plutus/src/script_context/mod.rscrates/amaru-ledger/src/rules/transaction/phase_two/mod.rscrates/amaru-ledger/src/rules/transaction/phase_one/proposals.rscrates/amaru-kernel/src/lib.rs
📚 Learning: 2025-09-29T20:08:29.906Z
Learnt from: yHSJ
Repo: pragma-org/amaru PR: 453
File: crates/amaru-metrics/src/ledger.rs:60-104
Timestamp: 2025-09-29T20:08:29.906Z
Learning: In amaru-ledger/src/state.rs, the txs_processed field represents the number of transactions in the current block (block.transaction_bodies.len() as u64), not a cumulative total. Therefore, using a Counter with .add() in the metrics is correct for accumulating per-block transaction counts.
Applied to files:
crates/amaru-ledger/src/rules/block.rs
📚 Learning: 2025-08-08T14:35:35.562Z
Learnt from: KtorZ
Repo: pragma-org/amaru PR: 370
File: crates/amaru-kernel/src/transaction_pointer.rs:36-44
Timestamp: 2025-08-08T14:35:35.562Z
Learning: In the amaru project, when decoding CBOR arrays, prefer using minicbor_extra::heterogenous_array with the expected length to validate definite-length arrays and correctly handle indefinite-length arrays. Example: crates/amaru-kernel/src/transaction_pointer.rs Decode should use heterogenous_array(d, 2, …) instead of ignoring the length from d.array().
Applied to files:
crates/amaru-kernel/src/non_empty_key_value_pairs.rscrates/amaru-kernel/src/transaction_body.rscrates/amaru-ledger/src/rules/transaction/phase_one/proposals.rscrates/amaru-kernel/src/lib.rs
📚 Learning: 2025-08-08T14:39:50.527Z
Learnt from: KtorZ
Repo: pragma-org/amaru PR: 370
File: crates/amaru-kernel/src/borrowed_datum.rs:32-39
Timestamp: 2025-08-08T14:39:50.527Z
Learning: In the amaru project, when converting BorrowedDatumOption::Data to an owned DatumOption in crates/amaru-kernel/src/borrowed_datum.rs, the call `.unwrap()` refers to pallas’s KeepRaw::unwrap, which is infallible (always returns the inner value) and is not a panic risk. Future reviews should not flag this unwrap as dangerous.
Applied to files:
crates/amaru-kernel/src/memoized/transaction_output.rs
📚 Learning: 2025-08-08T14:46:53.013Z
Learnt from: KtorZ
Repo: pragma-org/amaru PR: 370
File: crates/amaru-kernel/src/pool_params.rs:107-116
Timestamp: 2025-08-08T14:46:53.013Z
Learning: In crates/amaru-kernel/src/pool_params.rs, when serializing Relay::SingleHostAddr IPv6 to text, the project intentionally reverses each 4-byte chunk before constructing std::net::Ipv6Addr. This matches cardano-ledger’s IPv6 representation (four little-endian Word32 chunks). Do not “simplify” by passing the raw 16 bytes directly to Ipv6Addr::from; that would break ledger compatibility.
Applied to files:
crates/amaru-kernel/src/memoized/transaction_output.rs
📚 Learning: 2025-08-08T14:34:06.105Z
Learnt from: KtorZ
Repo: pragma-org/amaru PR: 370
File: crates/minicbor-extra/src/lib.rs:50-55
Timestamp: 2025-08-08T14:34:06.105Z
Learning: Project uses Rust 1.88 stable; using Result::is_err_and is acceptable and should not be flagged as unstable. In particular, crates/minicbor-extra/src/lib.rs relies on is_err_and when checking end_of_input; future reviews should not suggest replacing it for stability reasons.
Applied to files:
crates/amaru-kernel/src/lib.rs
🧬 Code graph analysis (13)
crates/amaru-plutus/src/script_context/v3.rs (1)
crates/amaru-kernel/src/transaction_body.rs (1)
get_original_hash(424-426)
crates/amaru-ledger/src/rules/transaction/phase_one/vkey_witness.rs (1)
crates/amaru-kernel/src/transaction_body.rs (1)
get_original_hash(424-426)
crates/amaru-kernel/src/block.rs (3)
crates/amaru-kernel/src/transaction.rs (1)
from(37-44)crates/amaru-kernel/src/transaction_body.rs (1)
from(398-421)crates/amaru-kernel/src/witness_set.rs (1)
from(66-77)
crates/amaru-ledger/src/rules/block.rs (2)
crates/amaru-kernel/src/transaction_body.rs (1)
get_original_hash(424-426)crates/amaru-ledger/src/state.rs (1)
network(224-226)
crates/amaru-plutus/src/script_context/v1.rs (1)
crates/amaru-kernel/src/transaction_body.rs (1)
get_original_hash(424-426)
crates/amaru-kernel/src/transaction.rs (2)
crates/amaru-kernel/src/transaction_body.rs (1)
from(398-421)crates/amaru-kernel/src/witness_set.rs (1)
from(66-77)
crates/amaru-kernel/src/transaction_body.rs (3)
crates/amaru-kernel/src/non_empty_key_value_pairs.rs (3)
d(153-153)decode(150-169)from(78-83)crates/amaru-kernel/src/non_empty_set.rs (2)
decode(67-84)from(48-51)crates/amaru-kernel/src/transaction.rs (1)
from(37-44)
crates/amaru-plutus/src/script_context/mod.rs (2)
crates/amaru-kernel/src/lib.rs (6)
from(304-306)from(327-329)from(365-367)from(533-538)from(542-549)from(601-609)crates/amaru-kernel/src/non_empty_key_value_pairs.rs (3)
from(78-83)try_from(93-99)try_from(109-126)
crates/amaru-kernel/src/memoized/transaction_output.rs (1)
crates/amaru-kernel/src/non_empty_key_value_pairs.rs (3)
try_from(93-99)try_from(109-126)from_vec(50-56)
crates/amaru-ledger/src/rules/transaction/phase_one/mod.rs (1)
crates/amaru-kernel/src/transaction_body.rs (1)
get_original_hash(424-426)
crates/amaru-ledger/src/rules/transaction/phase_two/mod.rs (1)
crates/amaru-kernel/src/transaction_body.rs (1)
get_original_hash(424-426)
crates/amaru-kernel/src/non_empty_set.rs (1)
crates/amaru-kernel/src/non_empty_key_value_pairs.rs (10)
decode(150-169)to_vec(46-48)from_vec(50-56)deref(136-141)try_from(93-99)try_from(109-126)from(78-83)into_iter(33-38)d(153-153)encode(177-204)
crates/amaru-ledger/src/rules/transaction/phase_one/proposals.rs (1)
crates/amaru-kernel/src/transaction_body.rs (1)
get_original_hash(424-426)
🔇 Additional comments (41)
crates/amaru-kernel/src/non_empty_set.rs (3)
63-85: Nice work on the optional tag handling, champion!The decode logic correctly handles the optional 258 tag (lines 69-75) while also validating non-empty constraint (lines 79-81). This is spot-on for the transition period before the tag becomes required in post-Conway eras. It's like having a backup plan that's also your main plan – proper belt-and-braces stuff.
87-101: Encode always writes the tag – intentional and future-proof.The encode implementation always writes the TAG_SET (258) tag on line 96, even though the comment on line 12 notes it's "optional" until post-Conway. This asymmetry with decode (which handles it as optional) is actually solid forward-compatibility design – like packing an umbrella even when it's sunny. When the tag becomes required in future eras, this code won't need changes.
27-61: LGTM – solid boilerplate implementations!The Deref, TryFrom, From, and IntoIterator implementations are all implemented correctly and follow idiomatic Rust patterns. The TryFrom returning the original Vec as the error type (line 36) is particularly nice – lets callers recover their data rather than losing it to the void.
crates/amaru-kernel/src/lib.rs (4)
48-48: Refactoring the exports – fair dinkum!Removing
NonEmptyKeyValuePairsfrom thepallas_codec::utilsre-export (line 48) makes sense since it's now exported via its own dedicated module (lines 127-128). This keeps the module organization tidy and aligns with the PR's goal of consolidating non-empty container types.
62-71: Nice namespace hygiene with the Pallas prefixes!Adding explicit aliases like
PallasNonEmptyKeyValuePairs,PallasRequiredSigners, andPallasVotingProcedures(lines 64-71) creates clear separation between pallas re-exports and amaru's own wrapper types. TheRedeemersKey as RedeemerKeyrename also fixes the plurality – it's one key, not multiple keys, innit?
127-131: Module organization looking crisp!The new module declarations for
non_empty_key_value_pairs(line 127) andnon_empty_set(line 130) with their corresponding public re-exports follow the established pattern in this file. Clean and consistent module organization – like Marie Kondo visited the codebase.
179-189: Type aliases add semantic clarity – bonzer!The new module declarations (lines 179-189) and type aliases (lines 1040-1043) strengthen the type system by giving semantic names to structured types.
RequiredSigners = NonEmptySet<AddrKeyhash>clearly communicates intent, and the nestedVotingProceduresstructure properly models the domain's non-empty constraints. This is exactly the kind of type-level documentation that makes codebases self-documenting.Also applies to: 1040-1043
crates/amaru-ledger/src/rules/transaction/phase_two/mod.rs (2)
19-20: Clean refactor from trait to function, mate!The import changes (lines 19-20) replacing
OriginalHashtrait withget_original_hashfunction are spot on. This simplifies the API by using a free function instead of requiring trait implementation, making it easier to use in generic contexts. Like swapping a Swiss Army knife for a purpose-built tool.
126-126: Correct usage of the new hash function!The call to
get_original_hash(transaction_body)on line 126 is correct. The function expects&KeepRaw<'a, T>andtransaction_bodyis already&KeepRaw<'_, MintedTransactionBody<'_>>(from the function signature on line 76), so this passes the reference correctly. Should produce identical results to the previoustransaction_body.original_hash()call.crates/amaru-plutus/src/script_context/v1.rs (2)
285-285: Import addition looks sound, cobber!Adding
get_original_hashto the imports (line 285) is necessary for the usage in the test. The placement in the import list is logical alongside other kernel utilities.
326-326: Test updated correctly with the new API!The call to
get_original_hash(&transaction.transaction_body)on line 326 is correct. Thetransaction.transaction_bodyfield is of typeKeepRaw<'_, MintedTransactionBody<'_>>, and taking a reference (&) produces&KeepRaw<...>which matches theget_original_hashsignature perfectly. The test should behave identically to before.crates/amaru-ledger/src/rules/transaction/phase_one/mod.rs (2)
17-18: Consistent refactor across the codebase – good stuff!The import changes (lines 17-18) follow the same pattern as the other modules in this PR. Removing the
OriginalHashtrait and adding theget_original_hashfunction creates consistency across the ledger and plutus crates. Like getting the band back together with matching outfits.
104-104: Proper usage of get_original_hash in phase one!Line 104 correctly calls
get_original_hash(&transaction_body)wheretransaction_bodyisKeepRaw<'_, MintedTransactionBody<'_>>(from line 97). The reference is taken and passed toget_original_hashwhich expects&KeepRaw<'a, T>, so this matches perfectly. Thetransaction_idwill be computed correctly, ready for use in the validation logic below.crates/amaru-ledger/src/rules/block.rs (2)
23-23: Solid import update, mate!Right, so you've swapped the
OriginalHashtrait for the shiny newget_original_hashfunction. This is textbook API evolution - moving from a trait method to a standalone function. Cleaner and more flexible, like upgrading from a Nokia 3310 to something that can actually run apps.
214-214: LGTM - Textbook API migration!You've nailed the transition here. The function call
get_original_hash(&transaction)is spot-on with the signature expecting&KeepRaw<'a, T>. Same hash, new API - it's like when they rebooted Spider-Man but kept the web-slinging intact.crates/amaru-plutus/src/script_context/v3.rs (2)
644-644: Test import looking crisp!Adding
get_original_hashto your test imports - no dramas here. It's all contained in the test module, so you're not polluting the production namespace. Good hygiene, like washing your hands after touching public transport poles.
687-687: Test code transition looks bonza!You're drilling down to
&transaction.transaction_bodybefore getting the hash - that's the right move for the Plutus test harness. The TxInfo construction stays the same, just with a new hash supplier. It's like changing the oil in your car - same engine, fresh lubricant.crates/amaru-ledger/src/rules/transaction/phase_one/proposals.rs (2)
74-74: Import refactor on point!Your test imports are getting the get_original_hash treatment. Keeping it consistent with the rest of the PR - I appreciate when a refactor doesn't try to reinvent the wheel in every file. It's like following the same map in Dark Souls; you know where the traps are.
128-128: Proposal test migration is mint!You're constructing that transaction tuple with
get_original_hash(&tx)- clean as. The function signature expects(TransactionId, TransactionPointer)and you're delivering the goods. This is the kind of mechanical refactor that makes me happy, like when all the Git conflicts resolve themselves.crates/amaru-ledger/src/rules/transaction/phase_one/vkey_witness.rs (2)
130-130: Test import addition looking solid!Adding
get_original_hashto the vkey witness test imports - you're following the same refactoring pattern across the board. It's like speedrunning a game after you've learned all the routes; efficient and satisfying to watch.
292-292: Bootstrap witness test looking grouse!You're computing the
transaction_idwithget_original_hash(&tx)before passing it to the witness validation. The signature verification needs the original hash to work, and you're serving it up fresh. It's like having the right key for the right lock - simple but essential.crates/amaru-kernel/src/memoized/transaction_output.rs (2)
213-252: Type system alignment looking deliberate!Right, so you're explicitly qualifying
pallas_primitives::NonEmptyKeyValuePairsin the legacy value conversion path. This tells me there's likely both a local wrapper type and the Pallas primitive type floating around, and you need to be crystal clear about which one you're using.The explicit type annotation at line 248 and the construction pattern are intentional - you're making sure the Value::Multiasset gets the Pallas-flavored pairs. It's like specifying you want the imported beer, not the local brew - sometimes you gotta be specific about what you're after.
316-322: Conversion pipeline looking choice!You've set up a nice little assembly line here:
Vec -> NonEmptyKeyValuePairs -> .to_pallas() -> pallas_primitives::NonEmptyKeyValuePairs. The.to_pallas()call at line 320 is the money shot - that's your bridge between the internal wrapper type and the external Pallas representation.The error handling with
.ok_orfor empty bundles is good defensive coding. Can't have a non-empty collection that's... empty. That'd be like a decaf coffee - defeats the whole purpose.crates/amaru-kernel/src/transaction.rs (2)
8-26: G'day mate! Looking solid here 🍀The
PseudoTxstruct is cleanly laid out with proper CBOR field annotations. The generic approach withKeepRawwrappers for the minted variant is a nice pattern—like having your save game state preserved while still being able to load it into a new engine.One small nit: using
std::clone::Clonein the where bounds (lines 11-13) is a tad verbose. You could just useClonesince it's in the prelude. Not a biggie though, more of a "would ya look at that" moment.
36-45: Conversion logic looks spot on!This
Fromimplementation follows the same unwrap-and-convert pattern as the other minted-to-owned conversions inwitness_set.rsandtransaction_body.rs. It's like converting your raw footage to the final cut—preserving what matters.crates/amaru-kernel/src/block.rs (2)
7-28: Nicely structured generic block type! 🎮The
PseudoBlockdesign is clean. I notice onlyT4has theClonebound at the struct level (line 10). This works because the derive macros will add the necessary bounds, but it's a bit asymmetric compared to howPseudoTxhandles it.Also, line 15 uses
#[b(1)]while the others use#[n(x)]—I assume this is intentional for borrowing semantics during CBOR encoding?Could you confirm the
#[b(1)]annotation is intentional here? It differs from the other field annotations.
49-56: Minor: Always usingDefvariantI see the conversion always wraps in
MaybeIndefArray::Def, even if the original wasIndef. This is probably fine for the owned representation since the raw CBOR is preserved inMintedBlockfor hashing. Just flagging it in case preserving that distinction matters downstream.crates/amaru-plutus/src/script_context/mod.rs (2)
20-27: Clean aliasing for type disambiguation 👍Good call on renaming the imports with
Pallas*prefixes. It's like giving your characters distinct names in an RPG—makes it crystal clear whichNonEmptyKeyValuePairsyou're dealing with when both the kernel and Pallas have their own versions.
955-959: Implementation signatures updated consistentlyAll the
FromandTryFromimplementations have been updated to use the newPallas*type aliases. The actual conversion logic remains unchanged—just the type signatures. Like a texture pack update without touching the gameplay mechanics.Also applies to: 1039-1044, 1137-1138
crates/amaru-kernel/src/non_empty_key_value_pairs.rs (3)
14-23: Well-designed non-empty container type 🎯Nice work on the
NonEmptyKeyValuePairsenum! TheDef/Indefvariants preserve the CBOR encoding style, which is important for round-trip fidelity. The serde attributes fortry_fromandintoVec are spot on—keeps the serialization layer simple while enforcing invariants.
144-170: CBOR Decode implementation looks solidThe decode flow is correct—you grab the datatype first to distinguish
MapvsMapIndef, then consume the items and validate non-emptiness. The error messages are descriptive too. Like having proper error screens in your game instead of just crashing to desktop!One thing to note: the learning from the project suggests using
minicbor_extra::heterogenous_arrayfor array decoding, but since this is a map structure (not an array), the current approach is appropriate.
172-205: Encode implementation mirrors decode correctlyThe encoding properly distinguishes between definite-length (
Def) and indefinite-length (Indef) maps. The symmetry with the decoder is on point—what goes in, comes out the same way.crates/amaru-kernel/src/witness_set.rs (3)
9-35: Clean witness set structure with CBOR map encodingThe
WitnessSetstruct is well-organized with proper optionalNonEmptySetwrappers for the witness components. Using#[cbor(map)]is the right call for this kind of structure.
52-53: Question on#[b(4)]annotationI notice
plutus_datauses#[b(4)]while other fields use#[n(x)]. Same pattern astransaction_bodiesinblock.rs. Is this for special borrowing semantics withKeepRaw? Just want to make sure it's intentional and not a typo, mate.
65-78: From implementation handles all the KeepRaw unwrapping correctlyThe conversion logic correctly:
- Copies fields that aren't wrapped in
KeepRawdirectly- Uses
.map(Into::into)forNonEmptySet<KeepRaw<..>>fields- Uses
.map(|x| x.unwrap())for theredeemerwhich isKeepRaw<Redeemers>This matches the established pattern in the codebase. Nice and consistent!
crates/amaru-kernel/src/transaction_body.rs (6)
9-72: Comprehensive Conway transaction body structureThis is a proper beast of a struct, covering all the Conway-era transaction body fields including the new governance additions (voting_procedures, proposal_procedures, treasury_value, donation). The CBOR field indices match the Cardano spec. Well done, like completing all the side quests!
100-194: Field decoder covers all Conway transaction body keysThe
decode_tx_body_fieldfunction handles all valid field indices properly. The skipped indices (6, 10, 12) are correctly not part of the Conway transaction body spec. Unknown keys return an error—fail fast, just like a game that tells you when you're doing something wrong instead of silently corrupting your save.
200-234: TxBodyFields collector handles both map lengths correctlyThe decode implementation handles both definite and indefinite-length CBOR maps. Accumulating values per key into a
Vecenables proper duplicate detection downstream. Solid defensive coding—like having a checkpoint before the boss fight!
397-422: From conversion handles all 20+ fields correctlyThe
From<MintedTransactionBody>implementation correctly converts all fields, with proper.into()calls for the output types that need conversion. It's like a faithful port of a game to a new platform—everything's there!
424-426: Clean hash utility functionThe
get_original_hashfunction is a nice, simple utility replacing the previous trait-based approach. It computes the Blake2b-256 hash of the raw CBOR bytes preserved byKeepRaw. Short, sweet, and does exactly what it says on the tin!
428-570: Comprehensive test coverage for decoding edge cases 🧪Love seeing all these negative test cases! You've covered:
- Missing required fields (inputs, outputs, fee)
- Empty values for NonEmptySet fields (certs, collateral, required_signers, proposals)
- Empty values for NonEmptyKeyValuePairs fields (withdrawals, voting_procedures)
- Empty mint validation
- Zero donation rejection
- Duplicate key rejection
This is the kind of test suite that lets you refactor with confidence. Like having a full achievement list and knowing you've 100%'d the game!
Signed-off-by: ruko <rukokarasu@gmail.com>
| K: Clone, | ||
| V: Clone, | ||
| { | ||
| type Error = String; |
There was a problem hiding this comment.
I'm not familiar with amaru error conventions, but String as error is a bit awkward to work with (callers have to convert it to their error type, and it forces an allocation). I think a better alternative would be to define an error type which implemented Error (like https://doc.rust-lang.org/src/core/num/error.rs.html#10)
There was a problem hiding this comment.
Signed-off-by: KtorZ <matthias.benkort@gmail.com>
There's no room for duplicates in there. Note that the current approach is not fully optimal because it must still re-traverse the set after decoding it, comparing elements with one another. We may later want to perform that check _while decoding_ (though it would require extra allocation). Another option is to simply decode into a BTreeSet / HashSet, but that requires the underlying T to abide by the relevant requirements. At this point, I am not sure whether ALL Note that this commits drops the support for decode -> encode roundtrip; which is generally *not necessary* so long as we preserve original bytes or hash of relevant structures that needs be re-serialised (transaction, datum, value, script, etc...). Signed-off-by: KtorZ <matthias.benkort@gmail.com>
Signed-off-by: KtorZ <matthias.benkort@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@crates/amaru-kernel/src/non_empty_key_value_pairs.rs`:
- Around line 163-184: The doc-comment for has_duplicate incorrectly states the
algorithm is O(n*log(n)) while the implementation is a quadratic O(n^2)
nested-loop check; update the documentation above fn has_duplicate to correctly
state O(n^2) (or "quadratic") and remove or adjust any mention of log(n) and
sorting assumptions so the comment matches the actual complexity and behavior of
has_duplicate.
In `@crates/amaru-kernel/src/non_empty_set.rs`:
- Around line 139-160: The doc comment for has_duplicate incorrectly states the
complexity as O(n*log(n)); update the comment to accurately reflect the
implementation's complexity as O(n^2) due to the nested loop over the slice,
keeping the existing rationale about avoiding Hash/Ord bounds and preserving
order; specifically edit the comment block above pub(crate) fn has_duplicate<T:
Eq>(xs: &[T]) -> bool to replace "The check is still in O(n*log(n))." with a
correct statement like "The check is O(n²) (nested scan)"; do not change the
implementation itself unless you intend to optimize it, in which case note the
function name has_duplicate as the target for any algorithmic change.
| /// Check whether a slice contains duplicate relying only on the `Eq` instance and minimizing | ||
| /// allocation. The check is still in O(n*log(n)). | ||
| /// | ||
| /// We do not use HashMap or BTreeMap for mainly two reasons: | ||
| /// | ||
| /// 1. They introduce additional requirements on `K` (Hash in one case, and Ord on the other). | ||
| /// 2. We want to preserve the underlying order when possible; | ||
| /// | ||
| /// Pre-condition: the slice is NOT empty. | ||
| pub(crate) fn has_duplicate<K: Eq, V>(kvs: &[(K, V)]) -> bool { | ||
| let last = kvs.len() - 1; | ||
|
|
||
| for i in 0..last { | ||
| let (k1, _) = &kvs[i]; | ||
| for (k2, _) in kvs.iter().take(last + 1).skip(i + 1) { | ||
| if k1 == k2 { | ||
| return true; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| false |
There was a problem hiding this comment.
Complexity note says O(n log n) but implementation is O(n²).
Update the doc to match reality.
📝 Suggested doc fix
-/// allocation. The check is still in O(n*log(n)).
+/// allocation. The check is O(n^2) due to the nested scan.📝 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.
| /// Check whether a slice contains duplicate relying only on the `Eq` instance and minimizing | |
| /// allocation. The check is still in O(n*log(n)). | |
| /// | |
| /// We do not use HashMap or BTreeMap for mainly two reasons: | |
| /// | |
| /// 1. They introduce additional requirements on `K` (Hash in one case, and Ord on the other). | |
| /// 2. We want to preserve the underlying order when possible; | |
| /// | |
| /// Pre-condition: the slice is NOT empty. | |
| pub(crate) fn has_duplicate<K: Eq, V>(kvs: &[(K, V)]) -> bool { | |
| let last = kvs.len() - 1; | |
| for i in 0..last { | |
| let (k1, _) = &kvs[i]; | |
| for (k2, _) in kvs.iter().take(last + 1).skip(i + 1) { | |
| if k1 == k2 { | |
| return true; | |
| } | |
| } | |
| } | |
| false | |
| /// Check whether a slice contains duplicate relying only on the `Eq` instance and minimizing | |
| /// allocation. The check is O(n^2) due to the nested scan. | |
| /// | |
| /// We do not use HashMap or BTreeMap for mainly two reasons: | |
| /// | |
| /// 1. They introduce additional requirements on `K` (Hash in one case, and Ord on the other). | |
| /// 2. We want to preserve the underlying order when possible; | |
| /// | |
| /// Pre-condition: the slice is NOT empty. | |
| pub(crate) fn has_duplicate<K: Eq, V>(kvs: &[(K, V)]) -> bool { | |
| let last = kvs.len() - 1; | |
| for i in 0..last { | |
| let (k1, _) = &kvs[i]; | |
| for (k2, _) in kvs.iter().take(last + 1).skip(i + 1) { | |
| if k1 == k2 { | |
| return true; | |
| } | |
| } | |
| } | |
| false | |
| } |
🤖 Prompt for AI Agents
In `@crates/amaru-kernel/src/non_empty_key_value_pairs.rs` around lines 163 - 184,
The doc-comment for has_duplicate incorrectly states the algorithm is
O(n*log(n)) while the implementation is a quadratic O(n^2) nested-loop check;
update the documentation above fn has_duplicate to correctly state O(n^2) (or
"quadratic") and remove or adjust any mention of log(n) and sorting assumptions
so the comment matches the actual complexity and behavior of has_duplicate.
| /// Check whether a slice contains duplicate relying only on the `Eq` instance and minimizing | ||
| /// allocation. The check is still in O(n*log(n)). | ||
| /// | ||
| /// We do not use HashSet or BTreeSet for mainly two reasons: | ||
| /// | ||
| /// 1. They introduce additional requirements on `T` (Hash in one case, and Ord on the other). | ||
| /// 2. We want to preserve the underlying order when possible; | ||
| /// | ||
| /// Pre-condition: the slice is NOT empty. | ||
| pub(crate) fn has_duplicate<T: Eq>(xs: &[T]) -> bool { | ||
| let last = xs.len() - 1; | ||
|
|
||
| for i in 0..last { | ||
| let x1 = &xs[i]; | ||
| for x2 in xs.iter().take(last + 1).skip(i + 1) { | ||
| if x1 == x2 { | ||
| return true; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| false |
There was a problem hiding this comment.
Complexity note is off—this loop is O(n²).
The nested scan is quadratic; update the doc comment to avoid misdirection.
📝 Suggested doc fix
-/// allocation. The check is still in O(n*log(n)).
+/// allocation. The check is O(n^2) due to the nested scan.🤖 Prompt for AI Agents
In `@crates/amaru-kernel/src/non_empty_set.rs` around lines 139 - 160, The doc
comment for has_duplicate incorrectly states the complexity as O(n*log(n));
update the comment to accurately reflect the implementation's complexity as
O(n^2) due to the nested loop over the slice, keeping the existing rationale
about avoiding Hash/Ord bounds and preserving order; specifically edit the
comment block above pub(crate) fn has_duplicate<T: Eq>(xs: &[T]) -> bool to
replace "The check is still in O(n*log(n))." with a correct statement like "The
check is O(n²) (nested scan)"; do not change the implementation itself unless
you intend to optimize it, in which case note the function name has_duplicate as
the target for any algorithmic change.
- We generally want those tests to be portable and easily re-used by
other implementors.
- Regardless, it is more readable to have raw CBOR lives outside of
source code, instead of creating walls of binary data inside rust
files.
- The test code before this commit would also contain a lot of
duplication: each test was copy-pasted from the previous one,
instead of decoupling the testing code from the tested data and
fixture.
- Importantly, all the failing test scenarios were created from a
single malformed basic example (which would wrongly decode
addresses). This means that they were simply code awaiting to break
as soon the parent test would have been fixed.
While fixing these, I've also slightly improved the error messages for
the empty set / empty key value decoding errors.
Signed-off-by: KtorZ <matthias.benkort@gmail.com>
Signed-off-by: KtorZ <matthias.benkort@gmail.com>
…actionBody' This requires holding onto the hash and the original size of the deserialised body, since those are required in later validations. This is better than (and sufficient to) retaining all the bytes. Signed-off-by: KtorZ <matthias.benkort@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@crates/amaru-kernel/tests/data/decode_transaction_body/conway/new.js`:
- Around line 31-41: Validate the hex string before calling Buffer.from by
checking that the argument used for body is a non-empty even-length string
containing only [0-9a-fA-F]; if it fails, call usage(1, 'invalid hex body') to
abort. Locate the code that reads process.argv[2] into body and the
Buffer.from(body, "hex") call and add a short guard that runs a regex test and
length check before the Buffer.from conversion so malformed or odd-length inputs
are rejected.
In `@crates/amaru-ledger/src/rules/transaction/phase_one/outputs.rs`:
- Around line 74-84: The current code unconditionally counts supplemental datums
by calling context.allow_supplemental_datum(*) for any MemoizedDatum::Hash in
output.datum; change this so the call only happens for normal
(non-collateral-return) outputs. Add a boolean flag (e.g., is_collateral_return)
to the function handling outputs or otherwise branch at the call site and only
invoke context.allow_supplemental_datum(*hash) when is_collateral_return is
false; update all callers to pass false for collateral-return outputs and true
for regular outputs so collateral-return datums are not counted.
♻️ Duplicate comments (1)
crates/amaru-kernel/src/non_empty_key_value_pairs.rs (1)
165-167: Doc says O(n log n) but implementation is quadratic.This nested scan is O(n²), so the comment should reflect that.
📝 Doc fix
-/// allocation. The check is still in O(n*log(n)). +/// allocation. The check is O(n^2) due to the nested scan.
🧹 Nitpick comments (3)
crates/amaru-kernel/tests/data/decode_transaction_body/conway/new.js (1)
4-7: Preferfs.mkdirSyncover shelling out.
mkdir -pviaexecSyncis a bit Mad Max: Fury Road—loud and risky for no reason.fs.mkdirSync(..., { recursive: true })is cross‑platform and avoids the shell.🎮 Proposed refactor
-const cp = require('node:child_process'); const fs = require('node:fs'); const path = require('node:path'); @@ - cp.execSync(`mkdir -p ${id}`); + fs.mkdirSync(id, { recursive: true });Also applies to: 46-46
crates/amaru-ledger/src/rules/transaction/phase_one/outputs/inherent_value.rs (1)
20-41: Consider caching CBOR sizes to avoid double encoding.We serialize both the output and its value; if
MemoizedTransactionOutputalready caches raw bytes/sizes, reusing them could shave hot‑path work.crates/amaru-kernel/src/non_empty_key_value_pairs.rs (1)
119-137: Attach decoder position for duplicate‑key errors.You already add
.at(position)for the empty case; mirroring that here would make CBOR diagnostics clearer.♻️ Suggested tweak
- return Err(Error::message(IntoNonEmptyKeyValuePairsError::HasDuplicate)); + return Err( + Error::message(IntoNonEmptyKeyValuePairsError::HasDuplicate) + .at(d.position()), + );
4 participants
Summary by CodeRabbit
New Features
Improvements
Tests
✏️ Tip: You can customize this high-level summary in your review settings.