perf: simplify decoding logic#632
Conversation
Signed-off-by: Jacob Finkelman <YeomanYaacov@gmail.com>
WalkthroughSwitches from a boxed-closure PartialDecoder pattern to an Option-per-field decoding model for heterogeneous CBOR maps; updates decoding sites to assign Some(...) for address, value, datum, and script, and converts missing-field handling to return concrete CBOR decode errors by field index. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧠 Learnings (1)📚 Learning: 2025-12-16T21:32:37.668ZApplied to files:
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
🔇 Additional comments (4)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/amaru-minicbor-extra/src/decode.rs (1)
284-305: Hold up—missing theSomewrapper here, chief.Lines 292-293 show direct assignments like
state.0 = d.decode_with(ctx)?, but your initial state at line 288 is(None::<u8>, None::<u8>), meaning both fields areOption<u8>. To set an Option to a value, you needSome(...)wrapping, not direct assignment.Looking at the other file (transaction_output.rs line 105), the pattern is
state.0 = Some(decode_address(...)), which is the correct approach.🐛 Proposed fix: Wrap decoded values in Some
match field { - 0 => state.0 = d.decode_with(ctx)?, - 1 => state.1 = d.decode_with(ctx)?, + 0 => state.0 = Some(d.decode_with(ctx)?), + 1 => state.1 = Some(d.decode_with(ctx)?), _ => return unexpected_field::<Foo, _>(field), }crates/amaru-kernel/src/memoized/transaction_output.rs (1)
99-122: Decoding logic is solid, but there's some redundant syntax to clean up.The Option-based approach here is implemented correctly:
- Required fields (address, value) start as
None, get wrapped inSome(...), then validated withok_or_else- The datum field uses
MemoizedDatum::Noneas its default and gets assigned directly- Optional script field follows the same None/Some pattern
However, the pipeline caught some redundant field names in your struct initialization at lines 119-120. When the variable name matches the field name, Rust lets you use the shorthand syntax.
✨ Proposed fix: Use field init shorthand
Ok(MemoizedTransactionOutput { is_legacy: false, address: address.ok_or_else(|| missing_field::<MemoizedTransactionOutput, Address>(0))?, value: value.ok_or_else(|| missing_field::<MemoizedTransactionOutput, Value>(1))?, - datum: datum, - script: script, + datum, + script, })
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
crates/amaru-kernel/src/memoized/transaction_output.rscrates/amaru-minicbor-extra/src/decode.rs
🧰 Additional context used
🧠 Learnings (5)
📚 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-minicbor-extra/src/decode.rscrates/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-minicbor-extra/src/decode.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-minicbor-extra/src/decode.rs
📚 Learning: 2025-04-20T17:57:23.233Z
Learnt from: rkuhn
Repo: pragma-org/amaru PR: 149
File: crates/amaru/src/stages/consensus/chain_forward/test_infra.rs:272-285
Timestamp: 2025-04-20T17:57:23.233Z
Learning: In test infrastructure code, rkuhn prefers explicit panics (using .unwrap() or similar) over returning Result types, as test failures should be immediate and obvious.
Applied to files:
crates/amaru-minicbor-extra/src/decode.rs
📚 Learning: 2025-12-16T21:32:37.668Z
Learnt from: rkuhn
Repo: pragma-org/amaru PR: 584
File: crates/amaru-network/src/handshake/tests.rs:40-47
Timestamp: 2025-12-16T21:32:37.668Z
Learning: In Rust, shadowing a binding with a new let does not drop the previous binding until the end of the scope. All shadowed bindings in a scope are dropped in reverse-declaration order when the scope ends. Therefore, multiple let _guard = register_*() calls will keep all guards alive until the end of the function (or the surrounding scope). When reviewing code, be mindful that resources tied to shadowed bindings persist longer than the most recent binding; to release early, constrain the lifetime in an inner block or explicitly drop guards when appropriate.
Applied to files:
crates/amaru-minicbor-extra/src/decode.rscrates/amaru-kernel/src/memoized/transaction_output.rs
🪛 GitHub Actions: Coding Practices
crates/amaru-kernel/src/memoized/transaction_output.rs
[error] 119-119: redundant field names in struct initialization. replace 'datum: datum' with 'datum'.
[error] 120-120: redundant field names in struct initialization. replace 'script: script' with 'script'.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: End-to-end snapshot tests (preprod, 1, 182, 15, 10.5.3)
- GitHub Check: End-to-end snapshot tests (preview, 2, 680, true, 15, 10.5.3)
- GitHub Check: Build aarch64/macos
- GitHub Check: Build x86_64/linux
- GitHub Check: Test the CLI
- GitHub Check: Build aarch64/linux
- GitHub Check: Build riscv32
- GitHub Check: Build x86_64/windows
- GitHub Check: Benches
- GitHub Check: Build wasm32
- GitHub Check: Test coverage
- GitHub Check: Analyze (rust)
🔇 Additional comments (5)
crates/amaru-minicbor-extra/src/decode.rs (4)
81-100: Documentation's spot on, mate!The updated example clearly shows the new Option-based flow. It's like going from a convoluted Rube Goldberg machine to a straight shot—much cleaner for future devs reading this.
150-153: Test imports cleaned up nicely.Gone are the old PartialDecoder helpers, in with the simplified API. Like Marie Kondo-ing your imports—only keeping what sparks joy (and compiles).
310-328: Default values pattern looks bang on.This test shows the alternative approach: start with concrete defaults (
14_u8, 42_u8) and overwrite them during decoding. No Options needed since you've got fallback values from the jump. Clean implementation for optional fields with defaults.
126-133: The API narrowing concern doesn't hold—all call sites are already passing u8 values.Fair point to double-check, but you can rest easy on this one. I've traced all the call sites of
missing_fieldacross the codebase, and they're all feeding it u8 literals (0, 1, etc.). Two instances in the test suite and two more intransaction_output.rs—all u8, all day. No strings, no fancy enums, no shenanigans.So while the parameter type restriction is technically narrower on paper, it's not a practical breaking change for the existing code. The optimization lands cleanly without tripping up the rest of the codebase.
crates/amaru-kernel/src/memoized/transaction_output.rs (1)
21-21: Imports aligned with the new decoding API.Out with the old PartialDecoder helpers, in with the streamlined Option-based approach. Straightforward migration.
Codecov Report✅ All modified and coverable lines are covered by tests.
... and 6 files with indirect coverage changes 🚀 New features to boost your workflow:
|
KtorZ
left a comment
KtorZ
left a comment
There was a problem hiding this comment.
sigh ... So it's all indeed a lie when they say Rust is functional programming language; because it always resort to mutating structures rather than composing functions :(
Thanks for ALSO updating the docs 🫶
2 participants
The first commit was noticed while doing memory profiling.
missing_fieldwas preemptively constructing the error message in case a field was missing, even though most of the time it turned out to be present.format!is surprisingly expensive, so reducing its use on the hot path was a clear win.With that fixed, the
Box<fn ...>in thePartialDecoderalso looked suspicious. It was being used only to track whether the fields of a Struct had been read from CBOR yet and whether a missing field should be an error or a default value.Optioncan work keeping track of this info. Using the Builder Pattern might be an even more idiomatic solution in this case, but this change seemed the least invasive.These kinds of short lived allocations sometimes anger allocators. Having now made these changes and tested the result, no connection. These changes do not make a measurable difference to peak memory usage or throughput. 🙁 It does reduce the number of allocations and thereby the overhead of using memory profiling tools. It should not be too hard to collect quantitative results if requested.
Summary by CodeRabbit
Refactor
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.