[client-lib] Update client wallet interface#1008
[client-lib] Update client wallet interface#1008altafan wants to merge 13 commits intoarkade-os:masterfrom
Conversation
… those those to get key pairs
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughConverts wallet APIs from address-centric to key-centric (NewKey/GetKey/ListKeys/KeyRef), moves address derivation into client-lib helpers (newAddress/getAddresses/deriveDefaultAddresses), updates call sites to use these helpers and types.Address with KeyID, and consolidates test setup to setupClient with derived Transport/Indexer and t.Cleanup teardown. Changes
Sequence Diagram(s)sequenceDiagram
participant Test
participant Client
participant Wallet
participant Transport
participant Indexer
Test->>Client: setupClient(ctx)
Client->>Wallet: NewKey/GetKey(ctx)
Wallet-->>Client: KeyRef (Id, PubKey)
Test->>Client: alice.SignTransaction(ctx, encodedTx)
Client->>Wallet: SignTransaction(ctx, tx, KeyRef)
Wallet-->>Client: SignedTx
Client->>Transport: Transport().SubmitTx(ctx, SignedTx)
Transport->>Indexer: FinalizeTx / Emit events
Indexer-->>Test: Event stream / confirmations
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![arkanaai[bot]](https://avatars.githubusercontent.com/u/170820085?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔍 Aggressive Code Review — #1008
Summary: Replaces GetAddresses/NewAddress/NewAddresses on WalletService interface with raw GetKeyPair/NewKeyPair. Address derivation moves from wallet layer → service layer (funding.go). Good architectural direction — the wallet should own keys, not protocol-specific address formats. But there are blocking issues.
🚨 BLOCKING: Cross-repo breakage — go-sdk will not compile
The go-sdk repo directly imports wallet.WalletService and calls GetAddresses(), NewAddress(), and NewAddresses() across 10+ files (sdk.go, client.go, send.go, utils_test.go, e2e tests). There is no companion PR on go-sdk.
Once this merges and go-sdk bumps its dependency, it will fail to compile. A coordinated PR on go-sdk is required before or immediately after merge. Please either:
- Open a companion PR on go-sdk, or
- Document in the PR description that go-sdk must be updated before the next release
Files affected in go-sdk: sdk.go:33 (re-exports GetAddresses on ArkClient interface), client.go (5 call sites), send.go, init_opts.go, utils_test.go (mock implements old interface methods).
⚠️ Breaking public API change on ArkClient.GetAddresses()
pkg/client-lib/ark_sdk.go: The ArkClient interface changes GetAddresses return type:
Before: ([]string, []string, []string, []string, error)
After: ([]string, []types.Address, []types.Address, []types.Address, error)
This is an intentional improvement (callers now get tapscripts alongside addresses), but it's a breaking change to the public ArkClient interface. All external consumers (go-sdk, any third-party integrations) that call GetAddresses will break. This should be called out in the PR description.
🔧 Code duplication in funding.go
pkg/client-lib/funding.go:300-450: newAddress() and getAddresses() share ~80% identical logic:
- Both create
DefaultVtxoScriptwith same params - Both compute
vtxoTapKeyviaTapTree() - Both encode offchain address via
arklib.Address.EncodeV0() - Both create boarding script + address
- Both compute onchain taproot key
Suggestion: Extract a common helper like deriveAddresses(pubkey *btcec.PublicKey) (onchain string, offchain, boarding, redemption *types.Address, error) and have both methods call it. newAddress calls NewKeyPair then deriveAddresses; getAddresses calls GetKeyPair then deriveAddresses. This eliminates a maintenance risk — if address derivation logic changes, you'd need to update both methods.
🔧 Dead code on bitcoinWallet
pkg/client-lib/wallet/singlekey/bitcoin_wallet.go: The old methods GetAddresses(), NewAddress(), NewAddresses() are still implemented on the struct but removed from the WalletService interface. They are now dead code — no interface requires them, and all internal callers have been migrated. They should be removed to avoid confusion and reduce maintenance surface.
✅ Good changes
- Nil-safety guards added:
SignTransaction,NewVtxoTreeSigner,SignMessagenow checkw.walletData == nilbefore proceeding — these were missing before and could panic. Good catch. - Test cleanup: Migration from
defer alice.Stop()tot.Cleanup(client.Stop)insetupClient()is correct — ensures cleanup even ont.Fatal. - Test simplification: Removing
setupArkSDKWithTransport,setupArkSDKwithPublicKey,setupIndexer— tests now usesetupClient(t)+client.Transport()/client.Indexer(). Much cleaner. defer close()→t.Cleanup(close): Correct pattern for test cleanup functions.GetKeyPairpath parameter: I see it's unused (_ string) in singlekey wallet but exists for future multi-key wallet support. Fine as a placeholder — just ensure any future HD wallet implementation actually uses it.
📋 Summary
| Category | Status |
|---|---|
| Protocol correctness | ✅ Address derivation logic is preserved identically |
| Security | ✅ Private key exposure unchanged, nil-safety improved |
| Cross-repo breakage | 🚨 go-sdk breaks — no companion PR |
| Public API breakage | ArkClient.GetAddresses() return type changed |
| Code quality | 🔧 Duplication in funding.go, dead code in bitcoin_wallet.go |
| Test coverage | ✅ Good — wallet_test.go rewritten, e2e tests updated |
Requesting changes for the go-sdk coordination gap. The code itself is solid — once the cross-repo plan is in place, this is ready.
🤖 Reviewed by 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: 1
🧹 Nitpick comments (1)
pkg/client-lib/funding.go (1)
300-366: Extract the shared address-derivation flow.
newAddressandgetAddressesduplicate nearly the entire taproot/offchain/boarding derivation pipeline. Any later change to the default script shape, network conversion, or tapscript encoding now has to be mirrored twice, which is easy to drift on.Also applies to: 368-447
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/client-lib/funding.go` around lines 300 - 366, Duplicate taproot/address derivation logic exists in newAddress and getAddresses; extract it into a single helper (e.g., deriveAddressBundle or buildAddressBundle) that takes ctx, pubkey (or obtains it via a.wallet.NewKeyPair when needed), network and exit delays and returns the onchain address string plus two *types.Address for offchain and boarding or an error. Move shared steps into that helper: utils.ToBitcoinNetwork conversion, creation of script.NewDefaultVtxoScript, calling TapTree, arklib.Address construction and EncodeV0, calling Encode on the script, boarding script creation, btcutil.NewAddressTaproot calls, and txscript.ComputeTaprootKeyNoScript; update newAddress and getAddresses to call the new helper and propagate errors, removing duplicated code.
🤖 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/wallet/wallet.go`:
- Around line 21-24: NewKeyPair currently creates a key but does not return its
derivation handle, preventing stable rediscovery with GetKeyPair; update the
NewKeyPair signature to return the derivation path/handle (e.g., NewKeyPair(ctx
context.Context) (prvkey *btcec.PrivateKey, pubkey *btcec.PublicKey, path
string, err error)) and propagate this change through the wallet backend
implementations and all callers so backends that emit distinct keys can persist
and later call GetKeyPair(ctx, path) to re-open the same key; ensure tests and
any single-key implementations are adjusted to return a deterministic path or a
fixed handle.
---
Nitpick comments:
In `@pkg/client-lib/funding.go`:
- Around line 300-366: Duplicate taproot/address derivation logic exists in
newAddress and getAddresses; extract it into a single helper (e.g.,
deriveAddressBundle or buildAddressBundle) that takes ctx, pubkey (or obtains it
via a.wallet.NewKeyPair when needed), network and exit delays and returns the
onchain address string plus two *types.Address for offchain and boarding or an
error. Move shared steps into that helper: utils.ToBitcoinNetwork conversion,
creation of script.NewDefaultVtxoScript, calling TapTree, arklib.Address
construction and EncodeV0, calling Encode on the script, boarding script
creation, btcutil.NewAddressTaproot calls, and
txscript.ComputeTaprootKeyNoScript; update newAddress and getAddresses to call
the new helper and propagate errors, removing duplicated code.
🪄 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: da9e4e80-b06a-4a4f-907f-5584aae9adc4
📒 Files selected for processing (13)
internal/test/e2e/e2e_test.gointernal/test/e2e/single_batch_smoke_test.gointernal/test/e2e/utils_test.gopkg/client-lib/ark_sdk.gopkg/client-lib/asset.gopkg/client-lib/batch_session.gopkg/client-lib/funding.gopkg/client-lib/send.gopkg/client-lib/service.gopkg/client-lib/unroll.gopkg/client-lib/wallet/singlekey/bitcoin_wallet.gopkg/client-lib/wallet/wallet.gopkg/client-lib/wallet/wallet_test.go
There was a problem hiding this comment.
Arkana Review — arkd#1008
Scope: Client wallet interface refactoring — replaces GetAddresses/NewAddress/NewAddresses with GetKeyPair/NewKeyPair, moving address derivation logic to the service layer.
Assessment
Architecture change is sound. The wallet interface becomes a pure key-management abstraction, and address derivation (which depends on server config like signer pubkey, exit delays) moves to service.go where it belongs. This is cleaner separation of concerns.
Key observations:
-
API surface reduction is correct. The
WalletServiceinterface shrinks from ~6 methods to ~4 that deal with keys, and address-related methods become internal to the service. This makes it easier to implement alternative wallet backends. -
Test refactoring is thorough. All e2e tests updated from
setupArkSDK→setupClient, removing manualdefer Stop()calls in favor oft.Cleanup. Good. -
SignTransactionno longer needsexplorerexternally — the service now owns the explorer reference. This is a nice ergonomic improvement. -
GetKeyPairandNewKeyPairreturn the same key in the single-key wallet —NewKeyPairdoesn't actually generate a new key. This is intentional for the single-key implementation but could be confusing. Consider adding a comment to the interface doc noting that single-key wallets return the same key pair. -
newAddresshelper in service.go — make sure this doesn't break callers that previously relied on thechangeparameter behavior ofNewAddress(ctx, change bool). The newnewAddress(ctx)appears to always passfalsefor change.
No protocol-critical concerns — this is a client-side refactoring that doesn't touch transaction construction, signing logic, or VTXO handling.
|
Overall I think Wallet is not anymore the right terminology if it reduces to low level signing methods and key derivation (address encoding is a pure function stateless and not changing between implementations, so irrelevant) In TypeScript we call it Identity and allows to be injected into Wallets (which are instead stateful and aware of how spending from scripts and transactions). |
There was a problem hiding this comment.
Arkana PR Review 🔍
Large refactoring of e2e tests to use a unified setupClient(t) helper instead of the previous setupArkSDK, setupArkSDKWithTransport, and setupArkSDKwithPublicKey helpers. This aligns tests with a new client wallet interface.
Key changes:
setupClient(t)returns a client that exposes.Transport(),.Indexer(),.Wallet(), and.GetAddresses()directly- Removes manual
defer alice.Stop()anddefer grpcClient.Close()in favor oft.Cleanupinside the helper SignTransactionsignature simplified: no longer takesexplorerparameterdefer close()→t.Cleanup(close)for event streams (safer cleanup order)
Observations:
- This is a test-only change — no production code modified. The risk is limited to test breakage.
- The new interface is cleaner: the client directly exposes
Transport()andIndexer()instead of requiring separate setup functions - The
SignTransaction(ctx, encodedArkTx)no longer takes an explorer — the client must handle UTXO lookups internally. This is a nicer API but means the client implementation handles more. - Several test functions remove the
mempool_explorer.NewExplorer(...)boilerplate, which is good cleanup.
Note: This PR has CHANGES_REQUESTED already. The underlying client-lib interface changes should be reviewed for correctness at the implementation level, not just the test level. The tests here just consume the new interface.
No protocol-critical concerns — this is test refactoring only.
I can go for the renaming in this PR directly |
| func (s *bitcoinWallet) SignTransaction( | ||
| ctx context.Context, explorerSvc explorer.Explorer, tx string, | ||
| ) (string, error) { | ||
| if s.walletData == nil { |
There was a problem hiding this comment.
should we also add these 2 checks into sinTapscriptSpend and signTaprootKeySpend
There was a problem hiding this comment.
i didn't touch the existing code but i'll check
There was a problem hiding this comment.
Arkana Incremental Review — New Commits (fa6785c, 1d33066)
Focusing on what changed since my last review on Apr 8-9. The new commits replace GetKeyPair/NewKeyPair with GetKey/NewKey/ListKeys, introduce KeyRef/KeyOption types, extract deriveDefaultAddresses, and add KeyID to types.Address.
Good progress — the code duplication I flagged is now resolved via deriveDefaultAddresses, and the interface is cleaner. But there are issues in the new code.
🚨 BUG: newAddress silently swallows errors from deriveDefaultAddresses
pkg/client-lib/funding.go:301-310
func (a *service) newAddress(
ctx context.Context,
) (onchainAddr string, offchainAddr, boardingAddr *types.Address, err error) {
key, err := a.wallet.NewKey(ctx)
if err != nil {
return "", nil, nil, err
}
onchainAddr, offchainAddr, boardingAddr, _, err = a.deriveDefaultAddresses(*key)
return onchainAddr, offchainAddr, boardingAddr, nil // ← returns nil, not err
}The named return err is assigned by deriveDefaultAddresses, but then explicitly overwritten with nil in the return statement. If deriveDefaultAddresses fails (e.g., TapTree() errors, invalid network params), the caller gets zero-value addresses with a nil error. This will cause silent corruption downstream — wrong addresses used for receiving funds, change outputs, boarding.
Fix: return onchainAddr, offchainAddr, boardingAddr, err
🔧 singleKeyID constant declared but never used
pkg/client-lib/wallet/singlekey/bitcoin_wallet.go:30
const singleKeyID = "single"NewKey and GetKey return &wallet.KeyRef{PubKey: w.privateKey.PubKey()} without setting Id. So KeyRef.Id is always "". This means:
types.Address.KeyIDis always empty (populated fromkey.IdinderiveDefaultAddresses)getAddresses()deduplication viaseenKeys[key.Id]works by accident for singlekey (empty string deduplicates), but semantically theIdfield is documented as meaningful
Either use singleKeyID when constructing KeyRef, or remove the constant. I'd recommend using it:
return &wallet.KeyRef{Id: singleKeyID, PubKey: w.privateKey.PubKey()}, nil⚠️ unroll.go:addInputs — good bug fix, but verify VTXO tapscripts are always populated
pkg/client-lib/unroll.go:536-540
The old code derived tapscripts from the wallet key, which was wrong for VTXOs with custom/delegated scripts. Now it correctly uses utxo.Tapscripts. This is a genuine bug fix. However, ensure that utxo.Tapscripts is always populated for all VTXO sources (indexer responses, local state). If any code path produces a types.Utxo without Tapscripts, this will now fail with a parse error instead of silently using the wrong script — which is actually better (fail-loud > silent corruption).
⚠️ Cross-repo status unchanged
No companion PR on arkade-os/go-sdk yet. The go-sdk still imports WalletService with the old interface methods. My previous concern stands — this must be coordinated before the go-sdk bumps its arkd dependency.
✅ Good changes in new commits
deriveDefaultAddressesextraction: Eliminates the duplication I flagged. Single source of truth for address derivation. Clean.KeyReftype hides private key:WalletServiceno longer exposes*btcec.PrivateKeythrough the interface. Private key access is internal to the wallet impl. This is a meaningful security improvement for the interface contract.KeyOptionvariadic pattern: Clean extensibility for future HD wallet options without breaking the interface.ListKeysmethod: Necessary forgetAddressesto work with multi-key wallets. Correct abstraction.types.Address.KeyID: Good addition — callers can now trace which wallet key produced an address.e2e_test.goTestDelegateRefresh: Updated correctly to useGetKey+aliceKey.PubKeyinstead of the oldGetKeyPairdestructuring.
Summary
| Finding | Severity |
|---|---|
newAddress swallows deriveDefaultAddresses error |
🚨 Bug — silent corruption risk |
singleKeyID unused, KeyRef.Id always empty |
🔧 Should fix |
| go-sdk companion PR still missing | |
addInputs tapscript fix |
✅ Correct |
Requesting changes for the error-swallowing bug. The rest is solid.
🤖 Reviewed by Arkana
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
pkg/client-lib/wallet/wallet.go (1)
15-18:⚠️ Potential issue | 🟠 MajorMake the key handle contract stable and explicit.
KeyRef.Idis the only thing callers can persist, but the current contract still treats it as arbitrary andGetKeydoes not require that same handle back in a discoverable way. That leaves HD implementations without a clear round-trip path and keeps the API implicitly single-key-friendly. I’d tighten this by documentingIdas stable/unique and either making it an explicitGetKeyparameter or adding a typed selector likeWithKeyID(...).Based on learnings, the intended flow is “new key → persist derivation handle → reopen the same key later,” and the single-key wallet should use a fixed constant handle.
Also applies to: 30-31
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/client-lib/wallet/wallet.go` around lines 15 - 18, KeyRef.Id must be declared and treated as a stable, unique handle for persistent key lookup; update the KeyRef docs to state Id is a stable/unique identifier that callers may persist and must round-trip, then change the wallet API so GetKey accepts that explicit handle (e.g., GetKey(ctx, keyID string) or add a typed selector WithKeyID(keyID string) used by existing GetKey/GetKeys flows) and ensure single-key implementations use a fixed constant handle; update usages/comments referencing KeyRef.Id, GetKey, and any key creation paths to persist and consume the same handle so HD and single-key wallets have a clear discoverable round-trip.pkg/client-lib/wallet/singlekey/bitcoin_wallet.go (1)
46-67:⚠️ Potential issue | 🟠 MajorPopulate
KeyRef.Idin the single-key backend.Both methods drop the stable handle and only return the pubkey, so downstream
KeyIDvalues stay empty and callers cannot round-trip the reference later. This file already defines the fixed handle; it just never gets used.Proposed fix
func (w *bitcoinWallet) NewKey( ctx context.Context, opts ...wallet.KeyOption, ) (*wallet.KeyRef, error) { if w.walletData == nil { return nil, fmt.Errorf("wallet not initialized") } if w.IsLocked() { return nil, fmt.Errorf("wallet is locked") } - return &wallet.KeyRef{PubKey: w.privateKey.PubKey()}, nil + return &wallet.KeyRef{ + Id: singleKeyID, + PubKey: w.privateKey.PubKey(), + }, nil } func (w *bitcoinWallet) GetKey( ctx context.Context, opts ...wallet.KeyOption, ) (*wallet.KeyRef, error) { if w.walletData == nil { return nil, fmt.Errorf("wallet not initialized") } if w.IsLocked() { return nil, fmt.Errorf("wallet is locked") } - return &wallet.KeyRef{PubKey: w.privateKey.PubKey()}, nil + return &wallet.KeyRef{ + Id: singleKeyID, + PubKey: w.privateKey.PubKey(), + }, nil }Based on learnings, the single-key wallet is expected to return a fixed constant handle so callers can persist and later reuse that key reference.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/client-lib/wallet/singlekey/bitcoin_wallet.go` around lines 46 - 67, The NewKey and GetKey methods on bitcoinWallet currently return wallet.KeyRef with only PubKey set, leaving KeyRef.Id empty; update both bitcoinWallet.NewKey and bitcoinWallet.GetKey to also populate KeyRef.Id with the single-key backend's fixed handle (the fixed handle variable defined in this file, e.g. singleKeyHandle) so callers can persist and round-trip the reference; ensure the returned struct is &wallet.KeyRef{Id: singleKeyHandle, PubKey: w.privateKey.PubKey()} and keep existing nil checks and lock checks unchanged.
🤖 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/wallet/wallet_test.go`:
- Around line 66-73: The test currently only asserts the public key matches; add
assertions on the returned key handle (KeyRef.Id) to ensure it is non-empty and
stable across calls: after walletSvc.GetKey(t.Context()) assert key.Id is not
empty, then call walletSvc.NewKey(...) and walletSvc.ListKeys(...) (or the
existing test positions around lines using GetKey/NewKey/ListKeys) and assert
the same KeyRef.Id is returned each time and is non-empty; update the assertions
in the spots referenced (GetKey, NewKey, ListKeys) so tests fail if an empty or
differing KeyRef.Id is returned.
---
Duplicate comments:
In `@pkg/client-lib/wallet/singlekey/bitcoin_wallet.go`:
- Around line 46-67: The NewKey and GetKey methods on bitcoinWallet currently
return wallet.KeyRef with only PubKey set, leaving KeyRef.Id empty; update both
bitcoinWallet.NewKey and bitcoinWallet.GetKey to also populate KeyRef.Id with
the single-key backend's fixed handle (the fixed handle variable defined in this
file, e.g. singleKeyHandle) so callers can persist and round-trip the reference;
ensure the returned struct is &wallet.KeyRef{Id: singleKeyHandle, PubKey:
w.privateKey.PubKey()} and keep existing nil checks and lock checks unchanged.
In `@pkg/client-lib/wallet/wallet.go`:
- Around line 15-18: KeyRef.Id must be declared and treated as a stable, unique
handle for persistent key lookup; update the KeyRef docs to state Id is a
stable/unique identifier that callers may persist and must round-trip, then
change the wallet API so GetKey accepts that explicit handle (e.g., GetKey(ctx,
keyID string) or add a typed selector WithKeyID(keyID string) used by existing
GetKey/GetKeys flows) and ensure single-key implementations use a fixed constant
handle; update usages/comments referencing KeyRef.Id, GetKey, and any key
creation paths to persist and consume the same handle so HD and single-key
wallets have a clear discoverable round-trip.
🪄 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: 7d327e5a-404a-40ba-aef8-26e82a4ad6ba
📒 Files selected for processing (10)
internal/test/e2e/e2e_test.gopkg/client-lib/asset.gopkg/client-lib/batch_session.gopkg/client-lib/funding.gopkg/client-lib/send.gopkg/client-lib/types/types.gopkg/client-lib/unroll.gopkg/client-lib/wallet/singlekey/bitcoin_wallet.gopkg/client-lib/wallet/wallet.gopkg/client-lib/wallet/wallet_test.go
🚧 Files skipped from review as they are similar to previous changes (4)
- pkg/client-lib/batch_session.go
- pkg/client-lib/unroll.go
- pkg/client-lib/asset.go
- internal/test/e2e/e2e_test.go
| key, err := walletSvc.GetKey(t.Context()) | ||
| require.NoError(t, err) | ||
| require.NotNil(t, key.PubKey) | ||
|
|
||
| prvkeyBytes, err := hex.DecodeString(seed) | ||
| require.NoError(t, err) | ||
| expectedPrvkey, _ := btcec.PrivKeyFromBytes(prvkeyBytes) | ||
| require.True(t, key.PubKey.IsEqual(expectedPrvkey.PubKey())) |
There was a problem hiding this comment.
Add assertions for the key handle, not just the pubkey.
These tests would still pass if the backend returned an empty KeyRef.Id, which is exactly the regression in the current single-key implementation. Please assert that the id is non-empty and stable across GetKey, NewKey, and ListKeys.
Based on learnings, callers are expected to persist a stable key handle/path and reuse it later, so the tests should validate that part of the API too.
Also applies to: 111-118, 157-165
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/client-lib/wallet/wallet_test.go` around lines 66 - 73, The test
currently only asserts the public key matches; add assertions on the returned
key handle (KeyRef.Id) to ensure it is non-empty and stable across calls: after
walletSvc.GetKey(t.Context()) assert key.Id is not empty, then call
walletSvc.NewKey(...) and walletSvc.ListKeys(...) (or the existing test
positions around lines using GetKey/NewKey/ListKeys) and assert the same
KeyRef.Id is returned each time and is non-empty; update the assertions in the
spots referenced (GetKey, NewKey, ListKeys) so tests fail if an empty or
differing KeyRef.Id is returned.
There was a problem hiding this comment.
Arkana Incremental Review — Merge Commit (168a35c)
🤖 Reviewed by Arkana
Scope: Single merge commit bringing master into the PR branch. Incorporates #999 (swept vtxo finalization tests) and #1001 (indexer admin endpoints).
Finding: Clean merge — no conflict resolutions in any PR-owned files:
pkg/client-lib/wallet/— unchangedpkg/client-lib/service.go,funding.go,send.go,batch_session.go,unroll.go,asset.go,ark_sdk.go— unchangedpkg/client-lib/types/types.go— unchangedinternal/test/e2e/utils_test.go,single_batch_smoke_test.go— unchanged
All incoming changes are from already-merged master commits, not from this PR. No new concerns beyond my previous reviews.
Prior review status: My earlier feedback on KeyRef naming, deriveDefaultAddresses error propagation, and KeyID population still applies. No regressions introduced by this merge.
There was a problem hiding this comment.
Arkana Incremental Review — Commit 6b8aa5f ("Fixes after review")
Focused on delta since my last review (168a35c).
Changes reviewed
-
pkg/client-lib/funding.go:309— ✅ Critical fix applied.return onchainAddr, offchainAddr, boardingAddr, nil→return ..., err. This was the silent error-swallowing bug I flagged in my Apr 10 review.deriveDefaultAddressesfailures are now correctly propagated. Good. -
pkg/client-lib/wallet/singlekey/bitcoin_wallet.go:28— ✅ Removes deadsingleKeyIDconstant. Verified no remaining references in the codebase. -
pkg/client-lib/wallet/wallet_test.go:68— ✅ Addsrequire.NotNil(t, key)guard before accessingkey.PubKey. Proper defensive testing. -
internal/test/e2e/e2e_test.go— ✅ Migrates the two remainingsetupArkSDKwithPublicKeytest cases ("vtxo already swept", "vtxo expired but not swept") tosetupClient. Verified zero remaining references tosetupArkSDK/setupArkSDKwithPublicKeyin e2e tests. The explorer construction removal is correct —alice.SignTransactionnow handles explorer internally.
Verdict
All prior findings addressed. The funding.go error propagation fix was the most important — that was a real bug that could silently corrupt address derivation in production.
newAddress, deriveDefaultAddresses) and transaction signing flows (SignTransaction). These are protocol-adjacent. Human reviewer should confirm the full integration path before merge.
🤖 Reviewed by Arkana
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/client-lib/funding.go (1)
67-72:⚠️ Potential issue | 🟠 Major
Balancestill assumes a single key.After switching to
a.getAddresses(ctx), the slices here can contain one entry per wallet key, but the aggregation below still runsgetOffchainBalanceand the full onchain sweep once per key and then stops after onlynbWorkersresults. With more than one key, that will ignore later addresses and/or double-count shared totals.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/client-lib/funding.go` around lines 67 - 72, The Balance method currently assumes a single key and only processes up to nbWorkers results, which causes later addresses from getAddresses(ctx) (onchainAddrs, offchainAddrs, boardingAddrs, redeemAddrs) to be ignored or double-counted; modify Balance to iterate over each address in those slices and aggregate totals across all addresses: call getOffchainBalance for each offchain address and sum the returned balances, perform the onchain sweep across all onchainAddrs (or run per-address and sum) rather than once-per-key then stopping early, and update the concurrency/worker logic so it schedules work for the full address lists and collects all results (do not short-circuit after nbWorkers). Ensure you update references inside Balance to correctly handle slices from getAddresses and to merge per-address results into the final Balance struct without duplication.
♻️ Duplicate comments (1)
pkg/client-lib/wallet/wallet_test.go (1)
63-75:⚠️ Potential issue | 🟡 MinorStill missing assertions on
KeyRef.Id.These tests only prove pubkey equality, so the current regression where the single-key backend returns an empty handle would still pass. Please assert that
Idis non-empty and stable acrossGetKey,NewKey, andListKeys.Based on learnings, callers are expected to persist a stable key handle/path and reuse it later.
Also applies to: 109-120, 154-167
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/client-lib/wallet/wallet_test.go` around lines 63 - 75, Update the tests (e.g., TestGetKey and the other test blocks around 109-120 and 154-167) to assert KeyRef.Id is non-empty and stable: after calling walletSvc.GetKey(t.Context()) capture key.Id and require it is not empty, then call walletSvc.NewKey / walletSvc.ListKeys (or call GetKey again) and assert the returned KeyRef.Id equals the previously captured id to ensure the single-key backend returns a persistent handle; locate the assertions near newUnlockedTestWallet, GetKey, NewKey, and ListKeys invocations and add checks for KeyRef.Id non-empty and equality across calls.
🤖 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/wallet/singlekey/bitcoin_wallet.go`:
- Around line 44-66: The NewKey and GetKey (and ListKeys) on bitcoinWallet
currently return KeyRef with only PubKey, leaving KeyRef.Id empty; add a stable
non-empty constant identifier (e.g. a package-level const like singleKeyID) and
populate KeyRef.Id with that value in bitcoinWallet.NewKey, bitcoinWallet.GetKey
and bitcoinWallet.ListKeys so the single-key wallet always returns the same
fixed handle/path that callers can persist and reuse (ensure the same constant
is referenced in all three methods).
---
Outside diff comments:
In `@pkg/client-lib/funding.go`:
- Around line 67-72: The Balance method currently assumes a single key and only
processes up to nbWorkers results, which causes later addresses from
getAddresses(ctx) (onchainAddrs, offchainAddrs, boardingAddrs, redeemAddrs) to
be ignored or double-counted; modify Balance to iterate over each address in
those slices and aggregate totals across all addresses: call getOffchainBalance
for each offchain address and sum the returned balances, perform the onchain
sweep across all onchainAddrs (or run per-address and sum) rather than
once-per-key then stopping early, and update the concurrency/worker logic so it
schedules work for the full address lists and collects all results (do not
short-circuit after nbWorkers). Ensure you update references inside Balance to
correctly handle slices from getAddresses and to merge per-address results into
the final Balance struct without duplication.
---
Duplicate comments:
In `@pkg/client-lib/wallet/wallet_test.go`:
- Around line 63-75: Update the tests (e.g., TestGetKey and the other test
blocks around 109-120 and 154-167) to assert KeyRef.Id is non-empty and stable:
after calling walletSvc.GetKey(t.Context()) capture key.Id and require it is not
empty, then call walletSvc.NewKey / walletSvc.ListKeys (or call GetKey again)
and assert the returned KeyRef.Id equals the previously captured id to ensure
the single-key backend returns a persistent handle; locate the assertions near
newUnlockedTestWallet, GetKey, NewKey, and ListKeys invocations and add checks
for KeyRef.Id non-empty and equality across calls.
🪄 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: da620916-c88f-4287-a59c-b98d81f028d0
📒 Files selected for processing (4)
internal/test/e2e/e2e_test.gopkg/client-lib/funding.gopkg/client-lib/wallet/singlekey/bitcoin_wallet.gopkg/client-lib/wallet/wallet_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/test/e2e/e2e_test.go
4 participants
This replaces the methods to derive and get addresses from the client's wallet interface with raw methods to get and derive new pubkeys (the single key wallet returns always the same keypair). The methods to derive addresses became internal methods of the client.
EDIT: took the occasion to do some cleanup in the e2e tests
Please @sekulicd @bitcoin-coder-bob review.
Summary by CodeRabbit
Refactor
Behavioral
Tests