Asset introspection pushes LE64 instead of scriptNum

[msinkec]

This PR fixes asset amount handling in asset introspection opcodes by moving amount values from scriptNum to 8-byte little-endian (LE64) stack values, and updates lookup semantics to a flag+value pattern.

Why

Asset amounts are defined as uint64, but previously they were pushed as scriptNum (int64) and later parsed with a 4-byte MakeScriptNum limit via PopInt(). That created correctness bugs for larger amounts:

• values above 2^63-1 could wrap on cast to int64
• values above ~2^31-1 could fail with ErrNumberTooBig when re-read as script numbers

What changed

• Added LE64 push helper in asset_opcodes.go and switched amount pushes to LE64 for:
  • OP_INSPECTASSETGROUP (input/output amount fields)
  • OP_INSPECTASSETGROUPSUM
  • OP_INSPECTOUTASSETAT
  • OP_INSPECTINASSETAT
• Updated lookup opcode semantics:
  • OP_INSPECTOUTASSETLOOKUP
  • OP_INSPECTINASSETLOOKUP
  • Found: push 1 then <amount LE64>
  • Not found: push 0 only
• Updated tests to compare amounts as LE64 and assert new lookup stack layout.
• Updated integration test script helper in test/utils_test.go to compare asset sums as LE64.

