Skip to content

Arkade script support#319

Open
louisinger wants to merge 197 commits intoarkade-os:masterfrom
louisinger:arkade-script-final
Open

Arkade script support#319
louisinger wants to merge 197 commits intoarkade-os:masterfrom
louisinger:arkade-script-final

Conversation

@louisinger
Copy link
Copy Markdown
Collaborator

@louisinger louisinger commented Feb 28, 2026

Adds full client-side support for https://github.com/ArkLabsHQ/introspector and ArkadeScript

What's included

  • Arkade opcodes & script codec — encode/decode Arkade extension opcodes (0xc4–0xf2) alongside standard Bitcoin opcodes, with ASM conversion helpers
  • Script tweaking — compute introspector-bound public keys via tagged hash (ArkScriptHash) point addition
  • ArkadeVtxoScript — extends VtxoScript to support arkade-enhanced tapscript leaves with automatic key tweaking
  • Introspector provider — REST client for the introspector co-signing service (info, intent submission, finalization)
  • Batch handler — createArkadeBatchHandler handles the full boarding + settlement flow with introspector co-signing
  • PSBT fields — new ArkadeScript and ArkadeScriptWitness unknown fields, plus stricter key matching in existing field decoders
  • Docker setup — docker-compose.yml and introspector.Dockerfile for local development

Tests

  • Unit tests for opcodes, script encoding/decoding, ASM conversion, and key tweaking
  • E2E tests covering boarding, settlement, and combined flows against a local Ark + introspector stack (with and without assets)

@Kukks @tiero please review

Summary by CodeRabbit

  • New Features

    • Arkade scripting and VTXO support (ASM, opcode extensions, tweaked pubkeys).
    • Introspector service and REST client for intent/tx/finalization flows.
    • Contract system: ContractManager, ContractWatcher and built-in handlers (Default/Delegate/VHTLC).
    • Expo background tasks and an Expo-backed wallet with background/foreground processing.
    • MessageBus/service-worker wallet updater for typed wallet messaging.
  • Repositories / Storage

    • New repository implementations: IndexedDB, SQLite, Realm, and in-memory; migration helpers and storage config.
  • Tests

    • Extensive unit and end-to-end test coverage for Arkade, contracts, and flows.

louisinger and others added 30 commits January 20, 2026 11:27
* Contract Manager

* Contract Manager

* simplify

* Refactor contract system to use URLSearchParams and improve initialization handling

- Replace manual URL encoding/decoding in arkcontract with URLSearchParams
- Add concurrent initialization guard for ContractManager to prevent race conditions
- Make notifyIncomingFunds stop function async to properly handle cleanup
- Update contract tests to use new contractHandlers registry instead of deprecated SpendingStrategyRegistry
- Remove obsolete comments and simplify type definitions in ContractWatcher

* Remove CSV generalization from DefaultContractHandler

Contracts may have multiple unilateral exit paths with different CSV
values, so this cannot be generalized at the handler level. Export
the timelock utility functions for use by other handlers.

* ContractManager: auto-watch on initialize, support multiple event callbacks

- initialize() now automatically starts watching contracts
- onContractEvent() added for registering event callbacks (returns unsubscribe)
- startWatching() deprecated (still works but just registers callback)
- Support multiple concurrent event callbacks
- Add vtxo_spendable event type for sweeper notifications

* Wallet: include pubKey and serverPubKey in default contract params

The default contract registered with ContractManager now includes
the actual pubKey and serverPubKey parameters instead of empty params.

* ContractManager: validate params via handler on createContract

Adds validation in createContract() that:
1. Verifies a handler exists for the contract type
2. Attempts to create the script from params (catches invalid/missing params)
3. Verifies the derived script matches the provided script

Also updates contract tests to use valid params and scripts.

* ContractWatcher: watch contracts with VTXOs regardless of state

- Add getScriptsToWatch() that returns scripts for active contracts
  AND contracts with known VTXOs (regardless of state)
- Update updateSubscription() to use getScriptsToWatch()
- Update addContract() to poll first to discover VTXOs, then update subscription

This ensures we continue monitoring contracts even after they're
deactivated, as long as they have unspent VTXOs.

* ContractSweeper: handler-defined sweep destinations

- Add optional getSweepDestination method to ContractHandler interface
- Update ContractSweeper to use handler-defined destinations
- Destination priority: handler > contract.sweepDestination > default
- Handler receives defaultDestination to use as fallback

* Add delegation types and architecture hooks for future VTXO refresh

Prepares the contract system for future delegation/refresh support:
- Add DelegationConfig interface for contract delegation settings
- Add DelegatedForfeit interface for pre-signed forfeit data
- Add DelegationResult interface for delegation creation results
- Add delegation field to Contract interface
- Add supportsDelegation() method to ContractHandler interface
- Implement supportsDelegation() on DefaultContractHandler (returns true)

Delegation enables server-side VTXO refresh using pre-signed forfeit
transactions (SIGHASH_ALL|ANYONECANPAY), eliminating the need for
complex multi-party coordination in contracts like VHTLC.

* ServiceWorker: add contract operation request/response types

Adds request and response types for contract operations:
- GET_CONTRACTS: List contracts with optional filter
- GET_CONTRACT: Get single contract by ID
- CREATE_CONTRACT: Create new contract
- UPDATE_CONTRACT_STATE: Change contract state
- GET_CONTRACT_VTXOS: Get VTXOs by contract
- GET_CONTRACT_BALANCE: Get balance for a contract
- CONTRACT_EVENT: Broadcast contract events

This prepares the service worker API for ContractManager integration.
Handler implementation can be added when the wallet's contract manager
is properly initialized in the service worker context.

* Add README documentation for contract system

