Standardize asset introspection with canonical asset id#97
Conversation
![arkanaai[bot]](https://avatars.githubusercontent.com/u/170820085?s=60&v=4){kind=small}
There was a problem hiding this comment.
Arkana Code Review — PR #97: Standardize asset introspection with canonical asset id
Reviewer: Arkana (automated deep review)
Verdict: Request changes — one code-level bug, one critical cross-repo breakage, needs migration guidance
Overall Assessment
This is a well-motivated refactor. The semantic shift from packet-position-based gidx to canonical issuance-index-based asset_gidx is the right architectural call: it makes AssetIDs position-independent and composable across transactions. The helper extraction (resolveAssetID, popAssetID, assetIDEqual) dramatically improves readability and eliminates 5+ duplicated resolve-or-derive blocks. Test coverage is thorough and the new edge-case tests (same txid/distinct gidx, fresh issuance round-trips, operand validation) are exactly what was missing.
However, this is a breaking change to opcode semantics in a protocol-level system. Two issues must be resolved before merge.
🔴 CRITICAL: Cross-repo breakage — solver/bancod Banco swap scripts
File: solver/pkg/banco/contract/offer.go:288 (and identical code in bancod)
The solver builds fulfill scripts that hardcode gidx=0 (via OP_0):
b.AddOp(txscript.OP_0) // output index 0
b.AddData(o.WantAsset.Txid[:])
b.AddOp(txscript.OP_0) // asset group lookup index 0 ← BREAKS
b.AddOp(arkade.OP_INSPECTOUTASSETLOOKUP)Before this PR: OP_INSPECTOUTASSETLOOKUP matched by packet position. Passing gidx=0 meant "check the first packet group." The taker controls packet layout, so this worked.
After this PR: OP_INSPECTOUTASSETLOOKUP matches by canonical AssetID (txid, asset_gidx). If WantAsset.Index != 0, the lookup will never match — the fulfill script silently fails, and the swap is bricked.
Fix: The solver must pass o.WantAsset.Index instead of hardcoding 0:
b.AddInt64(int64(o.WantAsset.Index)) // canonical asset_gidxAlso affected: layerzero-usdt0-arkade-demo/internal/scripts/builders.go uses a FINDASSETGROUPBYASSETID → INSPECTOUTASSETLOOKUP pattern where the packet position k returned by FIND is fed as gidx to LOOKUP. After this PR, LOOKUP expects canonical asset_gidx, not k. The layerzero demo should pass the original canonical (asset_txid, asset_gidx) directly to LOOKUP instead of the intermediate k.
Recommendation: This PR should either:
- Ship with companion PRs to solver, bancod, layerzero-demo, and compiler, OR
- Document a migration guide and coordinate the rollout
🟡 MEDIUM: Duplicate canonical AssetID ambiguity in lookup opcodes
File: pkg/arkade/asset_opcodes.go:384-400 (opcodeInspectOutAssetLookup) and :501-515 (opcodeInspectInAssetLookup)
Both lookup opcodes now iterate all groups and return the first match by canonical ID. If a packet contains two groups with the same canonical AssetID (same Txid + same Index), only the first group's amount is ever returned.
The old code filtered by packet position first (scriptNum(groupIdx) != gidx), making each group individually addressable. The new code makes duplicate-ID groups indistinguishable from the script's perspective.
Question: Does the Arkade V1 packet spec guarantee canonical AssetID uniqueness within a packet? If yes, this is fine (but deserves a comment). If no, this is a behavioral regression — scripts lose the ability to differentiate same-asset groups.
🟢 Code Quality — Good
resolveAssetID (line 588): Clean, correct. The uint16(k) cast is safe given the documented precondition that k ∈ [0, len(packet)) and packets can't exceed 65535 groups.
popAssetID (line 597): Good validation — rejects negative gidx, values > 65535, wrong-length txids, oversized ScriptNums, non-minimal encoding. All boundaries are tested.
assetIDEqual (line 621): Straightforward and correct.
opcodeInspectAssetGroupCtrl (line 76): The AssetRefByGroup branch now correctly resolves through resolveAssetID instead of duplicating the nil-check logic. The int() conversion on group.ControlAsset.GroupIndex (line 77) is correct since GroupIndex is uint16.
opcodeInspectInAssetAt (line 454): Correctly removes the intent_txid special case. Intent inputs now always emit the canonical issuance txid. This is a deliberate semantic change that's well-tested (OP_INSPECTINASSETAT_intent test, line 735).
opcodeInspectInAssetLookup (line 501): Correctly removes intent_txid matching. The OP_INSPECTINASSETLOOKUP_intent_txid_rejected test (line 799) verifies the old path now returns not-found.
🟢 Test Coverage — Thorough
New test cases I verified:
canonical_find_first_of_shared_txid/canonical_find_second_of_shared_txid— same txid, distinct canonical gidx ✅canonical_out_at_emits_canonical_gidx— verifies gidx=20 emitted (not packet position 1) ✅canonical_fresh_issuance_group_resolution_round_trip— INSPECTASSETGROUPASSETID → FINDASSETGROUPBYASSETID ✅canonical_fresh_issuance_output_round_trip— OUTASSETAT → OUTASSETLOOKUP composability ✅- Operand validation (negative gidx, gidx>65535, oversized ScriptNum, non-minimal, wrong txid length) — all ✅
canonical_ctrl_by_group_fresh_issuance— control asset resolution through fresh issuance ✅- Intent txid rejection in both INSPECTINASSETAT and INSPECTINASSETLOOKUP ✅
Missing test: No test for the OP_INSPECTASSETGROUPCTRL AssetRefByID path with a canonical gidx ≠ 0. The existing OP_INSPECTASSETGROUPCTRL_found test uses ctrlTxid with Index: 7 but doesn't verify the gidx value is emitted as 7 (it just drops it). Consider adding explicit gidx verification.
🟢 Fuzz Generator Update — Correct
File: pkg/arkade/opcode_fuzz_test.go:338-359
The fuzz generator now derives canonical assetGidx from group.AssetId.Index (or k for fresh issuances) instead of always using the packet position. The mutation at line 356 (assetGidx += len(packet)) ensures the fuzzer explores out-of-range canonical gidx values. Correct.
🟢 Documentation — Good
README opcode table updates accurately reflect the new semantics. The new terminology section clearly distinguishes k (packet position) from asset_gidx (canonical issuance index). The note that intent input source txids are not AssetIDs is important and well-placed.
Summary
| Finding | Severity | Status |
|---|---|---|
| Cross-repo breakage (solver, bancod, layerzero) | 🔴 Critical | Must fix before merge |
| Duplicate canonical AssetID ambiguity | 🟡 Medium | Needs clarification |
| Missing ctrl gidx verification test | 🟢 Low | Nice to have |
🤖 Reviewed by Arkana · #97 · Co-authored review with Claude Opus 4.6
This should be guaranteed by arkd. |
|
@msinkec merge with master to fix Trivy CI 🙏 |
Done. |
There was a problem hiding this comment.
Arkana Follow-up Review — Merge commit 3bbc05b9
The new commit is a merge of origin/master into the branch. The only change it introduces to the PR's scope is a Go version bump (1.26.3 → 1.26.4 in go.mod). No conflicts, no changes to opcode logic, tests, or helpers. This merge is clean — no new issues.
Outstanding items from previous review
-
🔴 Cross-repo breakage (solver, bancod, layerzero-demo) — Still unresolved. Downstream scripts hardcode
gidx=0inOP_INSPECTOUTASSETLOOKUPcalls. Companion PRs or a coordinated rollout plan needed before merge. -
🟡 Duplicate canonical AssetID ambiguity — @msinkec confirmed arkd guarantees uniqueness. ✅ Resolved. A brief code comment in the lookup opcodes noting this invariant would be nice but is not blocking.
Nothing else to flag. My request-changes from the first review remains in effect pending resolution of the cross-repo breakage.
🤖 Reviewed by Arkana · #97 · Iterative review of new commits
2 participants
Summary
This PR standardizes asset introspection around canonical AssetIDs represented as
(asset_txid, asset_gidx).It separates an asset’s issuance group index from its current packet position, ensures intent inputs use the issuance transaction ID rather than the intent source transaction ID, and adds strict AssetID operand validation.
Opcode Changes
OP_INSPECTASSETGROUPASSETIDkasset_txid asset_gidx(current_txid, k).OP_INSPECTASSETGROUPCTRLkasset_txid asset_gidx 1orempty 0 0OP_FINDASSETGROUPBYASSETIDasset_txid asset_gidxk 1or0 0OP_INSPECTOUTASSETAToutput_index asset_indexasset_txid asset_gidx amountasset_gidxinstead of the current packet position.OP_INSPECTOUTASSETLOOKUPoutput_index asset_txid asset_gidxamount 1or0 0OP_INSPECTINASSETATinput_index asset_indexasset_txid asset_gidx amountOP_INSPECTINASSETLOOKUPinput_index asset_txid asset_gidxamount 1or0 0Validation
Malformed AssetIDs now produce script errors for:
asset_gidxvaluesasset_gidxvalues greater than65535Additional Changes
Testing
go test ./... -count=1inpkg/arkadeTestContractIdWithAssetIdentitygit diff --checkTestAssetAccountCovenantreaches the local emulator but currently fails during setup withVTXO_NOT_FOUND, unrelated to these opcode changes.