arkd-wallet: adopt txsigner shared library - #1124
Conversation
…nalize Replace the inline tapscript-path signing block with txsigner.SignTapscriptInput and the extractRawTx finalize/extract block with txsigner.ExtractFinalizedTx (both verbatim matches of the shared primitives). Net -72 lines; key-path signing and chain-fetch prevout logic (wallet-specific) are unchanged.
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
65b02ea to
efca3c2
Compare
…A1019 x/net 0.55 deprecates h2c.NewHandler; suppress SA1019 via golangci-lint exclusion. Mirrors the #1118 base fix; self-drops on rebase onto a bumped master.
3a895bd to
4f77dfa
Compare
|
This PR has been open for 3+ days without review. @Kukks is anyone looking at this? |
|
This PR has been open for 30+ days without review. @Kukks is anyone looking at this? |
arkana-ai-bot
left a comment
There was a problem hiding this comment.
This PR has been open for 6+ days without review. @Kukks is anyone looking at this?
arkana-ai-bot
left a comment
There was a problem hiding this comment.
This PR (arkd-wallet adopt txsigner) has been open 63 days without review. @Kukks is anyone looking at this?
|
This PR has been open for 4-5+ days without a review. @Kukks is anyone looking at this? (arkd-wallet: adopt txsigner shared library; 5 days without review.) |
|
This PR has been open for 10+ weeks without a review. @Kukks — is this still active? |
arkana-ai-bot
left a comment
There was a problem hiding this comment.
Arkana review — #1124 (sha 4f77dfa)
arkd-wallet: adopt txsigner shared library
Assessment: dependency and tooling update — looks ready, but adding arkade-os/emulator as a dependency needs a human eye.
What changed
- Dependency bumps:
golang.org/x/net0.52→0.55,golang.org/x/crypto0.49→0.52, plus assortedgolang.org/x/{mod,tools,term,sys,text}bumps. - Adds
github.com/arkade-os/emulator(api-spec, pkg/arkade, pkg/emulator) as a new dependency, along withgithub.com/consensys/gnark-crypto v0.19.2andgithub.com/bits-and-blooms/bitset v1.20.0(transitive from gnark-crypto). - Adds a golangci exclusion for
SA1019oninterface/grpc/service.go(deprecatedh2c.NewHandler), tracked as a follow-up.
Things that look good
- The
golang.org/x/cryptobump closes known CVEs in older versions (worth verifying against the current advisory list, but generally good hygiene). - The golangci exclusion is scoped to a specific file and linter text — not a blanket suppress.
Concern: new emulator dependency
github.com/arkade-os/emulator brings in gnark-crypto (a ZK proof library). This is a significant new trust surface. Questions for the author:
- What does the arkd-wallet use from the emulator library? Is this from the ArkadeScript emulator in #1122 (standalone signer), or does the wallet itself now run ArkadeScript?
gnark-crypto v0.19.2— is this the release vetted by the ArkadeScript security review? ZK library versions matter for soundness.- Has the emulator library itself been reviewed for supply-chain integrity?
No security issues visible in the diff itself, but the dependency addition warrants explicit sign-off.
|
This PR has been open for 4 days without review. @Kukks is anyone looking at this? |
Summary
Completes the
txsignerextraction — Task 1 (in #1118) created the shared library; this makes arkd-wallet consume it. Replaces two inline blocks inwallet.SignTransactionwith the shared primitives:txsigner.SignTapscriptInputextractRawTxfinalize/extract block →txsigner.ExtractFinalizedTxBoth are verbatim matches of the shared functions. Net −72 lines. The key-path (wallet-owned) signing and the chain-fetch prevout logic — both wallet-specific — are unchanged.
Testing
pkg/arkd-walletbuilds; wallet + handler unit tests pass;vet+gofmtclean.txsigner.SignTapscriptInputis unit-tested in the library; the changed path (forfeit/connector signing + finalization) is exercised end-to-end by the integration suite.CI note — repo-wide security bump (self-dropping): This branch also carries a
golang.org/x/crypto→v0.52.0 /golang.org/x/net→v0.55.0 bump across all 10 modules, clearing a newly-published Trivy CVE batch (x/crypto/ssh+x/net/html) that was failing every arkd build (master included).x/net0.55 deprecatesh2c.NewHandler, so SA1019 is suppressed via a targeted.golangci.ymlexclusion (not inline//nolint, which trips the golines length limit). The bump is shared with #1118 and self-drops when this rebases onto amasterthat already carries it. Migrating offh2ctohttp.Server’sProtocolsfield is a tracked follow-up.