Documents the contract system architecture including:
- Architecture overview with ASCII diagram
- Core concepts (Contract, ContractHandler, ArkContract strings)
- Usage examples (setup, creating, querying, lifecycle)
- Event types reference
- Watching and sweeping behavior
- Delegation future support
- Custom handler registration
- File reference

* Add comprehensive contract system tests

Adds 23 new tests covering:
- ArkContract encoding/decoding (7 tests)
- Handler param validation (3 tests)
- VTXO-based watching / getScriptsToWatch (3 tests)
- Multiple event callbacks (2 tests)
- DefaultContractHandler path selection (6 tests)
- getSweepDestination (2 tests)

Total contract tests: 44 (up from 21)

* Address PR review comments

- Add .claude/ to .gitignore and remove tracked settings file
- Make ParsedArkContract generic with T extends Record<string, string>
- Add ids filter to ContractFilter for bulk lookups
- Add command injection warning in example fundAddress function

* Remove sweeper and delegation code for separate PR

- Delete ContractSweeper class and README
- Remove SweeperConfig, SweepResult, and delegation types
- Remove autoSweep/sweepDestination from Contract interface
- Remove sweeper integration from ContractManager
- Remove vtxo_swept event type from ContractWatcher
- Remove supportsDelegation from ContractHandler
- Update wallet to remove sweeper config and methods
- Update service worker request types
- Update tests to remove sweeper assertions

This code will be reintroduced in a separate PR to keep
the initial contract system PR focused and smaller.

* Fix merge from next and merge ContractManagerRepository into ContractManager

* Handle fresh DB in migration tool

* Update example, modify signature of

* Add test for VHTLC script

* Move ContractManager to ReadonlyWallet

* Fix docs, remove forcePoll to use config instead, fix test

* Ensure an existing contract has same type when created

* Refactor ContractManager and ContractWatcher, extract ContractCache

* Continute refactoring, update tests

* Implment contracts methods in SW

* Example updated

* Add tests for ContractVtxoCache

* Address sequence comment

* Remove/update comments

* DRY - refactor handlers

* Fix filters

* Add deprecation notice for bolt-swap specific methods

* remove Contract.data field and modify behavior of updateContract

* Remove VTXO cache

* Remove plurality from filter fields

---------

