implement pedantic codec validations via minicbor context #707
Conversation
WalkthroughIntroduces strict decoding/validation for PositiveCoin. Refactors Conway primitives with new Multiasset, NonEmptyMap, and strict Value/TransactionBody decoding/encoding. Updates txbuilder to construct NonEmptyMultiasset mint directly. Adjusts validation to pass Multiasset representation of mint to preservation checks. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor CBOR as CBOR Data
participant Dec as Decoder
participant SV as StrictValue/StrictMultiasset
participant PC as PositiveCoin
participant TB as StrictTransactionBody
CBOR->>Dec: Feed bytes
Dec->>PC: Decode PositiveCoin
alt value == 0
PC-->>Dec: Error (invalid zero)
else value > 0
PC-->>Dec: PositiveCoin
end
Dec->>SV: Decode Multiasset/Value
SV->>SV: Validate non-empty, non-zero, size limits
Dec->>TB: Decode TxBody fields (by index)
TB->>TB: Per-field strict validation
TB-->>Dec: TransactionBody
sequenceDiagram
autonumber
actor App as TxBuilder
participant NE as NonEmptyMultiasset
participant MA as Multiasset
App->>MA: Gather mint entries (NonZeroInt)
MA-->>App: Multiasset map
App->>NE: from_multiasset(MA.into_iter().collect())
NE-->>App: NonEmptyMultiasset mint
App-->>App: Use mint in tx body
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
pallas-validate/src/phase1/conway.rs (1)
368-369: Avoid cloning mint just to borrow Multiasset
m.clone().to_multiasset()allocates a full clone. Prefer borrowing the innerMultiassetto avoid copying.Recommended:
- Add
as_multiasset(&self) -> &Multiasset<T>onNonEmptyMultiasset<T>.- Use it here to pass a borrowed
&Multiasset<_>.Apply this diff locally:
- input = conway_add_minted_non_zero(&input, &m.clone().to_multiasset(), &PostAlonzo(NegativeValue))?; + // borrow mint instead of cloning + input = conway_add_minted_non_zero(&input, m.as_multiasset(), &PostAlonzo(NegativeValue))?;And add this helper in
pallas-primitives/src/conway/model.rswithinimpl<A> NonEmptyMultiasset<A>:impl<A> NonEmptyMultiasset<A> { + pub fn as_multiasset(&self) -> &Multiasset<A> { + &self.asset + } pub fn from_multiasset(ma: Multiasset<A>) -> Option<Self> { if ma.is_empty() { None } else { Some(NonEmptyMultiasset { asset: ma, }) } } pub fn to_multiasset(self) -> Multiasset<A> { self.asset } }pallas-txbuilder/src/conway.rs (1)
157-161: Minor: simplify iterator type annotationThe explicit type on
xisn’t needed; inference is clear frommint.iter(). Consider:- .flat_map(|x: &pallas_primitives::conway::NonEmptyMultiasset<NonZeroInt>| x.iter()) + .flat_map(|x| x.iter())pallas-primitives/src/conway/model.rs (3)
30-65: StrictContext scaffolding is solid; consider DefaultThe context/error accumulator is straightforward. Optionally derive
DefaultforBasicStrictContextto ease construction alongsidenew().
945-957: NonEmptyMultiasset: add a borrow accessorTo avoid cloning in downstream callers (e.g., validation), expose a borrow:
pub fn as_multiasset(&self) -> &Multiasset<T> { &self.asset }This enables borrowing the underlying
Multiassetwhere a reference is expected.Also applies to: 967-981
1358-1394: context_bound on Block/Tx: consider Strict wrappersAdding
#[cbor(context_bound = "StrictContext")]is useful but won’t surface errors fromctx.push_errorunless a Strict wrapper is used. Consider addingStrictBlock/StrictTxwrappers mirroringStrictTransactionBody/StrictWitnessSetto fail fast in pedantic decodes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
pallas-codec/src/utils.rs(3 hunks)pallas-primitives/src/conway/model.rs(12 hunks)pallas-txbuilder/src/conway.rs(3 hunks)pallas-validate/src/phase1/conway.rs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
pallas-validate/src/phase1/conway.rs (2)
pallas-txbuilder/src/transaction/model.rs (1)
input(58-63)pallas-validate/src/utils.rs (1)
conway_add_minted_non_zero(334-355)
pallas-primitives/src/conway/model.rs (1)
pallas-codec/src/utils.rs (19)
decode(22-33)decode(148-161)decode(326-345)decode(420-430)decode(495-501)decode(537-543)decode(593-597)decode(631-637)decode(672-685)decode(758-769)decode(849-866)decode(896-917)decode(1020-1026)decode(1076-1084)decode(1173-1183)decode(1284-1296)d(151-151)d(329-329)map(1326-1336)
pallas-txbuilder/src/conway.rs (2)
pallas-validate/src/phase2/script_context.rs (1)
mint(405-411)pallas-primitives/src/conway/model.rs (1)
from_multiasset(968-976)
pallas-codec/src/utils.rs (1)
pallas-primitives/src/conway/model.rs (13)
decode(94-111)decode(123-135)decode(149-161)decode(194-210)decode(222-234)decode(365-388)decode(690-717)decode(818-854)decode(866-878)decode(909-915)decode(950-956)decode(1123-1137)decode(1264-1276)
🔇 Additional comments (10)
pallas-codec/src/utils.rs (2)
861-865: Good: enforce NonEmptySet invariant at decodeRejecting empty sets here is correct and aligns with NonEmpty semantics used across Conway types and tests.
1001-1005: PositiveCoin: manual Decode with non‑zero validationRemoving the derived Decode and introducing a custom Decode that errors on zero is the right call. It guarantees Zero is rejected at the codec boundary and integrates cleanly with stricter Multiasset decoders.
Also applies to: 1019-1027
pallas-txbuilder/src/conway.rs (1)
71-72: Mint construction via NonEmptyMultiasset looks correctCollecting into
Multiassetand wrapping withNonEmptyMultiasset::from_multiassetmatches the new primitives. No issues.pallas-primitives/src/conway/model.rs (7)
66-112: Multiasset strict decode + size checkDecoding into a
BTreeMapand pushing:
- empty policy errors, and
- a size‑cap error via
is_multiasset_small_enough
works well with the Strict wrappers. Relying on theAdecoder (e.g.,PositiveCoin/NonZeroInt) to enforce non‑zero amounts is appropriate and keeps this generic.
190-211: Value encode/decode aligns with node semanticsEncoding as coin or 2‑tuple and supporting both definite/indef arrays during decode matches Conway’s representation. Tests cover zero‑coin and empty MA cases. LGTM.
505-567: TransactionBody: custom decode validates required fieldsRequiring inputs, outputs, and fee; capturing duplicates via
ctx.push_error; and wiring strict types (NonEmptyMap/Multiasset) provides the intended pedantic behavior when used withStrictTransactionBody. Good direction.
678-679: Unknown txbody fields: confirm intended strictnessDecoding errors on unknown keys unconditionally (not just via the Strict wrappers). If backward compatibility with future fields is needed in permissive mode, consider pushing to
ctxinstead of returningErr, leaving the hard error toStrictTransactionBody.
903-916: NonEmptyMap: push error (not Err) is consistent with Strict wrappersThis lets permissive decodes proceed while
StrictTransactionBodyturns it into a hard error. Good balance.
1100-1117: TransactionOutput: datatype‑based decode is correctSwitching by CBOR datatype (array→legacy, map→post‑alonzo) is robust and mirrors long‑standing traversal logic.
Also applies to: 1119-1138
1398-1411: is_multiasset_small_enough: matches node behaviorThe size heuristic and limit are in line with the Haskell node. Tests exercise the failure path. LGTM.
| use pallas_codec::minicbor::data::Type; | ||
|
|
||
| pub type Mint = Multiasset<NonZeroInt>; | ||
| trait StrictContext { |
There was a problem hiding this comment.
Some suggestions:
- If we name this something more general, like
ValidationContext, - and have the push_error should return a Result that we can
?;
Then we can have a NoopContext, which does nothing; an AccumulatingContext which accumulates things as errors, and a TerminatingContext, which returns an error.
Then, our Strict<T> implements minicbor::Decode, constructs a TerminatingContext, and deserializes T;
And we could have a StrictVerbose<T> that implements minicbor::Decode, constructs an AccumulatingContext, and then concatenates all the errors to return it's own at the end.
2 participants
This keeps the default permissive behavior of the codecs while providing the option to enable pedantically node-conformant validations by using
minicbor::decode_with(..., BasicStrictContext::new()).See also #697, pragma-org/amaru#494
Summary by CodeRabbit