Skip to content

Fix outputs_match_asset to not require asset_name#833

Open
SupernaviX wants to merge 6 commits intotxpipe:mainfrom
SupernaviX:sg/fix-watch-assets
Open

Fix outputs_match_asset to not require asset_name#833
SupernaviX wants to merge 6 commits intotxpipe:mainfrom
SupernaviX:sg/fix-watch-assets

Conversation

@SupernaviX
Copy link
Contributor

@SupernaviX SupernaviX commented Jan 18, 2026

The outputs_match_asset util wouldn't allow users to match policy ids unless they were also matching asset names. Update it to support this.

Also, fix the Phase1ValidationRejected error message so that it tells users what's wrong with their TX.

Summary by CodeRabbit

  • Improvements

    • Clearer validation error messaging that includes inner rejection details.
    • More precise asset matching with policy and asset-name filtering for accurate evaluations.
    • Block handling now preserves and includes original block bytes throughout processing.
  • Tests

    • Minor test alignment to fix predicate argument usage in comparison helpers.

@SupernaviX SupernaviX requested a review from scarmuega as a code owner January 18, 2026 17:46
@coderabbitai
Copy link

coderabbitai bot commented Jan 18, 2026

📝 Walkthrough

Walkthrough

Refined Phase1ValidationRejected message and adjusted transaction processing: outputs_match_asset now evaluates assets per-policy with optional empty-name wildcard; block_to_txs preserves original block bytes and populates the AnyChainTx block field. A test call-site reference fix was applied.

Changes

Cohort / File(s) Summary
Core error message
crates/core/src/lib.rs
Updated Phase1ValidationRejected display text to include the inner ValidationError message (no signature change).
Transaction processing / gRPC watch
src/serve/grpc/watch.rs
Reworked outputs_match_asset to check each output's assets against a policy-filtered pattern with empty-name as a wildcard; block_to_txs now captures original block bytes and sets AnyChainTx.block with native bytes and mapped block.
Tests (call-site fix)
tests/cardano/harness/compare.rs
Changed call site to pass &DiffByteRecord directly to the ignore predicate (removed extra reference indirection).

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Suggested reviewers

  • scarmuega

Poem

🐰 I hopped through code where bytes are kept tight,

Policies matched by name or empty-light,
Errors now whisper the inner tale clear,
Blocks carry memories, snug and near,
A rabbit cheers changes — hop, deploy, cheer! 🥕

🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title directly describes the main change: fixing outputs_match_asset to not require asset_name, which is the primary modification in src/serve/grpc/watch.rs.

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

✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

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.

@SupernaviX SupernaviX changed the title Sg/fix watch assets Fix outputs_match_asset to not require asset_name Jan 18, 2026
@scarmuega scarmuega force-pushed the main branch 2 times, most recently from 295b558 to 09d0b81 Compare January 28, 2026 07:14
Copy link
Contributor

@gonzalezzfelipe gonzalezzfelipe left a comment

Choose a reason for hiding this comment

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

@SupernaviX thanks for the contribution, functionality wise I think it makes perfect sense. Can you handle the conflicts for us to go forward with a merge?

@SupernaviX
Copy link
Contributor Author

@SupernaviX thanks for the contribution, functionality wise I think it makes perfect sense. Can you handle the conflicts for us to go forward with a merge?

I merged the main branch in. Tests are failing but they were before, and I didn't see any merge conflicts

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.

2 participants