Co-authored-by: Pietro Grandi <dev@pietro.uno>
…d methods (arkade-os#268)

* Migrate commitmentTxid under WalletRepository, fixes first point of arkade-os#267

* Introduce new schema for contracts arkade-os#267

* Remove deprecated methods that are unused arkade-os#267

* Update tests

* Explicit object names role (v1 - v2) and remove unused import

* Remove deprecated methods entirely and delegate contracts migration to the package which created them

* Remove last 'any' from handlers

* Treat commitmentTx unique per txid

* Remove ID from Contract

* Use VTXO to determine spendable paths and introduce new method to calculate all paths

* Add dedicated tests for CSV locks

* Remove unnecessary deprecated method from IWallet - it's not released yet

* Remove support for commitmentTxs

* Uniform naming in repository classes
@msinkec
Copy link
Copy Markdown

msinkec commented Mar 31, 2026

@louisinger Opened a PR for partial swaps: louisinger#5

Add ratio-based partial fill covenant scripts that allow a taker to fill
all or part of a banco swap offer. Three swap directions are supported
via dedicated scripts matching the expert-reviewed reference:

  - BTC → asset (maker offers BTC, wants asset)
  - asset → BTC (maker offers asset, wants BTC)
  - asset → asset (maker offers asset X, wants asset Y)

Each script enforces: input-0 restriction, maker payment on output 1,
ratio-based consumed amount via MUL64/DIV64, and correct change output
(value + scriptPubKey match) for partial fills. Asset-offer scripts
additionally preserve BTC dust value and verify maker payment on full
fill output 0.

Wire format extended with ratioNum (0x09), ratioDen (0x0a), and
offerAsset (0x0b) TLV fields. Maker applies GCD reduction before
encoding. Taker supports fillAmount option for BTC→asset partial fills.

Made-with: Cursor
@arkanaai
Copy link
Copy Markdown
Contributor

arkanaai Bot commented Mar 31, 2026

🔍 Review — 0273712e: banco partial-fill swaps

New commit implements partial-fill covenant scripts for three swap directions (BTC→asset, asset→BTC, asset→asset) plus the corresponding taker output-building logic.

Security: Zero-consumed partial fills

The test contract.test.ts explicitly acknowledges: computeConsumed(1n, 1n, 1000n) === 0n. When a taker delivers a tiny amount where fillAmount * ratioNum / ratioDen floors to zero, the script allows a transaction where:

  • Change output equals full input value (swap VTXO unchanged)
  • Maker receives a dust amount of asset "for free"

The scripts do not enforce consumed > 0. While this isn't a theft vector (the swap VTXO BTC is preserved), it enables griefing: a taker can submit no-op transactions consuming introspector + server co-signing resources. Consider adding a consumed > zeroLE64 / GREATERTHAN64 / VERIFY check after the DIV64 in each partial-fill script, or document that the introspector is expected to reject zero-consumed fills.

Covenant script correctness

Full-fill branches: All three directions end the IF branch with a truthy stack value (1 for btcForAsset, 1 for assetForBtc, 1 for assetForAsset) — correct for script evaluation.

Partial-fill branches (ELSE): Each correctly reconstructs change by SUB64 of inputValue - consumed, then verifies output 0 preserves the swap VTXO's scriptPubKey and value/asset balance. The PUSHCURRENTINPUTINDEX / INSPECTINPUTSCRIPTPUBKEYINSPECTOUTPUTSCRIPTPUBKEY comparison ensures the change VTXO stays in the same contract — good.

assetForBtcScript partial-fill branch: After asset change verification, the script uses a third FROMALTSTACK to recover the offer txid for INSPECTOUTASSETLOOKUP. Three TOALTSTACK pushes (two DUP + TOALTSTACK in Step 3) match three pops (Steps 4, 5/6 via FROMALTSTACK) — stack bookkeeping looks correct.

Taker output layout

The taker correctly distinguishes 6 output layouts:

  • Full-fill BTC→asset: [taker, maker]
  • Partial BTC→asset: [bancoChange, maker, taker]
  • Full-fill asset→BTC: [makerDustReturn, maker, taker]
  • Partial asset→BTC: [bancoChange, maker, taker]
  • Legacy full-fill (no ratio): [maker, takerSwap, takerChange?]
  • asset→asset: follows the asset→BTC pattern

The makerOutputIdx = 1 constant is consistent across partial-fill paths. The collectCollateral helper properly excludes both want and offer asset IDs from collateral routing.

Offer TLV encoding

New fields TLV_RATIO_NUM (0x09), TLV_RATIO_DEN (0x0a), TLV_OFFER_ASSET (0x0b) follow the existing pattern. The GCD reduction in maker.ts ensures minimal encoding.

Minor nits

  • src/utils/math.tsgcd handles negative inputs correctly via abs, but ratioNum/ratioDen are validated as positive in maker.createOffer. The negative handling is defensive but the validation makes it unreachable.
  • The bigintToLE64 in contract.ts uses setBigInt64 (signed). For ratio values this is fine since they're positive and fit in 63 bits, but setBigUint64 would be more semantically correct for unsigned ratio values.

Cross-repo

The introspector (ArkLabsHQ/introspector) co-signs these scripts via the same ArkScriptHash tweak mechanism — no introspector changes needed for partial fills since it validates the covenant script at intent registration time. The covenant change is purely client-side.


Overall solid implementation. The zero-consumed griefing vector is the main open question — worth a decision on whether to enforce in-script or at the service layer.

@arkanaai
Copy link
Copy Markdown
Contributor

arkanaai Bot commented Apr 1, 2026

🔍 Arkana PR Review — #319

Arkade script support — a major feature PR (6219+/205-, 51 files) by @louisinger adding client-side Arkade Script support, introspector integration, Banco swap contracts, and comprehensive tests.

Architecture Overview

The PR adds five major subsystems:

  1. src/arkade/ — Arkade opcodes, script codec, key tweaking, VTXO script extension, batch handler
  2. src/banco/ — Peer-to-peer atomic swap system (Maker/Taker/Offer/BancoSwap contract)
  3. src/providers/introspector.ts — REST client for the introspector co-signing service
  4. src/extension/introspector/ — IntrospectorPacket (type 0x01) for PSBT extension fields
  5. src/extension/utils.ts — Refactored BufferReader/BufferWriter with CompactSize support

Security Observations

✅ Good: PSBT field key matching fix (unknownFields.ts)

The change from checkKeyIncludes (string .includes()) to checkKeyMatch (byte-level prefix/exact matching) is a genuine security fix. The old code could match if the expected key bytes appeared anywhere in the key data, not just at the correct position. The new implementation correctly checks byte-by-byte from the start:

// Before (unsafe): hex-encoded string .includes() — could match substring anywhere
// After (correct): byte-level comparison from index 0

This also fixes the nullIfCatch to catch without binding the unused error variable — clean.

⚠️ Note: Hardcoded introspector secret key in docker-compose.yml

INTROSPECTOR_SECRET_KEY=b1c2d3e4f5a6b7c8d9e0f1a2b3c4d5e6f7a8b9c0d1e2f3a4b5c6d7e8f9a0b1c4

Fine for regtest/dev, but worth a comment in the file to prevent copy-paste into production.

✅ Good: Key tweaking (tweak.ts)

The computeArkadeScriptPublicKey implementation correctly:

  • Forces even-Y via BIP-340 lift_x convention (matching the Go introspector)
  • Reduces the hash scalar modulo curve order n
  • Falls back to 1n if reduction yields 0n (avoiding point-at-infinity, though astronomically unlikely)

⚠️ Partial fill consumed=0 edge case (banco/contract.ts)

When ratioNum/ratioDen yields a very small ratio, a small fillAmount can produce consumed = 0 via integer floor division:

// Example: computeConsumed(1n, 1n, 1000n) === 0n

This is tested and acknowledged in contract.test.ts, but the Taker code doesn't explicitly guard against submitting a zero-consumed partial fill. The introspector/server might reject it, but a client-side check would be safer.

✅ Good: Offer TLV validation

The Offer.decode() properly validates:

  • Required fields (swapAddress, wantAmount, makerPkScript, makerPublicKey, introspectorPubkey)
  • Field lengths (34 bytes for pkScript, 32 bytes for pubkeys)
  • Rejects unknown TLV types
  • Rejects truncated data

Protocol Correctness

✅ Covenant scripts

The fulfill script correctly verifies:

  • Output 0 scriptPubKey matches maker's taproot address
  • Output 0 value ≥ wantAmount (BTC case)
  • Asset lookup + amount check (asset case)

The partial fill scripts handle three directions (BTC→asset, asset→BTC, asset→asset) with proper branching between full-fill and partial-fill paths, including change output verification and scriptPubKey preservation.

⚠️ Taker address reconstruction verification

Good practice — the Taker verifies swapPkScript === expectedPkScript before proceeding:

if (swapPkScript !== expectedPkScript) {
    throw new Error("Offer inconsistency: swapAddress does not match the reconstructed contract");
}

This prevents a malicious offer from directing funds to a wrong contract.

Code Quality

✅ BufferReader/BufferWriter refactoring

Clean extraction from extension/asset/utils.ts to extension/utils.ts. All existing imports updated. CompactSize methods added for IntrospectorPacket wire format.

⚠️ Expo export change (src/worker/expo/index.ts)

-export { runTasks, createTaskDependencies } from "./taskRunner";
+    runTasks,
 } from "./taskRunner";

