feat(core): implement ReturnValueDecoder and FunctionCallDecoder for … - #384
Conversation
…contract return values
📝 WalkthroughWalkthroughContract specifications now preserve typed function, struct, enum, and union metadata. ChangesTyped Soroban decoding
Application error utilities
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ContractSpec
participant ReturnValueDecoder
participant ScVal
ContractSpec->>ReturnValueDecoder: provide function return type
ScVal->>ReturnValueDecoder: provide raw return value
ReturnValueDecoder->>ReturnValueDecoder: decode typed or dynamic value
ReturnValueDecoder-->>ContractSpec: return JSON value
Possibly related issues
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Comment |
|
@nazteeemba Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
![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
🧹 Nitpick comments (3)
crates/core/src/decode/return_decoder.rs (1)
437-543: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the enum and union paths.
decode_enumanddecode_unionare the most branch-heavy logic here (value-type tuples, struct-shaped cases, unmatched variants) and are currently untested.Want me to draft those test cases?
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/core/src/decode/return_decoder.rs` around lines 437 - 543, Extend the tests module with coverage for ReturnValueDecoder::decode_enum and decode_union, including value-type tuple variants, struct-shaped variants, and unmatched variant behavior. Build the necessary ScSpecTypeDef enum/union definitions and ScVal inputs, then assert the decoded JSON for each supported case and the expected fallback for unknown variants.crates/core/src/decode/function_call_decoder.rs (1)
72-108: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the value→string formatting into one helper.
The same
match v { Value::String(s) => s.clone(), other => to_string(other) }block appears three times here and again inReturnValueDecoder::decode_to_string.♻️ Proposed refactor
+fn format_value(v: &Value) -> String { + match v { + Value::String(s) => s.clone(), + other => serde_json::to_string(other).unwrap_or_else(|_| other.to_string()), + } +}Then each site becomes
decoded_args.iter().map(format_value).collect().🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/core/src/decode/function_call_decoder.rs` around lines 72 - 108, Extract the repeated Value-to-String conversion into a shared formatting helper, including the existing logic in ReturnValueDecoder::decode_to_string. Update both formatted argument maps and formatted return-value construction in the current decoder to call that helper, preserving string cloning and JSON serialization with the existing fallback.crates/core/src/spec/decoder.rs (1)
181-279: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: extract the repeated doc-normalization into a helper.
The
if doc.is_empty() { None } else { Some(doc.to_string()) }pattern is now duplicated across function, error enum, enum, union, union-case, and struct-field paths.♻️ Suggested helper
fn opt_doc(doc: &impl ToString) -> Option<String> { let s = doc.to_string(); if s.is_empty() { None } else { Some(s) } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/core/src/spec/decoder.rs` around lines 181 - 279, Optionally add a shared doc-normalization helper near the decoder utilities, such as opt_doc, that converts empty documentation to None and non-empty documentation to Some(String). Replace the duplicated normalization blocks across function, error enum, enum, union, union-case, and struct-field decoding paths, including the visible UdtEnumV0 and UdtUnionV0 branches, while preserving existing output.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/core/src/decode/function_call_decoder.rs`:
- Around line 154-164: Update the decode_function_call test inputs to match the
declared Address and I128 parameter definitions, replacing the Symbol and I32
ScVals with corresponding typed values. Add assertions that verify the decoded
argument contents, while preserving the existing function name, return value,
and formatted return value checks.
In `@crates/core/src/decode/return_decoder.rs`:
- Around line 352-357: Update the None branch in the variant decoding logic to
validate that v contains a second element before accessing v[1]. For a
one-element unmatched void variant, return the appropriate safe decoded
representation instead of panicking, while preserving the existing dynamic
decoding behavior when the payload is present.
---
Nitpick comments:
In `@crates/core/src/decode/function_call_decoder.rs`:
- Around line 72-108: Extract the repeated Value-to-String conversion into a
shared formatting helper, including the existing logic in
ReturnValueDecoder::decode_to_string. Update both formatted argument maps and
formatted return-value construction in the current decoder to call that helper,
preserving string cloning and JSON serialization with the existing fallback.
In `@crates/core/src/decode/return_decoder.rs`:
- Around line 437-543: Extend the tests module with coverage for
ReturnValueDecoder::decode_enum and decode_union, including value-type tuple
variants, struct-shaped variants, and unmatched variant behavior. Build the
necessary ScSpecTypeDef enum/union definitions and ScVal inputs, then assert the
decoded JSON for each supported case and the expected fallback for unknown
variants.
In `@crates/core/src/spec/decoder.rs`:
- Around line 181-279: Optionally add a shared doc-normalization helper near the
decoder utilities, such as opt_doc, that converts empty documentation to None
and non-empty documentation to Some(String). Replace the duplicated
normalization blocks across function, error enum, enum, union, union-case, and
struct-field decoding paths, including the visible UdtEnumV0 and UdtUnionV0
branches, while preserving existing output.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f281ee84-c689-4388-af81-ac8b886d9316
📒 Files selected for processing (7)
crates/core/src/decode/context.rscrates/core/src/decode/function_call_decoder.rscrates/core/src/decode/mod.rscrates/core/src/decode/return_decoder.rscrates/core/src/lib.rscrates/core/src/spec/decoder.rscrates/core/src/spec/mod.rs
| let args = vec![ | ||
| ScVal::Symbol(ScSymbol("recipient".try_into().unwrap())), | ||
| ScVal::I32(500), | ||
| ]; | ||
| let return_val = ScVal::Bool(true); | ||
|
|
||
| let decoded = decoder.decode_function_call("transfer", &args, Some(&return_val), &spec); | ||
| assert_eq!(decoded.function_name, "transfer"); | ||
| assert_eq!(decoded.arguments.len(), 2); | ||
| assert_eq!(decoded.return_value, Some(serde_json::json!(true))); | ||
| assert_eq!(decoded.formatted_return_value, Some("true".to_string())); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Test doesn't exercise typed argument decoding.
The args (Symbol, I32) don't match the declared param_defs (Address, I128), so both fall through to the dynamic path, and the only assertion is arguments.len() == 2. Use a matching ScVal::Address / ScVal::I128 pair and assert the decoded values.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/core/src/decode/function_call_decoder.rs` around lines 154 - 164,
Update the decode_function_call test inputs to match the declared Address and
I128 parameter definitions, replacing the Symbol and I32 ScVals with
corresponding typed values. Add assertions that verify the decoded argument
contents, while preserving the existing function name, return value, and
formatted return value checks.
| } else { | ||
| json!(variant_name) | ||
| } | ||
| } | ||
| None => json!({ variant_name: Self::decode_dynamic(&v[1]) }), | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Panic risk: v[1] indexed without a length check.
The guard only ensures !v.is_empty(). An unmatched void variant encoded as a 1-element vec ([Symbol]) reaches Line 356 and panics with index out of bounds. This is reachable whenever the on-chain union has a variant that isn't in the (possibly stale) spec.
🐛 Proposed fix
- None => json!({ variant_name: Self::decode_dynamic(&v[1]) }),
+ None => match v.get(1) {
+ Some(payload) => json!({ variant_name: Self::decode_dynamic(payload) }),
+ None => json!(variant_name),
+ },📝 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.
| } else { | |
| json!(variant_name) | |
| } | |
| } | |
| None => json!({ variant_name: Self::decode_dynamic(&v[1]) }), | |
| } | |
| } else { | |
| json!(variant_name) | |
| } | |
| } | |
| None => match v.get(1) { | |
| Some(payload) => json!({ variant_name: Self::decode_dynamic(payload) }), | |
| None => json!(variant_name), | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/core/src/decode/return_decoder.rs` around lines 352 - 357, Update the
None branch in the variant decoding logic to validate that v contains a second
element before accessing v[1]. For a one-element unmatched void variant, return
the appropriate safe decoded representation instead of panicking, while
preserving the existing dynamic decoding behavior when the payload is present.
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/core/src/decode/context.rs (1)
1-50: 🎯 Functional Correctness | 🔴 Critical | 🏗️ Heavy lift
crates/core/src/decode/context.rsis TypeScript, so the Rust crate can’t parse it.pub mod context;pulls this file into the module tree, butexport class AppError extends Error { ... }isn’t valid Rust. Move it to a.tsfile or rewrite it as Rust.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/core/src/decode/context.rs` around lines 1 - 50, Replace the TypeScript implementation in the context module with valid Rust, or relocate it to a TypeScript file outside the Rust module tree; ensure the module referenced by `pub mod context;` contains Rust-compatible definitions and preserves the existing error variants, status codes, and default messages.Source: Pipeline failures
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/core/src/decode/mod.rs`:
- Around line 27-28: Resolve the stale FunctionCallDecoder API in the decode
module: either restore the FunctionCallDecoder implementation in
function_call_decoder.rs, or remove its re-export from decode/mod.rs and update
every remaining caller and public API reference consistently. Ensure grat-core
compiles without unresolved FunctionCallDecoder symbols.
---
Outside diff comments:
In `@crates/core/src/decode/context.rs`:
- Around line 1-50: Replace the TypeScript implementation in the context module
with valid Rust, or relocate it to a TypeScript file outside the Rust module
tree; ensure the module referenced by `pub mod context;` contains
Rust-compatible definitions and preserves the existing error variants, status
codes, and default messages.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 777609b8-6c4e-44c2-8676-950d507f94dc
📒 Files selected for processing (3)
crates/core/src/decode/context.rscrates/core/src/decode/function_call_decoder.rscrates/core/src/decode/mod.rs
| pub use function_call_decoder::{DecodedFunctionCall, FunctionCallDecoder}; | ||
| pub use return_decoder::ReturnValueDecoder; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Restore FunctionCallDecoder or remove the stale re-export.
The current change removes the FunctionCallDecoder implementation, while this module still re-exports FunctionCallDecoder at Line 27-28. If no replacement definition exists, grat-core will fail to compile. Keep the implementation or update the public API and all callers together.
#!/usr/bin/env bash
set -euo pipefail
rg -n 'FunctionCallDecoder' \
crates/core/src/decode/function_call_decoder.rs \
crates/core/src/decode/mod.rs🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/core/src/decode/mod.rs` around lines 27 - 28, Resolve the stale
FunctionCallDecoder API in the decode module: either restore the
FunctionCallDecoder implementation in function_call_decoder.rs, or remove its
re-export from decode/mod.rs and update every remaining caller and public API
reference consistently. Ensure grat-core compiles without unresolved
FunctionCallDecoder symbols.
2 participants
closes #376
Overview
This PR implements
ReturnValueDecoderandFunctionCallDecoderingrat-coreto decode contract return values and function calls from rawScValform into typed, structured formats using contract specification definitions (ScSpecTypeDef&ContractSpec).Changes Introduced
1.
ReturnValueDecoder([crates/core/src/decode/return_decoder.rs]ReturnValueDecoderto decode rawScValreturn values against target function return specs (ScSpecTypeDef) and contract specs (ContractSpec).Bool,U32,I32,U64,I64,U128,I128,U256,I256,Timepoint,Duration,Void)C.../G...)0x...hex strings)Option,Result,Vec,Map,Tuple)Udtstructs, enums, unions mapped againstContractSpec)ScValdecoding when contract specs are omitted or unavailable.2.
FunctionCallDecoder([crates/core/src/decode/function_call_decoder.rs]FunctionCallDecoderto decode full contract function calls (argument list and return value).decode_call_arguments,decode_return_value, anddecode_function_call.3. Contract Specification Enhancements ([
crates/core/src/spec/decoder.rs](ScSpecTypeDefonContractFunction(return_type_def,param_defs) andContractStructField(type_def).UdtEnumV0andUdtUnionV0entries intoContractEnumDefandContractUnionDefonContractSpec.4. Integration into Diagnostic Pipeline ([
crates/core/src/decode/context.rs]ReturnValueDecoderintoextract_return_valuesoTransactionContext.return_valuedisplays typed decoded return values instead of raw XDR base64 strings.Verification
grat-coreandgrat-cli(cargo test).Summary by CodeRabbit