Lots of fixes and updated to the latest node version#624
Open
ginnun wants to merge 17 commits into
Open
Conversation
scarmuega
reviewed
Apr 1, 2025
|
|
||
| pub type RewardAccount = Bytes; | ||
| #[derive(Debug, Clone, Eq, PartialEq, PartialOrd, Ord)] | ||
| pub struct FieldedRewardAccount{ |
Member
There was a problem hiding this comment.
is the FieldedRewardAccount struct defined as part of some spec or is it a quality-of-life improvement for dealing with credentials?
If its the latter, I would prefer to have this in a higher layer of the stack and not as part of the codec of the miniprotocol.
Contributor
Author
There was a problem hiding this comment.
@scarmuega It's there to be able to produce the same error message with Haskell codebase. Since Rust doesn't have type specialization, old-good RewardAccount (which is a Byte) can't be used for HaskellDisplay to distinguish.
So not sure if this is a quality-of-life improvement or not. If do you think so, I could refactor it. Just tell me what layer it should be in that case. Thanks.
ginnun
changed the title
Contributor
Author
|
Please note that failing |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds FieldedRewardAccount (CBOR Encode/Decode), Plutus v4 cost-model support, new mismatch container MismatchArr, public DecoderError/DeserialiseFailure, manual minicbor Decode for Tx<'b>, widespread replacement of DisplayRewardAccount with FieldedRewardAccount, and numerous ordering/typing adjustments across network, protocol, display, and error layers. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant CBOR as "CBOR Decoder"
participant TxDec as "Tx<'b>::Decode"
participant Body as "TransactionBody"
participant Wits as "WitnessSet"
CBOR->>TxDec: start decode()
TxDec->>CBOR: read array header
TxDec->>Body: decode(transaction_body) with KeepRaw context
TxDec->>Wits: decode(witness_set) with KeepRaw context
alt success field present
TxDec->>CBOR: decode bool success
else
Note right of TxDec: success defaults to true
end
alt auxiliary_data present
TxDec->>CBOR: decode Nullable<AuxiliaryData>
else
Note right of TxDec: auxiliary_data defaults to Null
end
TxDec-->>CBOR: return Tx
sequenceDiagram
autonumber
participant Enc as "Encoder"
participant FE as "FieldedRewardAccount"
participant Buf as "Raw CBOR Bytes"
Enc->>FE: request serialization (Encode)
FE-->>Enc: provide 29-byte payload (1-byte header + 28-byte stake hash)
Enc->>Buf: write raw bytes
Buf->>FE: raw bytes read (Decode)
FE->>FE: TryFrom<&[u8]> parsing and validate 29 bytes
FE-->>Buf: FieldedRewardAccount instance
sequenceDiagram
autonumber
participant Codec as "localtxsubmission::codec"
participant Stream as "Input Stream"
participant Err as "DecoderError"
Codec->>Stream: attempt to decode error payload
alt decodes known error tag (incl. VRFKeyHashAlreadyRegistered)
Stream-->>Codec: Ok(error struct)
Codec-->>Codec: return Some(error)
else
Stream-->>Codec: Err(e)
Codec->>Err: propagate/convert to DecoderError variants
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
pallas-codec/src/utils.rs(1 hunks)pallas-hardano/src/display/haskell_display.rs(29 hunks)pallas-hardano/src/display/haskell_error.rs(5 hunks)pallas-network/src/miniprotocols/localstate/queries_v16/codec.rs(1 hunks)pallas-network/src/miniprotocols/localstate/queries_v16/mod.rs(14 hunks)pallas-network/src/miniprotocols/localtxsubmission/codec.rs(0 hunks)pallas-network/src/miniprotocols/localtxsubmission/primitives.rs(3 hunks)pallas-network/src/miniprotocols/localtxsubmission/protocol.rs(10 hunks)pallas-primitives/src/conway/model.rs(2 hunks)
💤 Files with no reviewable changes (1)
- pallas-network/src/miniprotocols/localtxsubmission/codec.rs
🧰 Additional context used
🧬 Code graph analysis (3)
pallas-network/src/miniprotocols/localtxsubmission/protocol.rs (1)
pallas-network/src/miniprotocols/localstate/queries_v16/mod.rs (8)
from(635-654)from(844-846)from(908-910)from(914-916)from(920-922)from(929-931)from(935-937)from(958-960)
pallas-primitives/src/conway/model.rs (1)
pallas-codec/src/utils.rs (18)
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(1065-1073)decode(1162-1172)decode(1273-1285)decode(1340-1355)d(151-151)d(329-329)
pallas-hardano/src/display/haskell_display.rs (3)
pallas-hardano/src/display/haskells_show_string.rs (1)
haskell_show_string(4-98)pallas-network/src/miniprotocols/localtxsubmission/codec.rs (16)
encode(39-54)encode(62-87)encode(130-140)encode(154-169)encode(184-194)encode(232-253)encode(320-328)encode(344-353)encode(384-402)encode(413-420)encode(435-446)encode(468-509)encode(632-766)encode(784-795)encode(842-872)encode(899-919)pallas-traverse/src/governance.rs (1)
reward_account(24-28)
⏰ 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). (4)
- GitHub Check: Test Suite (windows-latest, stable)
- GitHub Check: Test Suite (macOS-latest, stable)
- GitHub Check: Check (windows-latest, stable)
- GitHub Check: Test Suite (ubuntu-latest, stable)
🔇 Additional comments (1)
pallas-codec/src/utils.rs (1)
1342-1348: LGTM – approve compatibility fix, verify Nullable decoding
The change fromd.null()/d.undefined()tod.skip()is safe given the priord.datatype()?check. Please run your pallas-codec tests locally (e.g.cargo test -p pallas-codec) to confirm no regressions in Nullable decoding.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
pallas-network/src/miniprotocols/localtxsubmission/codec.rs (1)
335-347: Consider validating the CBOR array length for robustness.The
Decodeimplementation callsd.array()?but discards the returned length. SinceMismatchArris explicitly documented as a 2-element array, validating the length would catch malformed input early (e.g., extra elements would be silently ignored with the current implementation).That said, this pattern of not checking length is common in this codebase (e.g.,
VotingProcedure::decode), so this is a minor suggestion.🔧 Optional: Add length validation
impl<'b, T, C> Decode<'b, C> for super::MismatchArr<T> where T: Decode<'b, C>, { fn decode(d: &mut Decoder<'b>, ctx: &mut C) -> Result<Self, decode::Error> { - d.array()?; + let len = d.array()?; + if len != Some(2) { + return Err(decode::Error::message("Expected array of length 2 for MismatchArr")); + } let t0 = d.decode_with(ctx)?; let t1 = d.decode_with(ctx)?; Ok(super::MismatchArr(t0, t1)) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pallas-network/src/miniprotocols/localtxsubmission/codec.rs` around lines 335 - 347, The Decode impl for super::MismatchArr currently calls d.array()? but ignores the returned length; update it to capture the length (let len = d.array()?) and validate that it is exactly 2 when fixed-length (or allow None for indefinite-length CBOR), returning a decode error for any Some(n) where n != 2; then proceed to call d.decode_with(ctx)? twice and return super::MismatchArr(t0, t1). Ensure you reference the same symbols: Decode for super::MismatchArr, d.array(), d.decode_with(ctx) and super::MismatchArr(t0, t1) when implementing the check.pallas-network/src/miniprotocols/localstate/queries_v16/mod.rs (1)
460-524: Consider reducing duplication betweenProtocolParamandCurrentProtocolParam
CurrentProtocolParamappears to have the same fields asProtocolParam. If they are semantically identical, consider using a type alias or extracting a shared base to reduce maintenance burden. If there are subtle differences (e.g., different CBOR encoding requirements as hinted by the TODO on line 392), please add a comment explaining why both are needed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pallas-network/src/miniprotocols/localstate/queries_v16/mod.rs` around lines 460 - 524, CurrentProtocolParam duplicates ProtocolParam field-for-field; either unify them by replacing CurrentProtocolParam with a type alias to ProtocolParam or extract a shared struct used by both (e.g., rename ProtocolParam -> ProtocolParamBase and make ProtocolParam and CurrentProtocolParam newtype wrappers), and update usages to the chosen shared type; if there are encoding differences (see the TODO about CBOR encoding near the ProtocolParam implementation), add a clear comment on why two distinct types must remain and document the specific encoding/semantic difference on ProtocolParam and CurrentProtocolParam so future maintainers understand the rationale.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pallas-hardano/src/display/haskell_display.rs`:
- Around line 526-535: The fallback use of Debug in DecoderError::to_haskell_str
breaks the Haskell wire format; replace the catch-all "{:?}" branch with
explicit, stable Haskell-style string formatting for every DecoderError variant.
Update the impl HaskellDisplay for DecoderError::to_haskell_str to match each
variant (not just DecoderError::DeserialiseFailure) and return a deterministic
string (e.g., "DecoderErrorVariantName <fields...>") using existing helper
methods like to_haskell_str()/to_haskell_str_p() where appropriate so all
variants produce stable, predictable output.
- Line 397: The TooManyCollateralInputs message is passing the arguments to
as_mismatch in the wrong order (as_mismatch expects (expected, supplied));
change the call in the TooManyCollateralInputs formatter from
as_mismatch(actual, max) to as_mismatch(max, actual) and make the same swap for
the other occurrences referenced (the similar block around the 664-672 range) so
the serialized mismatch uses expected=max and supplied=actual.
In `@pallas-hardano/src/display/haskell_error.rs`:
- Around line 22-29: The helper as_cbor_decode_failure currently panics by
calling unwrap() on serde_json::to_string; change its signature from fn
as_cbor_decode_failure(message: String, position: u64) -> String to return
Result<String, serde_json::Error>, remove the unwrap and return the Result from
serde_json::to_string(&error), and update any callers of as_cbor_decode_failure
to handle the Result (propagate or map_err as appropriate). Keep the
construction of DecoderError::DeserialiseFailure, TxCmdError::TxReadError and
TxSubmitFail::TxSubmitFail unchanged — only alter the return type and remove the
unwrap so serialization errors are propagated.
---
Nitpick comments:
In `@pallas-network/src/miniprotocols/localstate/queries_v16/mod.rs`:
- Around line 460-524: CurrentProtocolParam duplicates ProtocolParam
field-for-field; either unify them by replacing CurrentProtocolParam with a type
alias to ProtocolParam or extract a shared struct used by both (e.g., rename
ProtocolParam -> ProtocolParamBase and make ProtocolParam and
CurrentProtocolParam newtype wrappers), and update usages to the chosen shared
type; if there are encoding differences (see the TODO about CBOR encoding near
the ProtocolParam implementation), add a clear comment on why two distinct types
must remain and document the specific encoding/semantic difference on
ProtocolParam and CurrentProtocolParam so future maintainers understand the
rationale.
In `@pallas-network/src/miniprotocols/localtxsubmission/codec.rs`:
- Around line 335-347: The Decode impl for super::MismatchArr currently calls
d.array()? but ignores the returned length; update it to capture the length (let
len = d.array()?) and validate that it is exactly 2 when fixed-length (or allow
None for indefinite-length CBOR), returning a decode error for any Some(n) where
n != 2; then proceed to call d.decode_with(ctx)? twice and return
super::MismatchArr(t0, t1). Ensure you reference the same symbols: Decode for
super::MismatchArr, d.array(), d.decode_with(ctx) and super::MismatchArr(t0, t1)
when implementing the check.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
pallas-codec/src/utils.rspallas-hardano/src/display/haskell_display.rspallas-hardano/src/display/haskell_error.rspallas-network/src/miniprotocols/localstate/queries_v16/codec.rspallas-network/src/miniprotocols/localstate/queries_v16/mod.rspallas-network/src/miniprotocols/localtxsubmission/codec.rspallas-network/src/miniprotocols/localtxsubmission/primitives.rspallas-network/src/miniprotocols/localtxsubmission/protocol.rspallas-primitives/src/conway/model.rspallas-validate/tests/conway.rs
michalrus
reviewed
Mar 24, 2026
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pallas-network/src/miniprotocols/localstate/queries_v16/mod.rs`:
- Around line 639-679: The current TryFrom<&[u8]> impl for FieldedRewardAccount
only validates the upper nibble (header_type) but reads the network via bytes[0]
& 0b0000_0001, which accepts non-canonical values like 0xe2/0xf3; update the
header/network validation in TryFrom to also verify the full lower nibble
(bytes[0] & 0x0F) is exactly the canonical values (0x0 for Testnet or 0x1 for
Mainnet) and return InvalidRewardAccount::InvalidHeaderType (or add a new
InvalidNetwork variant) when it is not; locate the check around header_type and
bytes[0] in the TryFrom impl and replace the single-bit test with a strict
lower-nibble match before mapping to Network.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: dffa7f09-ba08-4f2e-be56-5f72beb877c8
📒 Files selected for processing (3)
pallas-network/src/miniprotocols/localstate/queries_v16/codec.rspallas-network/src/miniprotocols/localstate/queries_v16/mod.rspallas-network/src/miniprotocols/localtxsubmission/protocol.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- pallas-network/src/miniprotocols/localstate/queries_v16/codec.rs
michalrus
added a commit
to blockfrost/blockfrost-platform
that referenced
this pull request
Apr 9, 2026
michalrus
approved these changes
Apr 9, 2026
Contributor
michalrus
left a comment
michalrus
left a comment
There was a problem hiding this comment.
The CBOR tests are passing in:
scarmuega
reviewed
May 9, 2026
|
|
||
| impl Eq for Tx<'_> {} | ||
|
|
||
| impl<'b, C> minicbor::Decode<'b, C> for Tx<'b> |
Member
There was a problem hiding this comment.
Could you walk me through what motivated this custom Decode for Tx<'b>?
let success = match d.decode_with(ctx) {
Ok(v) => v,
Err(e) if e.is_end_of_input() => true,
Err(e) => return Err(e),
};
let auxiliary_data = match d.decode_with(ctx) {
Ok(v) => v,
Err(e) if e.is_end_of_input() => Nullable::Null,
Err(e) => return Err(e),
};is it required at the CDDL level for conway?
Contributor
Author
There was a problem hiding this comment.
Good catch, you're right, the Conway CDDL doesn't require this.
The original idea behind the manual impl was tolerance for pre-Alonzo tx layouts ([body, witness_set, auxiliary_data / nil] and no isValid)
Decoding should fail if it's not a four-element array.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR contains lots of fixes, improvements and fixes some breaking changes that comes with the latest Cardano node version (for Haskell text generation)
Summary by CodeRabbit
New Features
Changes
Tests