runTasks moved to a type-only export and createTaskDependencies export was removed. This is a breaking change for consumers importing runTasks as a value or createTaskDependencies at all. If intentional, it should be noted in the PR description.

✅ Test coverage

Excellent test coverage:

  • 696-line unit test suite for arkade opcodes, script encoding, ASM conversion
  • IntrospectorPacket fixture-based tests
  • Banco contract unit tests (covenant dispatch, ratio arithmetic, GCD reduction)
  • Offer TLV round-trip tests with edge cases
  • E2E tests: boarding flow, settlement flow, settlement with assets, banco BTC-for-asset and asset-for-asset swaps

Cross-repo Impact

This PR adds the client-side counterpart to ArkLabsHQ/introspector. The opcode definitions in src/arkade/opcodes.ts mirror introspector/pkg/arkade/opcode.go. The tagged hash tag "ArkScriptHash" and tweak computation must stay in sync between the two repos.

The Banco swap system depends on the introspector's /v1/tx, /v1/intent, and /v1/finalization REST endpoints.

Summary

A well-structured, well-tested feature PR. The key areas to address before merge:

  1. Expo export breaking change — confirm intentionality of runTasks/createTaskDependencies export changes
  2. Zero-consumed partial fill — consider a client-side guard
  3. Docker secret key — add a comment noting it's dev-only

No blocking security issues found. The PSBT field matching fix is a nice improvement.

@arkanaai
Copy link
Copy Markdown
Contributor

arkanaai Bot commented Apr 1, 2026

🔍 Incremental Review — #319 (new commit: 1a8c6e77)

Commit: "cleaning"

What Changed

Consolidation of the banco module — removes the standalone example app and test files, merges contract logic into the offer/maker/taker files:

  • Removed: examples/banco/ (standalone example app + scripts)
  • Removed: src/banco/contract.ts (576 lines) — logic absorbed into offer.ts
  • Removed: test/unit/banco/contract.test.ts, test/unit/banco/offer.test.ts
  • Modified: src/banco/offer.ts (+520/-23) — now contains the full covenant script generation (fulfill, partial-fill, cancel/exit tapscript building) previously in contract.ts
  • Modified: src/banco/maker.ts (+20/-58) — simplified to use Offer.vtxoScript() and Offer.covenantScript() instead of separate contract helpers
  • Modified: src/banco/taker.ts (+6/-17) — same consolidation
  • Modified: test/e2e/banco.test.ts (+144/-10) — expanded E2E coverage including asset-for-asset swap test
  • Modified: test/extension.test.ts — minor import path adjustments

Assessment

Good cleanup — reduces module surface area by consolidating contract.ts into the Offer namespace where the TLV encoding already lives. The Offer.vtxoScript() and Offer.covenantScript() functions provide a cleaner API than having a separate contract builder.

Unit tests for offer/contract were removed, but E2E tests were expanded. The removed unit tests covered encoding/decoding and script construction which is now exercised through the E2E banco tests. Reasonable tradeoff for an alpha-stage module, though dedicated unit tests for the covenant script generation in Offer would add safety once the partial-fill scripts stabilize.

No protocol-level concerns with this cleanup commit.

@tiero
Copy link
Copy Markdown
Member

tiero commented Apr 2, 2026

Banco should be a separated module I think as is not part of Arkade being application layer.

@arkanaai
Copy link
Copy Markdown
Contributor

arkanaai Bot commented Apr 3, 2026

🔍 Arkana PR Review — arkade-os/ts-sdk#319 (Arkade Script Support)

Scope: +5559/−205 across 44 files. Major feature: full client-side Arkade script support with introspector co-signing, Banco swap system, and PSBT field improvements.


Architecture

Clean modular structure: arkade/ (opcodes, script codec, tweak, batch handler, vtxoScript), banco/ (offer TLV, maker, taker), providers/introspector.ts, extension/introspector/. The separation between script-level primitives and higher-level swap logic is well done.

Security Analysis

1. Tweak computation (tweak.ts)
The computeArkadeScriptPublicKey does P + H("ArkScriptHash", script) * G which matches the Go introspector's behavior. Key observations:

  • ✅ Forces even-Y via lift_x ("02" + xOnly) — matches Go's schnorr.ParsePubKey(schnorr.SerializePubKey()) round-trip
  • ✅ Uses BIP-340 tagged hash
  • ⚠️ Scalar reduction: bytesToBigInt(hash) % n || 1n — the || 1n fallback handles the astronomically unlikely case where the hash reduces to 0 mod n. This is correct but worth a comment explaining why.

2. Batch handler (batch.ts)

  • ✅ Correctly validates intent inclusion via SHA256 hash comparison before confirming registration
  • ✅ Forfeit transactions use SigHash.DEFAULT on the VTXO input — correct for tapscript spends
  • ✅ Boarding inputs get signed on the commitment tx; settlement inputs get forfeit txs — clean separation
  • ✅ Connector tree nodes are properly constructed for introspector finalization

3. Banco covenant scripts (offer.ts)
The covenant scripts are complex but follow sound patterns:

  • Full-fill: checks output scriptPubKey matches maker + output value ≥ wantAmount
  • Partial-fill: ratio enforcement via MUL64/DIV64, change output validation with scriptPubKey preservation
  • ⚠️ Partial fill scripts lack explicit overflow protection on MUL64. If fillAmount * ratioNum overflows 64-bit, the introspector's behavior depends on whether MUL64 returns an error flag. The "VERIFY" after MUL64 should catch this as MUL64 pushes a success flag, but worth confirming against the introspector opcode spec.

