feat: cbor witnesses support #830
Conversation
📝 WalkthroughWalkthroughThe Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~15 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Fix all issues with AI Agents 🤖
In @crates/trp/src/methods.rs:
- Around line 50-70: The loop that decodes witness hex (involving
WitnessInput::Hex, hex::decode, pallas::codec::minicbor::decode, and pushing
into new_vkeys) lacks size and structural validation; add constants like
MAX_WITNESS_HEX_BYTES and MAX_TOTAL_WITNESSES, check the incoming hex string
length before hex::decode and return a clear Error::InternalError (or a new
Error::ValidationError) if it exceeds the limit, validate the decoded CBOR bytes
length before minicbor::decode, ensure the decoded WitnessSet.vkeywitness exists
and its length is within per-message and aggregate limits before extending
new_vkeys, and enforce that new_vkeys.len() never exceeds MAX_TOTAL_WITNESSES
(returning an error if it would).
- Line 72: The code currently calls tx.transaction_witness_set.unwrap() which
will panic when the transaction has no witnesses; instead handle the None case
by initializing an empty witness set (e.g., create a new TransactionWitnessSet)
and assign it to let mut witness_set so the rest of the function can proceed
safely; replace the unwrap with a match or .unwrap_or_else(...) that constructs
an empty witness set when tx.transaction_witness_set is None and keep downstream
uses of witness_set unchanged.
🧹 Nitpick comments (2)
crates/trp/src/methods.rs (2)
61-63: Preserve original error information for better debugging.The error handlers discard the original error details from hex decoding and CBOR deserialization, which would be valuable for troubleshooting invalid witness submissions.
🔎 Proposed improvement to preserve error context
- let bytes = hex::decode(h).map_err(|_| Error::InternalError("Invalid witness hex".into()))?; + let bytes = hex::decode(h).map_err(|e| Error::InternalError(format!("Invalid witness hex: {}", e)))?; let witness_set: WitnessSet = pallas::codec::minicbor::decode(&bytes) - .map_err(|_| Error::InternalError("Invalid witness set cbor".into()))?; + .map_err(|e| Error::InternalError(format!("Invalid witness set cbor: {}", e)))?;
86-86: Consider replacingunwrap()with proper error handling.While encoding a valid transaction structure should not fail, using
unwrap()could mask unexpected issues. Consider using?with a proper error conversion or.expect()with a descriptive message.🔎 Proposed improvement
- Ok(pallas::codec::minicbor::to_vec(&tx).unwrap()) + pallas::codec::minicbor::to_vec(&tx) + .map_err(|e| Error::InternalError(format!("Failed to encode transaction: {}", e)))
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/trp/src/methods.rs
⏰ 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). (1)
- GitHub Check: Check Build
🔇 Additional comments (3)
crates/trp/src/methods.rs (3)
2-5: LGTM!The addition of
WitnessSetto the imports is necessary for the new CBOR witness decoding functionality.
45-45: LGTM!The function signature correctly reflects the new
WitnessInputtype from tx3_resolver.
8-8: The importsWitnessInputandSubmitParamsfromtx3_resolver::trpare valid and available in version 0.14.2. Both types are actively used in the code (WitnessInput at lines 45, 54, and 60 for pattern matching witnesses; SubmitParams at line 93 for parameter parsing). No concerns with this import change.
| let mut new_vkeys = Vec::new(); | ||
|
|
||
| for witness in witnesses { | ||
| match witness { | ||
| WitnessInput::Object(w) => { | ||
| new_vkeys.push(VKeyWitness { | ||
| vkey: Vec::<u8>::from(w.key.clone()).into(), | ||
| signature: Vec::<u8>::from(w.signature.clone()).into(), | ||
| }); | ||
| } | ||
| WitnessInput::Hex(h) => { | ||
| let bytes = hex::decode(h).map_err(|_| Error::InternalError("Invalid witness hex".into()))?; | ||
| let witness_set: WitnessSet = pallas::codec::minicbor::decode(&bytes) | ||
| .map_err(|_| Error::InternalError("Invalid witness set cbor".into()))?; | ||
|
|
||
| if let Some(vkeys) = witness_set.vkeywitness { | ||
| new_vkeys.extend(vkeys.to_vec()); | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Consider adding size limits and validation for witness inputs.
The function accepts unbounded hex-encoded witness data without size validation, which could be exploited for denial-of-service attacks through excessive memory consumption or maliciously crafted CBOR payloads.
Consider adding:
- Maximum size limits for hex-encoded witness data
- Validation of the decoded witness set structure
- Limits on the total number of witnesses being added
#!/bin/bash
# Description: Check if there are any existing size limits or validation patterns in the codebase
echo "=== Checking for existing validation patterns ==="
rg -n "max.*size|size.*limit|validate.*witness" --type rust -C3
echo ""
echo "=== Checking error handling patterns ==="
rg -n "InternalError|Error::" crates/trp/src -C2🤖 Prompt for AI Agents
In @crates/trp/src/methods.rs around lines 50-70, The loop that decodes witness
hex (involving WitnessInput::Hex, hex::decode, pallas::codec::minicbor::decode,
and pushing into new_vkeys) lacks size and structural validation; add constants
like MAX_WITNESS_HEX_BYTES and MAX_TOTAL_WITNESSES, check the incoming hex
string length before hex::decode and return a clear Error::InternalError (or a
new Error::ValidationError) if it exceeds the limit, validate the decoded CBOR
bytes length before minicbor::decode, ensure the decoded WitnessSet.vkeywitness
exists and its length is within per-message and aggregate limits before
extending new_vkeys, and enforce that new_vkeys.len() never exceeds
MAX_TOTAL_WITNESSES (returning an error if it would).
| } | ||
| } | ||
|
|
||
| let mut witness_set = tx.transaction_witness_set.unwrap(); |
There was a problem hiding this comment.
Handle missing witness set to prevent panic.
The unwrap() will panic if the transaction doesn't have an existing witness set. Transactions without witnesses are valid in Cardano, and this function should handle that case by initializing an empty witness set.
🔎 Proposed fix to handle missing witness set
- let mut witness_set = tx.transaction_witness_set.unwrap();
+ let mut witness_set = tx.transaction_witness_set.unwrap_or_else(|| WitnessSet {
+ vkeywitness: None,
+ native_script: None,
+ bootstrap_witness: None,
+ plutus_v1_script: None,
+ plutus_data: None,
+ redeemer: None,
+ plutus_v2_script: None,
+ plutus_v3_script: None,
+ });📝 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.
| let mut witness_set = tx.transaction_witness_set.unwrap(); | |
| let mut witness_set = tx.transaction_witness_set.unwrap_or_else(|| WitnessSet { | |
| vkeywitness: None, | |
| native_script: None, | |
| bootstrap_witness: None, | |
| plutus_v1_script: None, | |
| plutus_data: None, | |
| redeemer: None, | |
| plutus_v2_script: None, | |
| plutus_v3_script: None, | |
| }); |
🤖 Prompt for AI Agents
In @crates/trp/src/methods.rs around line 72, The code currently calls
tx.transaction_witness_set.unwrap() which will panic when the transaction has no
witnesses; instead handle the None case by initializing an empty witness set
(e.g., create a new TransactionWitnessSet) and assign it to let mut witness_set
so the rest of the function can proceed safely; replace the unwrap with a match
or .unwrap_or_else(...) that constructs an empty witness set when
tx.transaction_witness_set is None and keep downstream uses of witness_set
unchanged.
Needs a new version of tx3_resolver from tx3-lang/tx3#280
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.