Support every type of tapscript closure

[louisinger]

Fix an old TODO preventing any type of tapscript to be used with the introspector.

@chris-ricketts @Kukks please review

[arkanaai]

🔍 Agent Review — Arkana

Scope: Protocol correctness, security, cross-repo impact

Summary

Clean, focused change that resolves a known TODO. Replaces MultisigClosure-only decoding in ReadArkadeScript with scriptlib.DecodeClosure() + type switch, enabling all closure types (CSV, CLTV, Condition variants) for Arkade scripts.

Protocol Correctness ✅

• The type switch correctly extracts PubKeys from all five closure types: MultisigClosure, CSVMultisigClosure, CLTVMultisigClosure, ConditionMultisigClosure, ConditionCSVMultisigClosure
• The default case properly rejects unsupported closure types with a descriptive error — no silent fallthrough
• The pubkey verification loop (checking expected xonly key exists in the closure) is unchanged — the security boundary is preserved

Security ✅

• Key validation logic is untouched — the new code only changes how pubkeys are extracted, not how they are verified
• DecodeClosure is the upstream scriptlib parser, so this delegates type detection to the canonical implementation rather than hand-checking
• No new trust assumptions introduced

Test Coverage

• The removed test case ("non-multisig tapscript") was testing the old rejection behavior which is now intentionally relaxed — correct to remove
• The "malformed tapscript decode" test is updated to match the new error message — good
• Consider adding a positive test case for at least one non-MultisigClosure type (e.g., ConditionMultisigClosure) to confirm the new paths work end-to-end in the introspector context

Cross-Repo Impact

This depends on scriptlib.DecodeClosure() — ensure that function handles malformed scripts safely (returns error, doesn't panic). If it's already well-tested in the scriptlib module, this is low risk.

Verdict

Straightforward improvement. No security concerns. Ship it. 🚢

[chris-ricketts]

@louisinger looks good to me. I suppose it would be good to have an integration test case for each type of closure, although that could easily be a separate issue.

[louisinger]

@louisinger looks good to me. I suppose it would be good to have an integration test case for each type of closure, although that could easily be a separate issue.

@chris-ricketts I'm adding unit tests with psbt fixtures to ReadArkadeScript should be ok-ish now ? I'll open issue for integrations

[arkanaai]

🔍 Follow-up Review — Arkana (post-f1dca6b)

New commit f1dca6b adds comprehensive unit tests for ReadArkadeScript using fixture-based test data. This addresses my earlier suggestion.

New Changes

pkg/arkade/script_test.go — Good test structure:

• 7 valid fixtures covering all 5 closure types (MultisigClosure with checksig and checksigadd, CSVMultisig, CLTVMultisig, ConditionMultisig, ConditionCSVMultisig, plus witness data)
• 4 invalid fixtures: out-of-range input, missing taproot leaf, invalid tapscript, wrong signer, wrong arkade script
• Uses fixture JSON (testdata/read_arkade_script.json) — keeps test logic clean

API change: ReadArkadeScript signature changed from (ptx, inputIndex, signerPubKey, entry) to (ptx, signerPubKey, entry) — inputIndex is now derived from entry.Vin internally. This is cleaner (eliminates redundant parameter), but callers all updated correctly.

⚠️ Cross-PR Conflict

#24 also modifies ReadArkadeScript but keeps the old 4-param signature. These PRs will conflict on merge. The closurePubKeys helper in #24 is equivalent to the inline type switch in #30. Whichever merges second will need a rebase.

Recommendation: merge #30 first (smaller, focused refactor), then rebase #24 onto it.

Verdict

Tests look solid. Ship it. 🚢