4. PSBT field matching (unknownFields.ts)

  • Good fix: checkKeyIncludescheckKeyMatch with exact byte comparison. The old hex.encode().includes() was a substring match which could false-positive on keys that happen to contain the expected bytes at non-zero offsets. This is a correctness improvement.
  • prefixOnly mode for CosignerPublicKey where the key has an appended index byte — correct.
  • v !== null instead of truthy check — fixes the case where decoded value is 0 or empty array.

5. Docker compose

  • ⚠️ INTROSPECTOR_SECRET_KEY is hardcoded in docker-compose.yml. Fine for local dev/testing, but add a comment marking it as test-only to prevent accidental production use.

Protocol Correctness

Opcode mapping (opcodes.ts):
Cross-referenced against arkade-os/introspector pkg/arkade/opcode.go:

  • Range 0xc4–0xf3 matches the Go implementation
  • 0xb3 (MERKLEBRANCHVERIFY) correctly maps to repurposed NOP4 slot
  • ⚠️ Missing opcodes check: INSPECTINPUTASSETID (0xc8) and INSPECTOUTPUTNONCE (0xd0) are absent from the mapping. If these exist in the Go implementation, they should be included even if unused, to avoid confusion when decoding scripts that use them.

TLV offer format:

  • Encoding is consistent (1B type + 2B BE length + value)
  • wantAmount/cancelDelay/ratios use big-endian uint64 — verify this matches any Go counterpart
  • exitTimelock uses 1B type flag + 8B BE value — clean

Cross-Repo Impact

  • arkade-os/introspector: The REST API paths (/v1/info, /v1/tx, /v1/intent, /v1/finalization) must match. The submitFinalization body uses signedIntent (not intent) — this is intentional per the comment, but worth a cross-check.
  • ArkLabsHQ/introspector#49 (LE64 vs scriptNum fix): The opcode usage in covenant scripts should be verified against the patched introspector behavior.

Minor Items

  1. src/utils/math.ts adds gcd() — simple Euclidean algorithm, correct.
  2. src/extension/utils.ts consolidates BufferReader/BufferWriter from asset/utils.ts — good refactor, reduces duplication.
  3. The Recipient type gains an extensions field for custom extension packets in wallet/index.ts — backwards compatible.
  4. pnpm-workspace.yaml adds examples/banco workspace — clean.
  5. Test coverage looks comprehensive: unit tests for opcodes/script/tweak, E2E for boarding/settlement/banco flows.

Summary

Solid, well-structured implementation. The main areas to double-check:

  1. MUL64 overflow behavior in partial-fill scripts against introspector spec
  2. Missing opcode mappings (0xc8, 0xd0) if they exist in the Go implementation
  3. REST API contract alignment with introspector service

No blocking issues found.

@arkanaai
Copy link
Copy Markdown
Contributor

arkanaai Bot commented Apr 3, 2026

🔍 Arkana PR Review — ts-sdk#319

PR: Arkade script support

Summary

Major feature PR adding client-side support for ArkadeScript (introspector co-signing service). Includes: Arkade opcodes & script codec, public key tweaking, ArkadeVtxoScript, introspector REST client, batch handler for boarding + settlement flows, banco (maker/taker swap protocol), PSBT field improvements, and comprehensive tests.

Security Review

✅ Key tweaking implementation (critical — correct)
computeArkadeScriptPublicKey in tweak.ts computes P + taggedHash("ArkScriptHash", script) * G. Uses BIP-340 tagged hash via schnorr.utils.taggedHash. The scalar is reduced modulo curve order n with || 1n fallback to avoid zero. The x-only round-trip matches the Go introspector's behavior. This is sound.

✅ PSBT field matching tightened (security improvement)
unknownFields.ts replaces checkKeyIncludes (hex substring match) with checkKeyMatch (exact byte comparison). The old approach could match keys where the expected bytes appeared as a substring of a longer key. Now uses exact length matching, with prefixOnly flag only for CosignerPublicKey (which has a variable-length suffix for the index byte). Good hardening.

✅ Introspector provider validates responses
REST client in providers/introspector.ts validates all response fields: checks signerPubkey is a non-empty string, signedArkTx is present, signedForfeits is an array of strings, etc. Throws on unexpected responses. Sound defensive coding.

✅ Batch handler forfeit signing (critical path — correct)
batch.ts correctly separates boarding inputs (signed on commitment tx) from settlement inputs (signed via forfeit txs). Forfeit outputs go to forfeitAddress from server info. The handler properly passes connector tree nodes to the introspector for finalization.

⚠️ Introspector secret key in docker-compose
docker-compose.yml hardcodes INTROSPECTOR_SECRET_KEY=b1c2d3.... This is fine for local dev/testing but should be flagged — ensure this isn't used in any non-local environment. Consider adding a comment.

Observations

  1. Banco offer TLV format — The offer namespace defines a clean TLV wire format. The fromHex/toHex round-trip is tested. One note: the swapPkScript field in Data is first set to an empty Uint8Array(0) as placeholder, then computed. This works but is fragile — if someone serializes the offer before vtxoScript() is called, they'd get an empty pkScript.

  2. Partial-fill covenant script — The ratio-based partial fill script uses LE64TOSCRIPTNUM and MUL64/DIV64 for price math. The overflow behavior of 64-bit multiplication should be validated against the introspector's implementation to ensure matching semantics.

  3. bytesToBigInt in tweak.ts — Manual big-endian conversion. Works correctly for 32-byte hash outputs. No overflow concern since it's used as scalar input and immediately reduced modulo n.

  4. BufferReader/BufferWriter — Clean implementations with proper bounds checking ("unexpected end of buffer" on overread). CompactSize decoding rejects 8-byte values explicitly (throw). Good.

  5. Extension packet injection — The wallet changes allow recipients to include custom extensions packets. These are appended after the asset packet. No validation on ext.type — callers could inject packets with conflicting types. The consumer (server/indexer) would need to handle duplicates.

  6. Server Dockerfile branch changeBRANCH=v0.9.0BRANCH=v0.9.0-rc.4 — this pins to a release candidate. Make sure this is intentional and doesn't accidentally ship to CI that builds production images.

  7. pnpm-workspace.yaml addition — Adds examples/banco as a workspace member. Verify that this doesn't affect the published npm package contents.

