allow flexible boarding scripts in client-lib#966
allow flexible boarding scripts in client-lib#966bitcoin-coder-bob wants to merge 4 commits intomasterfrom
Conversation
WalkthroughAdds support for creating and persisting custom boarding addresses: new NewBoardingAddress methods on client, service, and wallet layers; per-UTXO vtxo script parsing in unroll; store support and tests for boarding descriptors and deduplication. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Client as ArkClient
participant Service as FundingService
participant Wallet as BitcoinWallet
participant Store as WalletStore
participant Script as ScriptValidator
User->>Client: NewBoardingAddress(ctx, vtxoScript)
Client->>Service: NewBoardingAddress(ctx, vtxoScript)
Service->>Service: safeCheck()
Service->>Script: Validate(vtxoScript, signerPubKey, delay)
Script-->>Service: validation result
Service->>Wallet: NewBoardingAddress(ctx, vtxoScript)
Wallet->>Script: TapTree/Parse vtxoScript
Script-->>Wallet: taproot key, tapscripts
Wallet->>Wallet: Build P2TR address
Wallet->>Store: AddBoardingDescriptor({address, tapscripts})
Store-->>Wallet: persisted/ack
Wallet-->>Service: *types.Address
Service-->>Client: *types.Address
Client-->>User: address with tapscripts
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
|
[Arkana Review — PR #966: allow flexible boarding scripts in client-lib] Reviewed at: 2026-03-14 14:00 UTC SummaryAdds What looks goodScript validation at generation time:
Store deduplication: Test coverage: Both in-memory and file store paths tested with add/retrieve/dedup/multi-descriptor cases. Questions / potential issues1. No removal / cleanup path
2. Script validation uses
3. File store concurrent writes In 4. Custom boarding addresses are appended before the default boarding address (from Nits
Cross-repo impactThis adds a new method to the VerdictThe core functionality is correct and the unroll fix is a genuine bug fix. Questions 1 and 2 are the most important to address — especially the delay mismatch if this is meant to serve the unrolled-VTXO rejoin path. — Arkana |
|
@arkanaai answering your questions: 1: I don't think there enough times that people board (and thus calls to 2: |
|
Thanks @bitcoin-coder-bob — that clarifies both points. On (1): Makes sense to keep it add-only for now given boarding frequency. If the address list ever grows unwieldy in long-lived wallets it'd be easy to add a cleanup pass later. On (2): Understood — That resolves my main blockers. The concurrent file write concern (point 3) is the one remaining nit but if the client-lib is single-goroutine in practice it's not a correctness issue. Good to go from my side. |
|
[Arkana Review — PR #966: allow flexible boarding scripts in client-lib (follow-up)] Reviewed at: 2026-03-26 11:34 UTC | Head: New since last reviewTwo commits since my initial review: a merge from Test coverage assessment
Both test files are well-structured. No gaps I can see for the current scope. Previously raised points — status
All main concerns resolved. No new issues introduced by the test commit. — Arkana |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
pkg/client-lib/unroll.go (1)
555-558: Include outpoint context when tapscript parsing fails.Returning the raw parse error makes debugging multi-input failures harder; include
Txid:VOutin the error.Proposed patch
vtxoScript, err := script.ParseVtxoScript(utxo.Tapscripts) if err != nil { - return err + return fmt.Errorf("parse vtxo tapscripts for %s:%d: %w", utxo.Txid, utxo.VOut, err) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/client-lib/unroll.go` around lines 555 - 558, The ParseVtxoScript call returns raw errors without context; modify the error path after vtxoScript, err := script.ParseVtxoScript(utxo.Tapscripts) to wrap or return a new error that includes the outpoint identification (Txid:VOut) from the utxo (e.g., utxo.Txid and utxo.Vout) so callers can see which input failed; update the return to include that string alongside the original err (use fmt.Errorf or an errors.Wrap-style pattern in the same function in unroll.go).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/client-lib/unroll_test.go`:
- Around line 17-19: The test currently ignores errors from
btcec.NewPrivateKey() for signerKey, ownerKey1, and ownerKey2; update
unroll_test.go so each call checks the returned error and fails the test on
error (e.g., use t.Fatal/t.Fatalf or require.NoError) before using the keys,
ensuring signerKey, ownerKey1 and ownerKey2 are not nil if key generation fails.
In `@pkg/client-lib/wallet/singlekey/bitcoin_wallet.go`:
- Around line 180-227: NewBoardingAddress currently persists any vtxoScript
allowing callers to register a boarding script with an exit delay below the
configured floor; before calling w.walletStore.AddBoardingDescriptor in
bitcoinWallet.NewBoardingAddress, retrieve data.BoardingExitDelay (already
fetched from w.configStore) and validate the script's exit delay (e.g. using the
VtxoScript method that exposes its exit/lock delay) is >=
data.BoardingExitDelay; if it is below the floor return an error (same behavior
as WalletService.NewBoardingAddress or a shared helper), so the check occurs in
this path prior to persisting the descriptor.
- Around line 215-221: The code is persisting the built-in default boarding
descriptor, causing the default boarding address to be duplicated by
GetAddresses; before calling w.walletStore.AddBoardingDescriptor(descriptor) in
the method that builds the BoardingDescriptor, detect if the constructed
descriptor matches the built-in default (compare the Address and/or Tapscripts
against the known default boarding descriptor used by GetAddresses, e.g., the
walletstore's default descriptor or the value returned by a
DefaultBoardingDescriptor helper) and skip calling AddBoardingDescriptor when it
is identical to that default.
In `@pkg/client-lib/wallet/singlekey/store/file/store.go`:
- Around line 146-150: The file write in the wallet store currently marshals
currentData and writes to s.filePath with mode 0755; change the permission to
owner-only (0600) so the wallet state is not world-readable/executable: after
json.Marshal(currentData) use os.WriteFile(s.filePath, jsonString, 0600) (in the
same function handling currentData and s.filePath) to harden state.json
permissions.
In `@pkg/client-lib/wallet/singlekey/store/inmemory/store.go`:
- Around line 35-55: The boarding descriptor Tapscripts slice is shared between
callers and the store; create a helper cloneDescriptor(descriptor
walletstore.BoardingDescriptor) walletstore.BoardingDescriptor that deep-copies
the Tapscripts slice (make and copy the slice) and use it in
AddBoardingDescriptor to append a cloned descriptor and in
GetBoardingDescriptors to return a slice of cloned descriptors (copy each
element via cloneDescriptor) so callers cannot mutate the store's internal
slices via shared slice headers.
---
Nitpick comments:
In `@pkg/client-lib/unroll.go`:
- Around line 555-558: The ParseVtxoScript call returns raw errors without
context; modify the error path after vtxoScript, err :=
script.ParseVtxoScript(utxo.Tapscripts) to wrap or return a new error that
includes the outpoint identification (Txid:VOut) from the utxo (e.g., utxo.Txid
and utxo.Vout) so callers can see which input failed; update the return to
include that string alongside the original err (use fmt.Errorf or an
errors.Wrap-style pattern in the same function in unroll.go).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ada211f3-7ade-4388-adc9-be8585921f03
📒 Files selected for processing (11)
pkg/client-lib/ark_sdk.gopkg/client-lib/funding.gopkg/client-lib/unroll.gopkg/client-lib/unroll_test.gopkg/client-lib/wallet/singlekey/bitcoin_wallet.gopkg/client-lib/wallet/singlekey/store/file/store.gopkg/client-lib/wallet/singlekey/store/inmemory/store.gopkg/client-lib/wallet/singlekey/store/store.gopkg/client-lib/wallet/singlekey/store/store_test.gopkg/client-lib/wallet/wallet.gopkg/client-lib/wallet/wallet_test.go
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/client-lib/unroll.go (1)
534-534: Remove the unusedcontext.Contextparameter fromaddInputs.Line 534 keeps
_ context.Contexteven though the helper no longer uses context. Since this is a private method, you can simplify the signature and both call sites.Suggested diff
-func (a *service) addInputs( - _ context.Context, updater *psbt.Updater, utxos []types.Utxo, -) error { +func (a *service) addInputs( + updater *psbt.Updater, utxos []types.Utxo, +) error {- if err := a.addInputs(ctx, updater, utxos); err != nil { + if err := a.addInputs(updater, utxos); err != nil {- if err := a.addInputs(ctx, updater, utxos); err != nil { + if err := a.addInputs(updater, utxos); err != nil {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/client-lib/unroll.go` at line 534, The addInputs helper currently has an unused first parameter _ context.Context; remove the context.Context parameter from addInputs' signature and update both call sites to stop passing a context. Locate the private function named addInputs and eliminate the leading context argument, then find callers that pass a context value (e.g., addInputs(...)) and adjust them to pass only the remaining parameters (updater *psbt.Updater, utxos []types.Utxo), ensuring imports/usages are updated accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/client-lib/unroll.go`:
- Line 534: The addInputs helper currently has an unused first parameter _
context.Context; remove the context.Context parameter from addInputs' signature
and update both call sites to stop passing a context. Locate the private
function named addInputs and eliminate the leading context argument, then find
callers that pass a context value (e.g., addInputs(...)) and adjust them to pass
only the remaining parameters (updater *psbt.Updater, utxos []types.Utxo),
ensuring imports/usages are updated accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4e0bd895-7b33-40d7-bdc7-47eeb5e44835
📒 Files selected for processing (5)
pkg/client-lib/unroll.gopkg/client-lib/unroll_test.gopkg/client-lib/wallet/singlekey/bitcoin_wallet.gopkg/client-lib/wallet/singlekey/store/file/store.gopkg/client-lib/wallet/singlekey/store/inmemory/store.go
🚧 Files skipped from review as they are similar to previous changes (3)
- pkg/client-lib/unroll_test.go
- pkg/client-lib/wallet/singlekey/bitcoin_wallet.go
- pkg/client-lib/wallet/singlekey/store/file/store.go
🔍 Arkana Review —
|
1 participant
closes #885
NewBoardingAddress(ctx, vtxoScript)toArkClientandWalletServiceinterfaces, allowing users to register custom boarding scripts (not just the default single-key script)GetAddressesnow returns custom boarding addresses alongside the default oneaddInputsinunroll.goto derivevtxoScriptfrom each individual UTXO's tapscripts instead of from a single freshly generated address (the old// TODO works only with single-key wallet)Summary by CodeRabbit
New Features
Tests