pedantic conway txbody #628
Conversation
WalkthroughAdds native kernel types (Block, Transaction, TransactionBody, WitnessSet), new collection abstractions (NonEmptyKeyValuePairs, NonEmptySet, KeyValuePairs), CBOR encode/decode and hashing utilities, AuxiliaryData/Metadatum handling, memoized encode/decode for scripts/data, many ledger call-site updates replacing Minted* types, and new Conway-era test vectors. (47 words) Changes
Sequence Diagram(s)sequenceDiagram
participant Client as User Code
participant Decoder as CBOR Decoder
participant BlockParser as Block::Decode
participant Hasher as Hasher
participant Iterator as Block::IntoIterator
Client->>Decoder: provide block bytes
Decoder->>BlockParser: decode header raw bytes
BlockParser->>Hasher: hash(header_raw_bytes)
Hasher-->>BlockParser: header_hash (rgba(30,144,255,0.5))
Decoder->>BlockParser: decode transaction_bodies, witnesses, auxiliary_data, invalid_transactions
BlockParser-->>Client: return Block { header, bodies, witnesses, auxiliary, invalids, sizes, header_hash }
Client->>Iterator: block.into_iter()
Iterator->>Iterator: zip bodies & witnesses, consult invalids, attach auxiliary by index
Iterator-->>Client: yield (index, Transaction)
sequenceDiagram
participant Test as Test Harness
participant TxDecoder as TransactionBody::Decode
participant FieldParser as numeric-key field handler
participant Validator as expect_field/blanket checks
participant Hasher as Hasher
Test->>TxDecoder: decode(transaction_body_cbor)
TxDecoder->>FieldParser: parse fields by numeric keys
FieldParser->>FieldParser: enforce no duplicate keys
FieldParser-->>TxDecoder: accumulated fields + raw range
TxDecoder->>Hasher: hash(original_header_bytes) (rgba(34,139,34,0.5))
TxDecoder->>Validator: verify required fields present
Validator-->>Test: TransactionBody { id/hash, original_size, fields }
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
2 times, most recently
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!
| 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.
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.
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()), + );
The original idea for the amaru-kernel was to be built on top of Pallas. However, implementations have proven to be incomplete and unfit to enforce invariants needed throughout the Amaru code base. So we've decided to move away from Pallas and internalize core primitives and types needed of Amaru. This is one of the first iteration of this effort. Co-authored-by: KtorZ <matthias.benkort@gmail.com> Signed-off-by: ruko <rukokarasu@gmail.com>
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>
- 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>
KtorZ
force-pushed
the
rrruko/pedantic-txbody-2
branch
from
36ca249 to
f915a08
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@crates/amaru-kernel/src/block.rs`:
- Around line 121-138: When decoding a Block (after producing
transaction_bodies, transaction_witnesses, auxiliary_data, and
invalid_transactions in the decode path), validate that transaction_bodies.len()
== transaction_witnesses.len() and that every index referenced by auxiliary_data
and invalid_transactions is within the range of transaction_bodies; if any
mismatch or out‑of‑bounds index is found, return a decode error rather than
silently zipping/ignoring items. Update the Block decode logic (the function
that constructs Block and computes original_body_size/original_header_size and
header_hash) to perform these checks and yield a clear error on failure so
malformed blocks are rejected.
- Around line 112-133: The captured start_position is taken before the
heterogeneous_array header, so header_hash and original_header_size wrongly
include the array header bytes; inside the heterogeneous_array closure (the
closure passed to heterogeneous_array in this block decoding code) capture a new
header_start = d.position() immediately before decoding header (instead of using
the outer start_position) and use header_start and end_position_header to
compute header_hash and original_header_size for the Block struct (keep
original_body_size calculation as-is); update references to start_position ->
header_start when computing header-related fields (symbols: heterogeneous_array
closure, header_start/header_hash, original_header_size, end_position_header).
In `@crates/amaru-kernel/src/key_value_pairs.rs`:
- Around line 158-180: The doc comment for has_duplicate is incorrect: the
implementation uses a double loop (nested iteration in has_duplicate) so its
time complexity is O(n²), not O(n*log(n)); update the comment to state O(n²) and
explain that an alternative is to use a HashSet-based approach that requires K:
Hash + Eq if you want O(n) average time. If you prefer changing implementation
instead, modify has_duplicate to use a HashSet<K> (requiring K: Hash + Eq) to
deduplicate in O(n) average time and mention the trait change in the function
signature and docs.
In `@crates/amaru-kernel/src/transaction_body.rs`:
- Around line 227-235: The inputs and outputs fields on TransactionBody should
enforce non-empty at decode time like certificates/collateral: change the types
of inputs and outputs in TransactionBody to NonEmptySet<TransactionInput> and
NonEmptyVec<TransactionOutput> (or your project's equivalent non-empty
collection types), update any decoder/state fields (state.required.inputs and
state.required.outputs) to produce those non-empty types, and replace the
current expect_field(...) calls for inputs/outputs with calls that
validate/convert to the NonEmpty types so decoding fails on empty collections;
ensure downstream code that consumes TransactionBody is adjusted to use the
non-empty collection APIs.
🧹 Nitpick comments (8)
crates/amaru-kernel/tests/data/decode_transaction_body/conway/new.js (2)
46-46: Preferfs.mkdirSyncover shelling out.Using
cp.execSyncfor a simple directory creation is like bringing a BFG to a knife fight – bit overkill, yeah? Node'sfs.mkdirSyncwith{ recursive: true }does the same job, is more portable (looking at you, Windows), and sidesteps any shell-related shenanigans.♻️ Proposed fix
- cp.execSync(`mkdir -p ${id}`); + fs.mkdirSync(id, { recursive: true });And since you won't need
child_processanymore, you can drop that import:const blake2b = require('blake2b'); -const cp = require('node:child_process'); const fs = require('node:fs');
48-48: Consider pretty-printing the JSON for readability.When you're debugging test failures at 2am (we've all been there, like that one Water Temple moment), having a nicely formatted
meta.jsonis a small kindness to future-you.✨ Optional tweak
- fs.writeFileSync(path.join(id, "meta.json"), JSON.stringify({well_formed: isValid, description })); + fs.writeFileSync(path.join(id, "meta.json"), JSON.stringify({ well_formed: isValid, description }, null, 2));crates/amaru-kernel/src/key_value_pairs.rs (1)
126-133: FIXME noted – protocol version handling is on the radar.The current strict duplicate rejection is the safest default. When you get around to implementing the version-specific behavior, you'll need access to protocol version context during decode. Might be worth tracking this in an issue so it doesn't get lost in the shuffle!
Would you like me to open an issue to track the protocol version-specific duplicate key handling?
crates/amaru-kernel/src/metadatum.rs (1)
67-84: Follow up on chunked encoding for >64 metadata.
Per the FIXME, if we ever re‑serialize metadata for block production, chunked bytes/text (>64) are expected by the ledger; worth wiring this before release. Happy to help.crates/amaru-ledger/src/rules/transaction/phase_one/collateral.rs (2)
27-32: Track the TODO for merging CollateralBalance into Value. Worth logging as a follow‑up so it doesn’t hang around like an unfinished side quest.If you want, I can draft an issue or sketch a merge plan.
324-329: Consider replacing the commented‑out test with a decode‑level assertion. Since Conway txs can’t have empty collateral, a focused decode test would keep coverage without keeping dead code around.crates/amaru-ledger/src/rules/transaction/phase_one/metadata.rs (1)
24-30: Keep hash sizes consistent in error variants.
MissingTransactionAuxiliaryDataHash now keys offAuxiliaryData::HASH_SIZE, butConflictingMetadataHashstill hardcodes32. Aligning the latter avoids drift if the hash size ever changes — think “director’s cut” continuity.♻️ Suggested tweak
ConflictingMetadataHash { - supplied: Hash<32>, - expected: Hash<32>, + supplied: Hash<{ AuxiliaryData::HASH_SIZE }>, + expected: Hash<{ AuxiliaryData::HASH_SIZE }>, },crates/amaru-kernel/src/transaction_body.rs (1)
240-256: Attach decode position to duplicate‑field errors.Other decode paths include positions; adding it here would make fixture debugging a bit less “Where’s‑Waldo.”
🧭 Add position to duplicate‑field errors
- if field.is_some() { - return Err(cbor::decode::Error::message(format!( - "duplicate field entry with key {k}" - ))); - } + if field.is_some() { + let position = d.position(); + return Err(cbor::decode::Error::message(format!( + "duplicate field entry with key {k}" + )) + .at(position)); + }
KtorZ
force-pushed
the
rrruko/pedantic-txbody-2
branch
from
f915a08 to
2f9814d
Compare
…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>
Signed-off-by: KtorZ <matthias.benkort@gmail.com>
Those are no longer necessary, now that we are slowly moving towards fully owning the primitive types. We have more freedom about the information we need to keep around which are usually only hashes and original sizes. Anything else can be ignored. Signed-off-by: KtorZ <matthias.benkort@gmail.com>
…decoding. Signed-off-by: KtorZ <matthias.benkort@gmail.com>
Signed-off-by: KtorZ <matthias.benkort@gmail.com>
KtorZ
force-pushed
the
rrruko/pedantic-txbody-2
branch
from
2f9814d to
5e95868
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/amaru-kernel/src/memoized/native_script.rs (1)
19-27: Addinto = "String"to align Serialize with Deserialize.
MemoizedNativeScriptdeserializes from hex strings viatry_from = "&str", but without aninto = "String"attribute, serde will serialize it as a struct map instead. This breaks JSON round-trips—you'd get back a different shape than what you put in. The codebase already has the pattern down cold withMemoizedPlutusData, so just mirror that approach:Suggested fix
#[derive(Debug, Clone, PartialEq, Eq, serde::Deserialize, serde::Serialize)] -#[serde(try_from = "&str")] +#[serde(try_from = "&str", into = "String")] pub struct MemoizedNativeScript {Then add the
Fromimpl:impl From<MemoizedNativeScript> for String { fn from(script: MemoizedNativeScript) -> Self { hex::encode(&script.original_bytes[..]) } }
🤖 Fix all issues with AI agents
In `@examples/shared/src/lib.rs`:
- Line 31: The type alias BlockWrapper<'b> unnecessarily declares a lifetime
parameter 'b even though it contains an owned Block; remove the unused lifetime
by changing the alias to type BlockWrapper = (u16, Block) and update any usages
that reference BlockWrapper<'b> to the new BlockWrapper name to satisfy the
nonstandard-style lint.
🧹 Nitpick comments (3)
crates/amaru-kernel/src/key_value_pairs.rs (1)
57-67: Early return on empty vec is redundant.The
!vec.is_empty() && has_duplicate(...)guard meanshas_duplicateis never called on empty vecs, which is good since it has a precondition that the slice is non-empty. However, an empty vec trivially has no duplicates, so you could simplify to justif has_duplicate(...)if you madehas_duplicatehandle empty slices gracefully. Not a blocker though – the current approach is defensive and works fine.crates/amaru-plutus/src/script_context/mod.rs (1)
899-922: Bin the commented-out legacy impl.
Keeping the oldTryFromblock as comments is a bit of clutter—better to delete and lean on history.🧹 Suggested cleanup
-// impl<'a> TryFrom<&'a MintedTransactionOutput<'a>> for TransactionOutput<'a> { -// type Error = TransactionOutputError; -// -// fn try_from( -// output: &'a MintedTransactionOutput<'a>, -// ) -> Result<TransactionOutput<'a>, Self::Error> { -// match output { -// MintedTransactionOutput::Legacy(output) => Ok(TransactionOutput { -// is_legacy: true, -// address: Cow::Owned(Address::from_bytes(&output.address)?), -// value: (&output.amount).try_into()?, -// datum: output.datum_hash.as_ref().into(), -// script: None, -// }), -// MintedTransactionOutput::PostAlonzo(output) => Ok(TransactionOutput { -// is_legacy: false, -// address: Cow::Owned(Address::from_bytes(&output.address)?), -// value: (&output.value).into(), -// script: output.script_ref.as_ref().map(Script::from), -// datum: output.datum_option.as_ref().into(), -// }), -// } -// } -// }crates/amaru-ledger/src/rules/block.rs (1)
207-244: Transaction iteration and phase execution.Iterating with
for (i, transaction) in blockis slick – the Block'sIntoIteratoryields(u32, Transaction)tuples.One thing to flag: at line 233, you're doing
transaction.body.clone()becausephase_one::executetakes ownership of theTransactionBody. This clone happens for every transaction in the block. In a high-throughput scenario, this could add up. It's not a blocker – just something to keep an eye on if performance profiling ever points here. Like when you realise you've been fast-traveling with encumbrance penalties the whole game!The witness and auxiliary data are passed by reference, which is good.
4 participants
Summary by CodeRabbit
New Features
Refactor
Tests
✏️ Tip: You can customize this high-level summary in your review settings.