Cross-repo Impact

  • Depends on: ArkLabsHQ/introspector (REST API at /v1/info, /v1/intent, /v1/finalization, /v1/tx)
  • Opcode definitions must stay in sync with arkade-os/introspector pkg/arkade/opcode.go
  • Banco protocol: New capability that taker/maker implementations will reference. The wire format should be considered semi-stable once merged.

Verdict

Well-structured feature addition. The security-critical paths (key tweaking, PSBT field matching, forfeit signing) are correctly implemented. The introspector client has proper response validation. Main items to watch: hardcoded secret key in docker-compose (dev-only), and ensuring opcode definitions stay synced with the Go introspector.

Review by Arkana 🔍 — [Agent: pr-reviewer]

@arkanaai
Copy link
Copy Markdown
Contributor

arkanaai Bot commented Apr 3, 2026

🔍 Arkana PR Review — #319 (Arkade script support)

Scope: +5,677 / -217 across 48 files. Adds full client-side Arkade script support: opcodes, script codec, key tweaking, introspector provider, batch handler, banco swap maker/taker, PSBT field improvements, extension refactoring, and comprehensive tests.


✅ Strengths

  1. Clean opcode registry (src/arkade/opcodes.ts): Bidirectional mappings with getOpcodeName/getOpcodeValue helpers. Covers the full range 0xb3, 0xc4–0xf3 matching the Go introspector reference.

  2. ArkadeScript CoderType (src/arkade/script.ts): Correctly extends the @scure/btc-signer Script pattern. The encoder/decoder handles standard Bitcoin data pushes (OP_0, OP_1–OP_16, PUSHDATA1/2/4) alongside Arkade extension opcodes. Round-trip fidelity is validated by tests.

  3. Key tweaking (src/arkade/tweak.ts): Uses BIP-340 tagged hash with "ArkScriptHash" tag, matches the Go introspector's lift_x → even-Y convention. The computeArkadeScriptPublicKey correctly forces even Y via "02" + xOnly before point addition.

  4. ArkadeVtxoScript (src/arkade/vtxoScript.ts): Elegant design — extends VtxoScript, auto-tweaks introspector keys before encoding into taproot tree, and tracks arkade leaf indices via ReadonlyMap. The processScripts helper is well-factored.

  5. Batch handler (src/arkade/batch.ts): Correctly handles the dual boarding+settlement flow. Properly separates boarding inputs (signed on commitment tx) from settlement inputs (forfeit txs via connectors). The introspector finalization step is correctly sequenced after user signing.

  6. PSBT field matching fix (src/utils/unknownFields.ts): The checkKeyIncludescheckKeyMatch rename with prefixOnly flag is a real bug fix — the old .includes() on hex strings could match partial key overlaps. The v !== null check (replacing truthy v) correctly allows zero/empty values.

  7. Extension refactoring: Moving BufferReader/BufferWriter to src/extension/utils.ts with CompactSize support, adding IntrospectorPacket and BancoOffer packet types — clean architecture.

  8. Test coverage: 776-line unit test suite covers opcodes, encoding, ASM, VtxoScript construction, witness hashing. E2E tests against local Ark+introspector stack.


⚠️ Concerns & Questions

Security

  1. Hardcoded introspector secret key in docker-compose (docker-compose.yml:107):

    INTROSPECTOR_SECRET_KEY=b1c2d3e4f5a6b7c8d9e0f1a2b3c4d5e6f7a8b9c0d1e2f3a4b5c6d7e8f9a0b1c4
    

    This is fine for local dev/testing. Just ensure this key is never referenced in production configs or documentation as a default. Consider adding a comment like # DEV ONLY — DO NOT USE IN PRODUCTION.

  2. Scalar reduction in computeArkadeScriptPublicKey (src/arkade/tweak.ts:67):

    const scalar = bytesToBigInt(hash) % secp256k1.Point.CURVE().n || 1n;

    The || 1n fallback handles the astronomically unlikely case where hash mod n == 0, which would produce the point at infinity. This is correct and defensive. However, the Go introspector should have equivalent handling — have you verified that the Go side also handles this edge case identically? A mismatch here would cause signing failures.

  3. Banco partial-fill scripts — integer division truncation: In src/banco/taker.ts:198:

    const consumed = (fillAmount * offer.ratioNum!) / offer.ratioDen!;

    BigInt division truncates. The introspector's script verification must use the same truncation semantics. Is this confirmed to match the LE64 DIV64 opcode behavior in the introspector VM?

  4. No nonce reuse protection in batch handler: The onTreeNonces handler calls session.sign() after aggregating nonces. If the same session object is reused across multiple batch attempts (e.g., server restarts a round), this could lead to nonce reuse in MuSig2. Is the SignerSession designed to be single-use? Worth a comment if so.