Script ergonomics improvement (Issue #40)

Using asset lookup amounts is now simpler and more consistent with 64-bit ops, matching the direction in Issue #40:

• no -1 sentinel handling boilerplate
• no OP_SCRIPTNUMTOLE64 conversion step before OP_ADD64 / OP_MUL64 / comparisons
• straightforward found-check via top-stack flag (OP_VERIFY), with amount already in LE64

Edge cases covered

Added explicit tests for large amounts and arithmetic/comparison interoperability:

• large amount reads from group/input/output paths
• large group sums (e.g. 10B)
• direct use with OP_ADD64
• direct use with OP_GREATERTHANOREQUAL64
• lookup found path with large amount using flag+LE64

Breaking change / migration note

This is a breaking stack-shape/encoding change for scripts using asset amount outputs:

• Amounts are now LE64 byte arrays, not scriptNum.
• Lookup opcodes now return flag + amount (found) or flag only (not found), replacing the old -1 sentinel behavior.

[arkanaai]

🔍 PR Review — Asset introspection pushes LE64 instead of scriptNum

Correctness fix: ✅ This is a solid and necessary change. uint64 amounts pushed as scriptNum (int64 → variable-length encoding with 4-byte MakeScriptNum limit) was a real bug for amounts > ~2.1B sats. The LE64 encoding aligns with how OP_INSPECTOUTPUTVALUE already pushes satoshi values, making the opcodes consistent and composable with 64-bit arithmetic ops.

What looks good

• pushAmountLE64 helper — clean, matches existing patterns. Correct endianness (LE matches Elements-style 64-bit ops).
• Lookup flag+value pattern — replacing -1 sentinel with flag + amount (found) vs flag only (not found) is a better API. No more ambiguity with -1 as a valid scriptNum, and cleaner script ergonomics (just OP_VERIFY the flag, amount is already LE64-ready for arithmetic).
• Edge case coverage — large amount tests (3B, 7B, 10B sums) directly exercise the former failure path. LE64 → OP_ADD64 / OP_GREATERTHANOREQUAL64 interop tests confirm scripts can use amounts without conversion steps.
• Integration test updated — uint64LE in test/utils_test.go stays consistent.

Observations

1. Breaking change scope: This changes the stack shape for every asset amount opcode. All downstream script compilers and SDKs that emit or parse these opcodes need updates:

   • arkade-os/compiler — needs to emit LE64 for amount comparisons in compiled scripts
   • arkade-os/ts-sdk — particularly #396 (CovVHTLC) and #319 (Arkade script support), which use asset introspection
   • arkade-os/rust-sdk — any asset introspection script construction
   • Worth a tracking issue or at minimum a mention in the merge commit for SDK authors

2. Lookup not-found: stack depth change — Previously lookup always pushed exactly 1 element (amount or -1). Now it pushes 2 on found (flag + amount) but 1 on not-found (flag only). Scripts that branch on the result need to handle different stack depths per path. The PR description notes this, but it's the kind of thing that trips up script authors. Consider whether a consistent 2-element push (0 + dummy 8-byte zero for not-found) would be safer for script ergonomics, even if it wastes 8 bytes of witness.

3. No overflow guard on pushAmountLE64 — uint64 can't overflow LE64, so this is fine. But the safeSumInputs/safeSumOutputs use big.Int and check .IsUint64() before calling pushAmountLE64, which is correct — sums that exceed uint64 max are rejected before encoding.

4. scriptNum cast for non-amount fields is still safe — Vin, Vout, group indices, etc. remain scriptNum (they're small integers). No issue there.

Verdict

Clean, well-motivated fix with thorough test coverage. The only design question worth discussing is the asymmetric stack depth on lookup (point 2 above). Everything else LGTM.

[louisinger]

The problem with returning 0 for "not found" is that the script author has to remember two patterns, because OP_FINDASSETGROUPBYASSETID still uses -1. Also, we could have asset outputs with an amount of 0 allowed... I'd keep -1 if possible.

[louisinger]

Suggested change

	// Not found: pushes 0 (failure flag only — no amount on stack).
	// Not found: pushes -1 (failure flag only — no amount on stack).

why not -1 to be consistent with other opcodes ?

[louisinger]

Suggested change

	// Not found: pushes 0 (failure flag only — no amount on stack).
	// Not found: pushes -1 (failure flag only — no amount on stack).

[louisinger]

let's update also README (and the spec : https://github.com/ArkLabsHQ/arkade-assets/tree/master in parallel)

[msinkec]

The the reason I don't really like -1 is that OP_IF doesn't accept it (not minimal encoding) and that OP_VERIFY doesn't fail on -1. Assets with amount of 0 aren't a problem, since flag value is unrelated to the amount value.

[msinkec]

I think it would be overall a better idea an cleaner to use 0 values, since they're actually falsy.

Currently there are 3 more opcodes that use -1 as a sentinel value for missing stuff:

• OP_INSPECTASSETGROUPCTRL (pkg/arkade/asset_opcodes.go)

  • Pushes -1 when a group has no control asset.

• OP_FINDASSETGROUPBYASSETID (pkg/arkade/asset_opcodes.go)

  • Pushes -1 when the asset group is not found.

• OP_INSPECTINPUTSCRIPTPUBKEY and OP_INSPECTOUTPUTSCRIPTPUBKEY via pushScriptPubKey (pkg/arkade/opcode.go)

  • Pushes -1 to indicate the script is non-native segwit (acts as a marker in place of a segwit version).

OP_INSPECT_X_SCRIPTPUBKEY opcodes are a special case here and can be left the way it is. Others can use a missing 0 flag to stay consistent.

Only the opcodes relating to asset amounts need the two stack element handling. Also I agree with the comment raised by Arkana:

2. Lookup not-found: stack depth change — Previously lookup always pushed exactly 1 element (amount or -1). Now it pushes 2 on found (flag + amount) but 1 on not-found (flag only). Scripts that branch on the result need to handle different stack depths per path. The PR description notes this, but it's the kind of thing that trips up script authors. Consider whether a consistent 2-element push (0 + dummy 8-byte zero for not-found) would be safer for script ergonomics, even if it wastes 8 bytes of witness.

I think pushing 2 elements for these consistently makes it more ergonomic without too much overhead.

[arkanaai]

🔍 PR Review (updated commits) — Asset introspection pushes LE64 instead of scriptNum

Reviewing new changes since f80e6ba → e814085

What changed since last review

The feedback on asymmetric stack depth has been addressed:

1. Consistent 2-element push on lookup (found/not-found) — OP_INSPECTOUTASSETLOOKUP and OP_INSPECTINASSETLOOKUP now push flag + amount in both paths. Not-found pushes 0 + 8-byte-zero. This is the right call — script authors get a predictable stack shape regardless of branch, and the 8 extra bytes of witness overhead is negligible.

2. OP_INSPECTASSETGROUPCTRL — consistent 3-element push — Missing control asset now pushes 0 + nil_txid + 0 instead of just -1. Found pushes 1 + txid + index. Same predictable stack shape benefit.

3. OP_FINDASSETGROUPBYASSETID — consistent 2-element push — Not-found pushes 0 + 0 instead of just -1. Found pushes 1 + index.

4. OP_INSPECTINPUTSCRIPTPUBKEY refactored — Now inlines witness program extraction rather than calling shared pushScriptPubKey. Still uses -1 for non-segwit version marker, which is fine since segwit versions are non-negative.

All of these are improvements over the initial version. ✅

Minor observations

1. Comment typo introduced (opcode.go): The doc comment for opcodeInspectOutputScriptPubkey was changed to opcodeInspectOutputScriptkey (missing "Pub"). Trivial nit.

2. pushScriptPubKey now orphaned? — opcodeInspectInputScriptPubkey no longer calls pushScriptPubKey, but opcodeInspectOutputScriptPubkey presumably still does. If pushScriptPubKey is still used by the output variant, this is fine. If not, dead code worth cleaning up.

3. Cross-repo tracking — Same note from previous review applies: the compiler, ts-sdk (especially #396 CovVHTLC and #319 Arkade scripts), rust-sdk, and arkade-assets spec all need coordinated updates for the new stack shapes. louisinger already flagged the spec/README update.

Test coverage

New tests for large amounts (3B, 7B, 10B sums), LE64→OP_ADD64 interop, OP_GREATERTHANOREQUAL64 interop, and lookup flag verification all look correct and well-structured.

Verdict

The updated commits address all prior feedback. Consistent stack shapes, clean LE64 encoding, solid test coverage. LGTM pending the spec updates in arkade-assets.

[arkanaai]

🔍 PR Review (commit 81b626f) — Revert accidentally committed changes

New since last review: Single commit reverting unrelated changes in opcode.go.

Changes reviewed

1. opcodeInspectInputScriptPubkey — Reverts inlined witness-program handling back to the shared pushScriptPubKey helper. This is correct: the helper already handles both witness and non-witness scripts identically, so the duplication was unnecessary and divergent.

2. Comment fix — opcodeInspectOutputScriptkey → opcodeInspectOutputScriptPubkey. Matches the actual function name.

Assessment

✅ Clean revert. No functional changes to the LE64 asset amount logic from prior commits. The revert removes ~15 lines of duplicated code and restores DRY usage of the shared helper.

No issues found. The overall PR (LE64 amount encoding + flag-based lookup semantics) remains solid from previous review.

[msinkec]

Updated the missing-value handling to use explicit tagged results with the found flag on top of the stack, instead of relying on -1 sentinels or varying stack depth.

What changed:

• OP_INSPECTASSETGROUPCTRL now returns txid, index, found, with missing encoded as "", 0, 0.
• OP_FINDASSETGROUPBYASSETID now returns group_index, found, with missing encoded as 0, 0.
• OP_INSPECTOUTASSETLOOKUP and OP_INSPECTINASSETLOOKUP now always return amount_le64, found, using 0 + 0 on misses so both branches have the same stack depth.

Why:

• 0 works naturally as an explicit false/missing flag, while -1 is truthy and forces special-case comparisons.
• Returning the flag on top makes the opcodes easier to use directly in scripts, since you can branch immediately without first rotating or swapping stack items.
• Fixed-width tagged results also make success and failure paths leave the same stack shape.

Example:

<txid> <gidx> OP_FINDASSETGROUPBYASSETID
OP_IF
    # found, group_index remains
OP_ELSE
    OP_DROP
    # missing
OP_ENDIF

[arkanaai]

Follow-up review (fe0cfbde — push order fix)

Good correction. The previous commits pushed the flag first (buried under the payload), requiring scripts to SWAP/ROT to reach it. Now flag is always the last push → lands on top of the stack → scripts can branch immediately with OP_IF/OP_VERIFY.

Affected opcodes and their new stack-top layout (top → bottom):

Opcode	Found	Missing
OP_INSPECTASSETGROUPCTRL	1, index, txid	0, 0, ""
OP_FINDASSETGROUPBYASSETID	1, group_index	0, 0
OP_INSPECTOUTASSETLOOKUP	1, amount_le64	0, 0_le64
OP_INSPECTINASSETLOOKUP	1, amount_le64	0, 0_le64

Tests updated consistently — the assertions now pop the flag first, verify it, then check the values underneath. LGTM on this commit.