Skip to content

fix: accept namespaced TII param refs#51

Open
LorenzoRD2003 wants to merge 1 commit into
txpipe:mainfrom
LorenzoRD2003:fix/namespaced-tii-refs
Open

fix: accept namespaced TII param refs#51
LorenzoRD2003 wants to merge 1 commit into
txpipe:mainfrom
LorenzoRD2003:fix/namespaced-tii-refs

Conversation

@LorenzoRD2003

@LorenzoRD2003 LorenzoRD2003 commented Jun 3, 2026

Copy link
Copy Markdown

Summary

Normalize namespaced TII schema refs before handing the JSON to tx3-sdk.

This makes cshell tx invoke and cshell tx resolve accept TIIs generated by current trix build, where parameter refs come as:

  • https://tx3.land/specs/v1beta0/tii#/$defs/Bytes
  • https://tx3.land/specs/v1beta0/tii#/$defs/Address
  • https://tx3.land/specs/v1beta0/tii#/$defs/UtxoRef

What changed

  • read the TII file as JSON in src/tx/common.rs
  • recursively rewrite only the three known namespaced refs to their legacy core#... equivalents
  • build the protocol with tx3_sdk::tii::Protocol::from_json(...)

Tests

  • added unit coverage for legacy refs, namespaced refs, and unknown refs

Validation

validated against a minimal repro where cshell tx invoke previously failed with invalid param type

Fixes #50

Summary by CodeRabbit

Release Notes

  • Bug Fixes
    • Added support for legacy core reference values in JSON files, enabling backward compatibility with configurations using legacy reference formats.
    • Improved parsing logic to properly handle both legacy and namespaced reference types while maintaining validation of unknown references.

@LorenzoRD2003 LorenzoRD2003 requested a review from scarmuega as a code owner June 3, 2026 15:28
@coderabbitai

coderabbitai Bot commented Jun 3, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

The pull request adds backward compatibility for legacy core $ref values in TIi JSON files by introducing a normalization layer. New helper functions recursively traverse JSON structures and rewrite namespaced core refs to core refs before protocol parsing. The prepare_invocation function now uses this normalized loading path. Unit tests validate both legacy and namespaced refs succeed while unknown refs are rejected.

Changes

TIi JSON Legacy $ref Normalization

Layer / File(s) Summary
Constants and imports for $ref normalization
src/tx/common.rs
Module imports std::path::Path and defines new private constants for core $ref target URIs to support legacy ref pattern matching.
$ref normalization helpers and loader integration
src/tx/common.rs
Recursive helper functions normalize namespaced core refs to core refs in JSON objects and arrays. A new loader applies this normalization before Protocol::from_json. The prepare_invocation function integrates the loader into the transaction file loading path.
Unit tests for $ref normalization
src/tx/common.rs
Test module with temporary file helpers and three tests validating acceptance of legacy core refs, namespaced refs, and rejection of unknown refs.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Poem

🐰 A rabbit hops through JSON trees,
Normalizing refs with utmost ease,
Legacy paths now safely blend,
With modern forms—a backward friend! 📜✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 25.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately and clearly summarizes the main change: adding support for namespaced TII parameter references by normalizing them to legacy core refs.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 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 `@src/tx/common.rs`:
- Around line 405-406: The assertion uses error.to_string() which only shows the
outermost anyhow context; change the check to inspect the full error chain
(e.g., use alternate formatting format!("{error:#}") or iterate error.chain()
and assert any chain element contains "invalid param type") so the assertion on
the variable `error` (obtained from result.expect_err) reliably finds the inner
cause; update the assertion around `let error = result.expect_err("unknown refs
should still fail");` to use one of these approaches.
🪄 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

Run ID: 79df5e54-a5de-48fd-ad9f-686526d6808e

📥 Commits

Reviewing files that changed from the base of the PR and between 5134bbc and 5a6ff16.

📒 Files selected for processing (1)
  • src/tx/common.rs

Comment thread src/tx/common.rs
Comment on lines +405 to +406
let error = result.expect_err("unknown refs should still fail");
assert!(error.to_string().contains("invalid param type"));

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🌐 Web query:

Does anyhow Error Display (to_string) include source/context chain or only the top-level message, and what does the alternate {:#} format print?

💡 Result:

When using the anyhow crate, the standard Display format (typically triggered by the {} format specifier or to_string) displays only the outermost error message or context [1][2][3]. It does not include the lower-level source or cause chain, behaving identically to the Display implementation of the underlying error type from which the anyhow::Error was constructed [1][3][4]. The alternate format specifier {:#} is used to print the error along with its full cause chain [5][3][4]. This format utilizes anyhow's built-in formatting logic to display the entire chain of underlying errors [6][5]. If you need to access or manually render the source chain without using the {:#} format, you can use the Error::chain method, which provides an iterator over the full sequence of source errors [3][4][7].

Citations:


Fix the error assertion to check anyhow’s full cause chain

anyhow’s to_string() / default {error} formatting prints only the outermost context message (e.g., "parsing tii file"), not the inner "invalid param type" from the source error chain. Use alternate formatting ({error:#}) or iterate error.chain().

🐛 Proposed fix
-        let error = result.expect_err("unknown refs should still fail");
-        assert!(error.to_string().contains("invalid param type"));
+        let error = result.expect_err("unknown refs should still fail");
+        assert!(
+            format!("{error:#}").contains("invalid param type"),
+            "unexpected error: {error:#}"
+        );
📝 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.

Suggested change
let error = result.expect_err("unknown refs should still fail");
assert!(error.to_string().contains("invalid param type"));
let error = result.expect_err("unknown refs should still fail");
assert!(
format!("{error:#}").contains("invalid param type"),
"unexpected error: {error:#}"
);
🤖 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 `@src/tx/common.rs` around lines 405 - 406, The assertion uses
error.to_string() which only shows the outermost anyhow context; change the
check to inspect the full error chain (e.g., use alternate formatting
format!("{error:#}") or iterate error.chain() and assert any chain element
contains "invalid param type") so the assertion on the variable `error`
(obtained from result.expect_err) reliably finds the inner cause; update the
assertion around `let error = result.expect_err("unknown refs should still
fail");` to use one of these approaches.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

cshell tx invoke fails with invalid param type after updating with tx3up

1 participant