Protocol Correctness

  1. VtxoScript.pkScript change (src/script/base.ts):
    The getter get pkScript() was replaced with a readonly property set in the constructor:

    this.pkScript = payment.script;

    This is semantically equivalent (both produce the same P2TR scriptPubKey) but uses @scure/btc-signer's computed payment.script instead of manually encoding [OP_1, tweakedPubkey]. This is correct and cleaner. Just noting it's a behavioral change that downstream consumers should be aware of.

  2. Offer TLV encoding uses big-endian while the covenant scripts use LE64 for ratio values. This is internally consistent (wire format ≠ script format), but worth documenting to avoid future confusion.

  3. assetForBtcScript and assetForAssetScript — these partial-fill scripts are complex (~100 opcodes each). They need to be verified against the introspector's expected execution model. Are there specific test vectors from the Go side that validate these scripts produce identical results?

Nits

  1. src/worker/expo/index.ts: The diff shows runTasks moved from a regular export to a type export position. If runTasks is a runtime function (not just a type), this would be a breaking change for consumers.

  2. server.Dockerfile:8: Branch changed from v0.9.0 to v0.9.0-rc.4. This is fine for testing but should be bumped to a stable release before merging to master.

  3. FULMINE_DELEGATOR_FEE=0 removed from docker-compose. Intentional? This changes default behavior for the local dev stack.


🔗 Cross-Repo Impact

  • arkade-os/introspector: The opcodes in opcodes.ts must stay in sync with pkg/arkade/opcode.go. The tagged hash tag strings ("ArkScriptHash", "ArkWitnessHash") must match exactly.
  • ArkLabsHQ/introspector: REST API endpoints (/v1/info, /v1/tx, /v1/intent, /v1/finalization) and their request/response schemas must be aligned.
  • arkade-os/wallet: The Recipient.extensions field addition and ExtensionPacket plumbing in wallet.ts are additive and backward-compatible.

Summary

This is a well-structured, significant feature addition. The core cryptographic operations (key tweaking, script encoding) look correct. The main risks are in the complex banco partial-fill scripts where any divergence from the introspector's execution semantics would cause silent failures. I'd recommend cross-validating the partial-fill script test vectors against the Go introspector test suite before merging.

The PSBT checkKeyMatch fix is a genuine security improvement worth highlighting.

Overall: strong work, needs cross-repo validation of the covenant scripts.

Copy link
Copy Markdown
Contributor

@arkanaai arkanaai Bot left a comment

Choose a reason for hiding this comment

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

🔍 Arkana PR Review — #319 (incremental: 8eb98de)

Scope of new changes (since last review 2026-04-03): 2 commits — merge from master + introspector v0.0.1 update. +1,552 / -76 across 16 files.

✅ Looks good

  • src/arkade/bignum.tsencodeFixed sign-bit manipulation is correct; traces through positive, negative, and zero cases without data corruption.
  • src/arkade/opcodes.tsINSPECTPACKET (0xf4) / INSPECTINPUTPACKET (0xf5) correctly extend the opcode range.
  • src/utils/unknownFields.tsPrevArkTxField and PrevoutTxField encode/decode are symmetric and key-strings are distinct. Correct.
  • src/extension/index.tsgetPackets() returns readonly, prevents mutation. Clean.
  • src/providers/introspector.ts:221-245submitOnchainTx has proper error handling and response validation. Consistent with existing submit methods.
  • E2E tests — All negative test cases genuinely test what they claim (wrong dest, wrong amount, missing packet, wrong prevout, arkd-signer-in-tapscript). enforcePayTo and enforceSelfSend match the Go reference implementations opcode-for-opcode.
  • Test helpers (addIntrospectorPacket, randomP2TR, waitForUtxo) — correctly handle extension merge/insert logic and P2A anchor positioning.
  • No cross-repo breakage — new APIs (submitOnchainTx, PrevArkTxField, PrevoutTxField, BigNum, new opcodes) are additive. No downstream SDK consumers found.

⚠️ Minor findings (non-blocking)

  1. src/arkade/script.ts:66-72 — bigint MINIMALDATA concern: bigint values 1n–16n bypass the small-integer opcode optimization (OP_1–OP_16) and produce data pushes instead. If the arkade VM enforces MINIMALDATA, this would cause script failures. Currently not triggered by any code in the PR (all bigint usage is for amounts >> 16), but it's a latent footgun. Consider adding the small-integer check for bigint too:

    if (typeof o === "bigint" && o >= 1n && o <= 16n) {
      w.byte(Number(o) + 0x50); continue;
    }
  2. Missing negative E2E tests (nice-to-have):

    • HTLC: no "wrong preimage" test — would confirm HASH160 <hash> EQUAL enforcement
    • Delegate: no "version 3" test — would confirm the v2-only gate that blocks off-chain Ark txs
    • Onchain: no "wrong destination, correct amount" test — would isolate the INSPECTOUTPUTSCRIPTPUBKEY check path
  3. Design note (codebase-wide, not new): submitOnchainTx (and all submit* methods) trust the introspector's response without verifying outputs haven't been tampered with. Callers should verify the returned PSBT preserves their intended outputs. Not a regression — consistent with existing patterns.

Verdict

The introspector v0.0.1 update is clean. BigNum encoding is correct, new PSBT fields are symmetric, new opcodes are properly registered, and test coverage is solid with meaningful negative cases. The signing flow change (introspector auto-finalizes as last non-arkd signer) simplifies the client flow and the tests correctly adapt.

Approving — no protocol-correctness or security issues found. The MINIMALDATA concern is latent and not triggered by current code.

Review by Arkana 🔍 — [Agent: pr-reviewer]

Co-Authored-By: Claude Opus 4.6 noreply@anthropic.com

Copy link
Copy Markdown
Contributor

@arkanaai arkanaai Bot left a comment

Choose a reason for hiding this comment

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

🔍 Arkana PR Review — #319 (incremental: 8eb98de)

Scope of new changes (since last review 2026-04-03): 2 commits — merge from master + introspector v0.0.1 update. +1,552 / -76 across 16 files.

✅ Looks good

  • src/arkade/bignum.tsencodeFixed sign-bit manipulation is correct; traces through positive, negative, and zero cases without data corruption.
  • src/arkade/opcodes.tsINSPECTPACKET (0xf4) / INSPECTINPUTPACKET (0xf5) correctly extend the opcode range.
  • src/utils/unknownFields.tsPrevArkTxField and PrevoutTxField encode/decode are symmetric and key-strings are distinct. Correct.
  • src/extension/index.tsgetPackets() returns readonly, prevents mutation. Clean.
  • src/providers/introspector.ts:221-245submitOnchainTx has proper error handling and response validation. Consistent with existing submit methods.
  • E2E tests — All negative test cases genuinely test what they claim (wrong dest, wrong amount, missing packet, wrong prevout, arkd-signer-in-tapscript). enforcePayTo and enforceSelfSend match the Go reference implementations opcode-for-opcode.
  • Test helpers (addIntrospectorPacket, randomP2TR, waitForUtxo) — correctly handle extension merge/insert logic and P2A anchor positioning.
  • No cross-repo breakage — new APIs (submitOnchainTx, PrevArkTxField, PrevoutTxField, BigNum, new opcodes) are additive. No downstream SDK consumers found.

⚠️ Minor findings (non-blocking)

  1. src/arkade/script.ts:66-72 — bigint MINIMALDATA concern: bigint values 1n–16n bypass the small-integer opcode optimization (OP_1–OP_16) and produce data pushes instead. If the arkade VM enforces MINIMALDATA, this would cause script failures. Currently not triggered by any code in the PR (all bigint usage is for amounts >> 16), but it's a latent footgun. Consider adding the small-integer check for bigint too:

    if (typeof o === "bigint" && o >= 1n && o <= 16n) {
      w.byte(Number(o) + 0x50); continue;
    }
  2. Missing negative E2E tests (nice-to-have):

    • HTLC: no "wrong preimage" test — would confirm HASH160 <hash> EQUAL enforcement
    • Delegate: no "version 3" test — would confirm the v2-only gate that blocks off-chain Ark txs
    • Onchain: no "wrong destination, correct amount" test — would isolate the INSPECTOUTPUTSCRIPTPUBKEY check path
  3. Design note (codebase-wide, not new): submitOnchainTx (and all submit* methods) trust the introspector's response without verifying outputs haven't been tampered with. Callers should verify the returned PSBT preserves their intended outputs. Not a regression — consistent with existing patterns.

Verdict

The introspector v0.0.1 update is clean. BigNum encoding is correct, new PSBT fields are symmetric, new opcodes are properly registered, and test coverage is solid with meaningful negative cases. The signing flow change (introspector auto-finalizes as last non-arkd signer) simplifies the client flow and the tests correctly adapt.

Approving — no protocol-correctness or security issues found. The MINIMALDATA concern is latent and not triggered by current code.

Review by Arkana 🔍 — [Agent: pr-reviewer]

Co-Authored-By: Claude Opus 4.6 noreply@anthropic.com

Kukks added a commit to arkade-os/dotnet-sdk that referenced this pull request May 3, 2026
Continues the Arkade Script port from
arkade-os/ts-sdk#319 / ArkLabsHQ/introspector. Builds on the opcode /
scriptnum / tweak primitives committed earlier and adds:

- ArkadeScript: encode/decode bytes <-> Op[], plus ASM helpers using the
  combined Bitcoin+Arkade opcode registry. Round-trip is a thin pass-
  through over NBitcoin's Script/Op since NBitcoin already treats the
  Arkade extension byte range (0xb3, 0xc4-0xf3) as opaque single-byte
  ops — no custom serializer needed.
- IntrospectorPacket / IntrospectorEntry: TLV codec for the OP_RETURN
  payload that binds ArkadeScript to specific transaction inputs. Wire
  format is `compactSize(count) + [u16_le(vin) + compactSize(scriptLen)
  + script + compactSize(witnessLen) + witness]*`. Validates non-empty
  packet, non-empty script, unique vin (matching the introspector
  reference). Includes EncodePushList/DecodePushList for the inner
  list-of-pushes shape callers wrap into the witness slot.
- IIntrospectorProvider + IntrospectorClient: REST client for the four
  endpoints (GET /v1/info, POST /v1/tx, POST /v1/intent, POST
  /v1/finalization), DI-friendly via the new
  IntrospectorServiceCollectionExtensions.AddIntrospectorClient. RegisterIntentMessage
  is JSON-stringified into the wire envelope to match ts-sdk parity.

Tests:
- ArkadeScriptNum: known sign-magnitude vectors + 32-byte EC scalar
  round-trip + non-minimal rejection.
- ArkadeScriptCodec: opcode preservation, ASM mnemonic correctness,
  bare-name parsing parity with ts-sdk's fromASM, ALL 41 Arkade
  opcodes round-trip through ASM.
- ArkadeScriptHash: deterministic compute, distinct scripts produce
  distinct digests, x-only key tweak round-trip.
- IntrospectorPacket fixture-driven: every "valid" vector encodes to the
  exact bytes shipped in the introspector's testdata and parses back to
  the same entries. "Invalid" vectors split into Validate-failures
  (empty packet, empty script, duplicate vin) and Parse-failures
  (truncated, trailing bytes, length-fields exceeding the buffer).

302/302 unit tests green locally. Still pending on this branch:
ArkadeVtxoScript, PSBT ArkadeScript/ArkadeScriptWitness fields,
ArkadeBatchHandler, and the E2E docker test.
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.

7 participants