Split client-lib and client-wallet#1078
Open
altafan wants to merge 12 commits into
Open
Conversation
…t-wallet from client-lib and leave there only types, interface and functions
Contributor
WalkthroughConsolidates client API into pkg/client-lib, adds batch-session and offchain-tx modules (join/settle/send/issue/reissue/burn/pending/verify), rewires gRPC indexer/explorer, updates CLI and e2e tests, and removes legacy wallet/send/batch service code. ChangesClient-lib refactor and orchestration
Sequence Diagram(s)sequenceDiagram
participant Caller
participant Client
participant BatchHandler
participant Arkd
Caller->>Client: BuildAndSignRegisterIntent(proof)
Client->>Arkd: RegisterIntent RPC
Arkd-->>Client: intentId
Caller->>Client: GetEventStream(topics)
Client-->>BatchHandler: events channel
BatchHandler->>Arkd: SubmitTreeNonces/Signatures
Arkd-->>BatchHandler: BatchFinalization
BatchHandler->>Client: SubmitSignedForfeit/Commitment
Client->>Arkd: FinalizeTx
Arkd-->>Caller: BatchTxRes
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related issues
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 20
🧹 Nitpick comments (7)
pkg/client-lib/offchain-tx/verify.go (2)
11-45: ⚡ Quick winVerify the signed/original checkpoint sets are symmetric.
The loop only walks
indexedOriginalCheckpoints, so any signed checkpoint that does not correspond to an original is silently accepted (and duplicates withinsignedCheckpointsare silently collapsed by the map write at line 31). If a caller relies on a successful return as "exactly my checkpoints were signed", that invariant is not enforced.Consider asserting a strict pairing — equal counts and rejecting orphan/duplicate signed entries — so the function's guarantee matches its name.
♻️ Proposed change
+ if len(originalCheckpoints) != len(signedCheckpoints) { + return fmt.Errorf( + "checkpoint count mismatch: %d originals vs %d signed", + len(originalCheckpoints), len(signedCheckpoints), + ) + } + for _, cp := range signedCheckpoints { signedPtx, err := psbt.NewFromRawBytes(strings.NewReader(cp), true) if err != nil { return err } - indexedSignedCheckpoints[signedPtx.UnsignedTx.TxID()] = signedPtx + txid := signedPtx.UnsignedTx.TxID() + if _, dup := indexedSignedCheckpoints[txid]; dup { + return fmt.Errorf("duplicate signed checkpoint %s", txid) + } + indexedSignedCheckpoints[txid] = signedPtx } for txid, originalPtx := range indexedOriginalCheckpoints { signedPtx, ok := indexedSignedCheckpoints[txid] if !ok { return fmt.Errorf("signed checkpoint %s not found", txid) } if err := verifyOffchainTx(originalPtx, signedPtx, signerpubkey); err != nil { return err } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/client-lib/offchain-tx/verify.go` around lines 11 - 45, In VerifySignedCheckpointTxs ensure the sets are strictly symmetric: when building indexedSignedCheckpoints detect duplicate signed entries (track seen txids and return an error if a duplicate is found), assert the counts match (or after building both maps iterate indexedSignedCheckpoints to ensure every txid has a corresponding entry in indexedOriginalCheckpoints and return an error for any orphan signed txid), and keep calling verifyOffchainTx for each matched pair (refer to VerifySignedCheckpointTxs, indexedOriginalCheckpoints, indexedSignedCheckpoints, and verifyOffchainTx to locate the code).
11-47: 💤 Low valueMake the
signerPubKeyparameter naming consistent.
VerifySignedCheckpointTxsusessignerpubkeywhileVerifySignedTxusessignerPubKey. Pick one (the camelCased variant matches Go conventions) for both exported signatures.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/client-lib/offchain-tx/verify.go` around lines 11 - 47, Rename the parameter signerpubkey in the exported function VerifySignedCheckpointTxs to the camelCase signerPubKey to match VerifySignedTx; update the function signature and all internal references (e.g., the verifyOffchainTx call and any other uses of signerpubkey) to use signerPubKey so the exported API and internal usage are consistent with Go naming conventions.pkg/client-lib/offchain-tx/pending.go (1)
36-47: ⚡ Quick winSurface per-tx finalize failures to the caller instead of swallowing them.
Per-tx failures are only logged via the global logrus logger; the caller receives
(txids, nil)and cannot distinguish "no pending txs", "all finalized", and "all failed". For a library function this hides actionable errors from the embedding application and complicates retry/observability.Consider returning an aggregated error alongside the successful txids (e.g. via
errors.Join) so callers can decide how to react.♻️ Proposed refactor
- txids := make([]string, 0, len(pendingTxs)) - for _, tx := range pendingTxs { - txid, _, err := finalizeTx(ctx, args.Client, args.SignTx, tx) - if err != nil { - log.WithError(err).Errorf("failed to finalize pending tx: %s", tx.Txid) - continue - } - txids = append(txids, txid) - } - - return txids, nil + txids := make([]string, 0, len(pendingTxs)) + var finalizeErrs []error + for _, tx := range pendingTxs { + txid, _, err := finalizeTx(ctx, args.Client, args.SignTx, tx) + if err != nil { + log.WithError(err).Errorf("failed to finalize pending tx: %s", tx.Txid) + finalizeErrs = append(finalizeErrs, fmt.Errorf("tx %s: %w", tx.Txid, err)) + continue + } + txids = append(txids, txid) + } + + return txids, errors.Join(finalizeErrs...)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/client-lib/offchain-tx/pending.go` around lines 36 - 47, The loop over pendingTxs calls finalizeTx and currently swallows per-tx errors (logging them) and returns (txids, nil); change the function signature to return ([]string, error), collect any per-tx errors into a slice (wrap each with context including tx.Txid and the finalizeTx error), keep logging if desired, and at the end return the successful txids plus an aggregated error (use errors.Join or fmt.Errorf with %-joined errors) instead of nil so callers can distinguish partial failures; references: pendingTxs, finalizeTx, txids.pkg/client-lib/batch-session/collaborative_exit.go (1)
48-54: 💤 Low valueConsider validating
ChangeAddragainst the network too.
Receiver.Tois decoded againstnetParams, butChangeAddris only checked for non-empty. A malformed or wrong-network change address will fail later (deeper inselectFunds/tx building) with a less actionable error. Decoding it here keeps validation symmetric and fails fast.Proposed fix
- netParams := clientlib.ToBitcoinNetwork(clientlib.NetworkFromString(a.ServerInfo.Network)) - if _, err := btcutil.DecodeAddress(a.Receiver.To, &netParams); err != nil { - return fmt.Errorf("invalid receiver address") - } - if len(a.ChangeAddr) <= 0 { - return fmt.Errorf("missing change address") - } + netParams := clientlib.ToBitcoinNetwork(clientlib.NetworkFromString(a.ServerInfo.Network)) + if _, err := btcutil.DecodeAddress(a.Receiver.To, &netParams); err != nil { + return fmt.Errorf("invalid receiver address") + } + if len(a.ChangeAddr) <= 0 { + return fmt.Errorf("missing change address") + } + if _, err := btcutil.DecodeAddress(a.ChangeAddr, &netParams); err != nil { + return fmt.Errorf("invalid change address") + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/client-lib/batch-session/collaborative_exit.go` around lines 48 - 54, Validate the ChangeAddr against the same network as Receiver.To to fail fast: after computing netParams (via clientlib.ToBitcoinNetwork(clientlib.NetworkFromString(a.ServerInfo.Network))) and decoding Receiver.To with btcutil.DecodeAddress, also call btcutil.DecodeAddress(a.ChangeAddr, &netParams) and return a descriptive error (e.g., "invalid change address") on failure; this keeps validation symmetric with Receiver.To and prevents downstream failures in selectFunds/tx building.pkg/client-lib/batch-session/batch_session_opts_test.go (1)
46-51: 💤 Low valueSubtest name says "zero or negative" but only tests
0.Either rename the subtest or add a
WithRetries(-1)case so the assertion matches the intent.Proposed fix
t.Run("zero or negative", func(t *testing.T) { opts := newOptions() err := WithRetries(0).apply(opts) require.Error(t, err) require.Contains(t, err.Error(), "retry num must be in range [1, 3]") + + err = WithRetries(-1).apply(opts) + require.Error(t, err) + require.Contains(t, err.Error(), "retry num must be in range [1, 3]") })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/client-lib/batch-session/batch_session_opts_test.go` around lines 46 - 51, The subtest named "zero or negative" only exercises WithRetries(0); update the test to match its intent by either renaming the subtest to "zero" or adding a negative-case call to WithRetries(-1).apply and asserting the same error; locate the t.Run block in batch_session_opts_test.go where WithRetries(0).apply is used and add (or change) the call to WithRetries(-1).apply with the same require.Error and require.Contains checks so the subtest truly covers "zero or negative".pkg/client-lib/batch-session/handler/types.go (1)
11-34: 💤 Low valueConsistent use of named returns across the interface.
Some methods name their boolean return (
OnTreeNoncesAggregated,OnTreeNoncesusesigned bool) while semantically similar methods (OnBatchStarted,OnTreeSigningStarted) leave returns unnamed. Documenting allboolreturns with a name (or none of them) makes the contract clearer for implementers — e.g., what does theboolfromOnBatchStartedandOnTreeSigningStartedmean?🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/client-lib/batch-session/handler/types.go` around lines 11 - 34, The interface mixes named and unnamed boolean returns; make them consistent by naming all boolean return values (or none). Update the signature of OnBatchStarted and OnTreeSigningStarted to name their bool return (e.g., accepted bool or signed bool) to match OnTreeNoncesAggregated and OnTreeNonces, and ensure the chosen identifier clearly describes the meaning (use the same identifier across all methods). Keep the other return types and method names (OnBatchStarted, OnTreeSigningStarted, OnTreeNoncesAggregated, OnTreeNonces) unchanged so implementers can unambiguously understand the boolean semantics.pkg/client-lib/explorer/utils.go (1)
141-147: ⚡ Quick winMake
supportedType.String()deterministic.Map iteration order is random, so this output can vary between runs (noisy logs/errors/tests). Sort keys before joining.
Suggested fix
import ( "bytes" "context" "encoding/hex" "errors" "fmt" "net" "net/url" "os" + "sort" "strings" "syscall" @@ func (t supportedType[V]) String() string { types := make([]string, 0, len(t)) for tt := range t { types = append(types, tt) } + sort.Strings(types) return strings.Join(types, " | ") }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/client-lib/explorer/utils.go` around lines 141 - 147, supportedType[V].String() currently iterates a map whose order is non-deterministic; make the output deterministic by collecting the keys into the slice (as done into the variable types), sorting that slice (use sort.Strings(types)) and then joining it, so the String() method always returns keys in a stable order; update the supportedType[V].String() implementation to sort the types slice before calling strings.Join.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/ark-cli/main.go`:
- Line 577: The struct literal uses the wrong field name casing: change the
construction of clientlib.ExistingControlAsset in main.go (where
controlAssetPolicy is set via
clientlib.ControlAsset(clientlib.ExistingControlAsset{...})) to use the actual
field name declared in pkg/client-lib/types.go (ExistingControlAsset.Id) — i.e.,
replace the ID: controlAssetId entry with Id: controlAssetId (or alternatively,
update the struct definition to use ID if you prefer the exported convention; be
sure to update all call sites such as the call to clientlib.ControlAsset
accordingly).
In `@pkg/client-lib/batch-session/batch_session_opts.go`:
- Around line 78-83: The docstring for WithExpiryThreshold and the
implementation in filterVtxosByExpiry disagree: the comment says "exclude vtxos
expiring sooner than the threshold" but the code currently keeps near-expiry
vtxos. Fix by making the runtime match the documented contract: keep the Option
WithExpiryThreshold(threshold int64) and options.expiryThreshold as-is, but
change filterVtxosByExpiry to exclude any vtxo whose timeUntilExpiry <
o.expiryThreshold (i.e., filter out near-expiry coins), and update the comment
if you instead prefer the opposite semantics (or rename the Option to something
like WithMinExpiry or WithExpiryInclusionThreshold). Also ensure any
callers/tests that rely on the old behavior are updated to the new exclusion
semantics.
In `@pkg/client-lib/batch-session/batch_session_types.go`:
- Around line 80-82: The validateForRegister routine currently rejects
registrations when a.Cosigners is empty; change this to only enforce non-empty
cosigners if the intent actually requires tree signing. Update
validateForRegister to check the tree-signing flag on the intent (the existing
"tree signing" field referenced in the contract—e.g., TreeSigningRequired or
TreeSigningEnabled) and only return fmt.Errorf("missing cosigners") when that
flag is true and a.Cosigners is nil/empty; leave note-only intents with no
cosigners allowed.
In `@pkg/client-lib/batch-session/batch_session.go`:
- Around line 55-67: The current use of indexedOutputs (map keyed by
txOut.PkScript) inside the loop that builds indexedOutputs and the per-leaf
matching that uses a single break causes duplicate PkScript entries to collapse
and only one offchain output to be associated per identical script; update the
logic to preserve counts and positions instead of a set: when converting outputs
via Output.ToTxOut() capture either a slice of indexes or a map[string][]int (or
similar) keyed by hex.EncodeToString(txOut.PkScript) to record all matching
output indices, and then change the per-leaf matching code (the loop that
currently breaks after the first match) to iterate over all recorded indices for
that script and map/assign each corresponding offchain output (so all duplicates
are preserved); reference symbols: indexedOutputs, Output.ToTxOut,
txOut.PkScript and the per-leaf match/break logic.
- Around line 186-188: The sleep in the retry loop blocks shutdown because it
uses time.Sleep(100 * time.Millisecond); replace it with a context-aware wait
(use select with ctx.Done() and time.After(100 * time.Millisecond)) so the loop
in the function that references retryCount and maxRetry will return immediately
when ctx is canceled; ensure deleteIntent() is still called only when
appropriate and check ctx after the wait to avoid performing work after
cancellation.
In `@pkg/client-lib/batch-session/handler/default_handler.go`:
- Around line 194-200: The code indexes commitmentTx.UnsignedTx.TxOut[0] without
checking length; add a guard after creating commitmentTx to verify
len(commitmentTx.UnsignedTx.TxOut) > 0 and return a descriptive error if zero.
Update the flow around the variables commitmentTx, batchOutput and
batchOutputAmount so you only access TxOut[0] after the length check, returning
the error to the caller instead of allowing a panic.
- Around line 482-498: The code currently allows len(vtxos)>0 with
connectorTree==nil to pass validation; change validateVtxoTree (or the block
around connectorTree/VTXO checks in default_handler.go) to fail fast when
refreshed VTXOs arrive without a connector tree by returning an error if
len(vtxos) > 0 && connectorTree == nil; reference the connectorTree and vtxos
variables and ensure this check precedes any connectorTree.Validate() and
Leaves() logic so OnBatchFinalization and createAndSignForfeits cannot skip
forfeits for those VTXOs.
- Around line 507-508: The call to
btcutil.DecodeAddress(h.ServerInfo.ForfeitAddress, nil) should pass explicit
chain params to avoid panics and enforce the expected HRP; replace the nil
defaultNet with the chaincfg.Params obtained via
clientlib.ToBitcoinNetwork(h.network) (or the existing helper that returns chain
params) when decoding into parsedForfeitAddr, and ensure subsequent use
(forfeitPkScript / txscript.PayToAddrScript) handles the error returned so
decoding fails fast on mismatched networks; update the DecodeAddress invocation
and its error handling around parsedForfeitAddr accordingly.
In `@pkg/client-lib/batch-session/handler/handler.go`:
- Around line 125-131: The TreeSignatureEvent branch currently errors if
vtxoTree is nil, but when signing is disabled we should silently ignore these
events; update the clientlib.TreeSignatureEvent case to first check the
signVtxoTree flag (the same boolean controlling signing) and if it's false
simply continue, otherwise proceed with the existing step check and the vtxoTree
nil check (i.e., only return "vtxo tree not initialized" when signVtxoTree is
true and step == treeNoncesAggregated); reference the
clientlib.TreeSignatureEvent case, the signVtxoTree variable, the step variable
and treeNoncesAggregated constant to locate the change.
- Around line 59-66: The current code spawns a goroutine per event to forward
notify.Event to options.replayEventsCh (the anonymous func), which is
unnecessary and racy; replace that goroutine with a direct non-blocking send:
check options.replayEventsCh != nil and then perform a select { case
options.replayEventsCh <- notify.Event: default: } inline (no go func) so events
are forwarded without extra goroutines or closure-timing issues.
In `@pkg/client-lib/batch-session/handler/utils.go`:
- Around line 109-116: The code currently slices output.PkScript[2:] without
verifying its length or shape which can panic for undersized/malformed scripts;
update the loop over leaves/leaf.UnsignedTx.TxOut to first verify the script is
at least 2 + len(vtxoTapKey) bytes (and optionally check the 2-byte
taproot/version prefix if you want stricter validation) before doing bytes.Equal
against vtxoTapKey, and if the checks fail simply continue (treat as non-match)
instead of slicing — reference symbols: leaves, leaf.UnsignedTx.TxOut,
output.PkScript, vtxoTapKey, receiver.Amount.
In `@pkg/client-lib/batch-session/redeem_notes.go`:
- Around line 22-59: RedeemNotesArgs.validate() currently only checks
ServerInfo.Network but must mirror the JoinBatch handler's requirements: update
RedeemNotesArgs.validate() to also require ServerInfo.ForfeitPubKey and
ServerInfo.ForfeitAddress be non-empty and verify ForfeitPubKey is valid hex
(parse/DecodeString) before returning nil; keep the existing Network check. Also
note the summation logic uses note.Note.Value (uint32) added into a uint64
accumulator—no change needed for overflow handling but you can leave a brief
comment if desired. Reference: RedeemNotesArgs.validate(),
ServerInfo.ForfeitPubKey, ServerInfo.ForfeitAddress, and the JoinBatch handler
Args.validate() in default_handler.go.
In `@pkg/client-lib/batch-session/utils_test.go`:
- Around line 12-19: The test's exact-threshold case is flaky because it builds
clientlib.Vtxo instances using time.Now() and compares equality at the threshold
constant; update the test to avoid using an exact boundary by either (a)
creating the boundary cases using explicit margins (e.g. use
threshold-1*time.Second and threshold+1*time.Second when constructing
vtxoExpiring3Days) or (b) inject a deterministic clock into the filter and use a
fixed base time for all vtxo ExpiresAt values; modify the variables named
vtxoExpiring1Day, vtxoExpiring3Days, vtxoExpiring5Days, and vtxoAlreadyExpired
(and any assertions around them) to use the chosen approach so the test becomes
deterministic.
In `@pkg/client-lib/batch-session/utils.go`:
- Around line 74-76: The branch that handles io.EOF currently returns a freshly
created error (fmt.Errorf("connection closed by server")), which prevents
callers from matching the exported sentinel; change that return to use the
exported sentinel clientlib.ErrConnectionClosedByServer (or wrap it with %w if
you need extra context) so errors.Is(err, clientlib.ErrConnectionClosedByServer)
succeeds; update the return in the EOF branch where the function returns "", "",
-1, nil, nil, fmt.Errorf(...) to instead return "", "", -1, nil, nil,
clientlib.ErrConnectionClosedByServer (or fmt.Errorf("connection closed by
server: %w", clientlib.ErrConnectionClosedByServer) if wrapping).
In `@pkg/client-lib/client/client.go`:
- Around line 23-28: grpcClient's single shared listenerId causes races because
GetEventStream() can be called multiple times and reconnects reset that shared
field; change ownership of the stream ID to be per-event-stream rather than on
grpcClient. Update the implementation so that the listenerId is stored on the
event-stream object returned by GetEventStream (or maintained in a map keyed by
that stream instance/ID) and protect it with the existing listenerMu; adjust
GetEventStream to set the stream-local listenerId on connect/reconnect and
update ModifyStreamTopics and OverwriteStreamTopics to use the stream-local
listenerId (or lookup by stream key) instead of grpcClient.listenerId so topic
updates target the correct stream and no longer race across streams. Ensure
reconnect logic clears/sets only that stream's listenerId and remove or stop
using grpcClient.listenerId.
In `@pkg/client-lib/explorer/service.go`:
- Around line 276-279: GetAddressesEvents can panic because e.listeners is nil
before Start() (or on the noTracking path); change GetAddressesEvents (method
explorerSvc.GetAddressesEvents) to create the channel first, only call
e.listeners.add(ch) when e.listeners != nil, and otherwise return an inert
(closed) channel so callers can safely range/receive without panicking;
reference e.listeners and Start() when locating the fix.
In `@pkg/client-lib/internal/utils/listener_test.go`:
- Around line 259-266: Replace the fixed time.Sleep by waiting for a readiness
condition: poll received.Load() with a bounded timeout (e.g., using time.After
or context with deadline) and a short sleep between polls until received.Load()
> 0 or the timeout elapses; once readiness is observed (or timeout fails the
test), call b.Close(), wg.Wait(), and then use require.Greater to assert
deliveries. Update the code around received, b.Close(), wg.Wait() and the
require.Greater assertion to use this polling/readiness loop instead of
time.Sleep.
In `@pkg/client-lib/internal/utils/stream_retry.go`:
- Around line 467-472: The codes.Unknown branch in shouldReconnect currently
returns false,0 (non-retryable) unless the message contains "524", which makes
transient transport/proxy hiccups terminal; update the codes.Unknown handling in
the switch inside shouldReconnect so that after checking for "524" it returns a
retry decision (e.g., true with a short backoff) instead of false,0; refer to
the shouldReconnect function and the switch on st.Code() / the codes.Unknown
case and return a retryable outcome (use the existing backoff policy or a small
default like 1s) to allow brief failures to recover.
In `@pkg/client-lib/internal/utils/utils.go`:
- Around line 44-47: The comparator passed to sort.SliceStable over the vtxos
slice uses a non-strict negated Before check which returns true for equal
ExpiresAt values and breaks the "less" contract; update both sort.SliceStable
calls that reference vtxos and ExpiresAt to use a strict comparison by returning
vtxos[i].ExpiresAt.After(vtxos[j].ExpiresAt) so ordering is newest-first (oldest
last) and ties do not make both directions true.
In `@pkg/client-lib/service.go`:
- Around line 118-137: The code currently sets utxoTime = time.Now().Unix() when
u.Status.BlockTime == 0, which causes RedeemableAt to be derived from wall-clock
time; instead, when u.Status.BlockTime == 0 set createdAt = time.Time{} and do
NOT compute RedeemableAt (leave it zero-valued) so it remains unset until a real
block time is available; update the Utxo construction to only set RedeemableAt
using time.Unix(u.Status.BlockTime, 0).Add(...) when u.Status.BlockTime != 0,
keeping references to the existing variables (createdAt, utxoTime, RedeemableAt,
Outpoint, signingClosure) intact.
---
Nitpick comments:
In `@pkg/client-lib/batch-session/batch_session_opts_test.go`:
- Around line 46-51: The subtest named "zero or negative" only exercises
WithRetries(0); update the test to match its intent by either renaming the
subtest to "zero" or adding a negative-case call to WithRetries(-1).apply and
asserting the same error; locate the t.Run block in batch_session_opts_test.go
where WithRetries(0).apply is used and add (or change) the call to
WithRetries(-1).apply with the same require.Error and require.Contains checks so
the subtest truly covers "zero or negative".
In `@pkg/client-lib/batch-session/collaborative_exit.go`:
- Around line 48-54: Validate the ChangeAddr against the same network as
Receiver.To to fail fast: after computing netParams (via
clientlib.ToBitcoinNetwork(clientlib.NetworkFromString(a.ServerInfo.Network)))
and decoding Receiver.To with btcutil.DecodeAddress, also call
btcutil.DecodeAddress(a.ChangeAddr, &netParams) and return a descriptive error
(e.g., "invalid change address") on failure; this keeps validation symmetric
with Receiver.To and prevents downstream failures in selectFunds/tx building.
In `@pkg/client-lib/batch-session/handler/types.go`:
- Around line 11-34: The interface mixes named and unnamed boolean returns; make
them consistent by naming all boolean return values (or none). Update the
signature of OnBatchStarted and OnTreeSigningStarted to name their bool return
(e.g., accepted bool or signed bool) to match OnTreeNoncesAggregated and
OnTreeNonces, and ensure the chosen identifier clearly describes the meaning
(use the same identifier across all methods). Keep the other return types and
method names (OnBatchStarted, OnTreeSigningStarted, OnTreeNoncesAggregated,
OnTreeNonces) unchanged so implementers can unambiguously understand the boolean
semantics.
In `@pkg/client-lib/explorer/utils.go`:
- Around line 141-147: supportedType[V].String() currently iterates a map whose
order is non-deterministic; make the output deterministic by collecting the keys
into the slice (as done into the variable types), sorting that slice (use
sort.Strings(types)) and then joining it, so the String() method always returns
keys in a stable order; update the supportedType[V].String() implementation to
sort the types slice before calling strings.Join.
In `@pkg/client-lib/offchain-tx/pending.go`:
- Around line 36-47: The loop over pendingTxs calls finalizeTx and currently
swallows per-tx errors (logging them) and returns (txids, nil); change the
function signature to return ([]string, error), collect any per-tx errors into a
slice (wrap each with context including tx.Txid and the finalizeTx error), keep
logging if desired, and at the end return the successful txids plus an
aggregated error (use errors.Join or fmt.Errorf with %-joined errors) instead of
nil so callers can distinguish partial failures; references: pendingTxs,
finalizeTx, txids.
In `@pkg/client-lib/offchain-tx/verify.go`:
- Around line 11-45: In VerifySignedCheckpointTxs ensure the sets are strictly
symmetric: when building indexedSignedCheckpoints detect duplicate signed
entries (track seen txids and return an error if a duplicate is found), assert
the counts match (or after building both maps iterate indexedSignedCheckpoints
to ensure every txid has a corresponding entry in indexedOriginalCheckpoints and
return an error for any orphan signed txid), and keep calling verifyOffchainTx
for each matched pair (refer to VerifySignedCheckpointTxs,
indexedOriginalCheckpoints, indexedSignedCheckpoints, and verifyOffchainTx to
locate the code).
- Around line 11-47: Rename the parameter signerpubkey in the exported function
VerifySignedCheckpointTxs to the camelCase signerPubKey to match VerifySignedTx;
update the function signature and all internal references (e.g., the
verifyOffchainTx call and any other uses of signerpubkey) to use signerPubKey so
the exported API and internal usage are consistent with Go naming conventions.
🪄 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: 87b45da4-dbee-444f-8c3f-44988024de97
⛔ Files ignored due to path filters (3)
pkg/ark-cli/go.sumis excluded by!**/*.sumpkg/client-lib/go.sumis excluded by!**/*.sumpkg/client-wallet/go.sumis excluded by!**/*.sum
📒 Files selected for processing (128)
go.modinternal/test/e2e/delegate_utils_test.gointernal/test/e2e/e2e_test.gointernal/test/e2e/single_batch_smoke_test.gointernal/test/e2e/utils_test.gopkg/ark-cli/go.modpkg/ark-cli/main.gopkg/ark-lib/tree/signer.gopkg/client-lib/asset.gopkg/client-lib/batch-session/batch_session.gopkg/client-lib/batch-session/batch_session_opts.gopkg/client-lib/batch-session/batch_session_opts_test.gopkg/client-lib/batch-session/batch_session_test.gopkg/client-lib/batch-session/batch_session_types.gopkg/client-lib/batch-session/collaborative_exit.gopkg/client-lib/batch-session/collaborative_exit_test.gopkg/client-lib/batch-session/handler/default_handler.gopkg/client-lib/batch-session/handler/handler.gopkg/client-lib/batch-session/handler/handler_opts.gopkg/client-lib/batch-session/handler/types.gopkg/client-lib/batch-session/handler/utils.gopkg/client-lib/batch-session/intent.gopkg/client-lib/batch-session/intent_test.gopkg/client-lib/batch-session/redeem_notes.gopkg/client-lib/batch-session/redeem_notes_test.gopkg/client-lib/batch-session/settle.gopkg/client-lib/batch-session/settle_test.gopkg/client-lib/batch-session/utils.gopkg/client-lib/batch-session/utils_test.gopkg/client-lib/batch_session.gopkg/client-lib/batch_session_handler.gopkg/client-lib/batch_session_opts.gopkg/client-lib/client.gopkg/client-lib/client/client.gopkg/client-lib/client/grpc/client.gopkg/client-lib/client/reconnect_test.gopkg/client-lib/client/types.gopkg/client-lib/explorer/connection_pool.gopkg/client-lib/explorer/listeners.gopkg/client-lib/explorer/mempool/explorer.gopkg/client-lib/explorer/mempool/utils_test.gopkg/client-lib/explorer/opts.gopkg/client-lib/explorer/service.gopkg/client-lib/explorer/service_test.gopkg/client-lib/explorer/types.gopkg/client-lib/explorer/utils.gopkg/client-lib/explorer/utils_test.gopkg/client-lib/go.modpkg/client-lib/identity.gopkg/client-lib/indexer.gopkg/client-lib/indexer/cache.gopkg/client-lib/indexer/cache_test.gopkg/client-lib/indexer/client.gopkg/client-lib/indexer/paginated_fetch_test.gopkg/client-lib/indexer/reconnect_get_subscription_stream_test.gopkg/client-lib/indexer_opts.gopkg/client-lib/indexer_opts_test.gopkg/client-lib/internal/utils/listener_test.gopkg/client-lib/internal/utils/reconnect.gopkg/client-lib/internal/utils/stream_retry.gopkg/client-lib/internal/utils/stream_retry_test.gopkg/client-lib/internal/utils/types.gopkg/client-lib/internal/utils/utils.gopkg/client-lib/internal/utils/utils_test.gopkg/client-lib/offchain-tx/args.gopkg/client-lib/offchain-tx/asset.gopkg/client-lib/offchain-tx/asset_test.gopkg/client-lib/offchain-tx/build.gopkg/client-lib/offchain-tx/opts.gopkg/client-lib/offchain-tx/pending.gopkg/client-lib/offchain-tx/pending_test.gopkg/client-lib/offchain-tx/send.gopkg/client-lib/offchain-tx/send_test.gopkg/client-lib/offchain-tx/types.gopkg/client-lib/offchain-tx/utils.gopkg/client-lib/offchain-tx/utils_test.gopkg/client-lib/offchain-tx/verify.gopkg/client-lib/offchain-tx/verify_test.gopkg/client-lib/receiver_opts.gopkg/client-lib/receiver_opts_test.gopkg/client-lib/redemption/redeem.gopkg/client-lib/send.gopkg/client-lib/send_opts.gopkg/client-lib/service.gopkg/client-lib/service_opts.gopkg/client-lib/sign_opts.gopkg/client-lib/store/inmemory/config_store.gopkg/client-lib/store/service.gopkg/client-lib/store/service_test.gopkg/client-lib/types.gopkg/client-lib/types/types.gopkg/client-lib/unroll_ops.gopkg/client-lib/utils.gopkg/client-lib/utils_test.gopkg/client-lib/vtxos_opts.gopkg/client-lib/wallet.gopkg/client-wallet/asset.gopkg/client-wallet/batch_session.gopkg/client-wallet/example/README.mdpkg/client-wallet/example/alice_to_bob/alice_to_bob.gopkg/client-wallet/example/multi_connection_demo/multi_connection_demo.gopkg/client-wallet/funding.gopkg/client-wallet/funding_opts.gopkg/client-wallet/funding_opts_test.gopkg/client-wallet/go.modpkg/client-wallet/identity/identity.gopkg/client-wallet/identity/identity_test.gopkg/client-wallet/identity/store/file/store.gopkg/client-wallet/identity/store/inmemory/store.gopkg/client-wallet/identity/store/store.gopkg/client-wallet/identity/store/store_test.gopkg/client-wallet/identity/utils.gopkg/client-wallet/init.gopkg/client-wallet/send.gopkg/client-wallet/store/file/store.gopkg/client-wallet/store/file/types.gopkg/client-wallet/store/file/utils.gopkg/client-wallet/store/inmemory/store.gopkg/client-wallet/store/service.gopkg/client-wallet/store/service_test.gopkg/client-wallet/types.gopkg/client-wallet/types/interfaces.gopkg/client-wallet/types/types.gopkg/client-wallet/unroll.gopkg/client-wallet/unroll_ops.gopkg/client-wallet/utils.gopkg/client-wallet/wallet.gopkg/client-wallet/wallet_opts.go
💤 Files with no reviewable changes (18)
- pkg/client-lib/internal/utils/types.go
- pkg/client-lib/client/grpc/client.go
- pkg/client-lib/service_opts.go
- pkg/client-lib/store/inmemory/config_store.go
- pkg/client-lib/internal/utils/utils_test.go
- pkg/client-lib/explorer/mempool/utils_test.go
- pkg/client-lib/send_opts.go
- pkg/client-lib/receiver_opts_test.go
- pkg/client-lib/store/service.go
- pkg/client-lib/receiver_opts.go
- pkg/client-lib/sign_opts.go
- pkg/client-lib/asset.go
- pkg/client-lib/batch_session.go
- pkg/client-lib/batch_session_opts.go
- pkg/client-lib/send.go
- pkg/client-lib/batch_session_handler.go
- pkg/client-lib/explorer/mempool/explorer.go
- pkg/client-lib/internal/utils/reconnect.go
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/ark-cli/main.go (1)
757-767:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReject mixed or multi-onchain receiver payloads here.
This branch executes only
onchainReceivers[0]and returns, so extra onchain receivers, any offchain receivers in the same--receiverspayload, and any asset payload on the chosen onchain receiver are silently dropped. That turns a multi-recipient request into a partial send without surfacing an error.Suggested guard
if len(onchainReceivers) > 0 { + if len(onchainReceivers) != 1 || len(offchainReceivers) > 0 || len(onchainReceivers[0].Assets) > 0 { + return fmt.Errorf("mixed, multi-recipient, or asset onchain sends are not supported") + } res, err := client.CollaborativeExit( ctx.Context, onchainReceivers[0].To, onchainReceivers[0].Amount, )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/ark-cli/main.go` around lines 757 - 767, The code currently only processes onchainReceivers[0] and silently ignores extra onchain or offchain entries; add a guard before calling client.CollaborativeExit to validate the receivers payload: if len(onchainReceivers) > 1 OR (len(onchainReceivers) == 1 && len(offchainReceivers) > 0) OR the selected onchain receiver contains an asset field, return an error instead of proceeding. Locate the branch using onchainReceivers, client.CollaborativeExit, client.SendOffChain and printJSON and enforce these checks so mixed or multi-onchain receiver payloads are rejected explicitly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@pkg/ark-cli/main.go`:
- Around line 757-767: The code currently only processes onchainReceivers[0] and
silently ignores extra onchain or offchain entries; add a guard before calling
client.CollaborativeExit to validate the receivers payload: if
len(onchainReceivers) > 1 OR (len(onchainReceivers) == 1 &&
len(offchainReceivers) > 0) OR the selected onchain receiver contains an asset
field, return an error instead of proceeding. Locate the branch using
onchainReceivers, client.CollaborativeExit, client.SendOffChain and printJSON
and enforce these checks so mixed or multi-onchain receiver payloads are
rejected explicitly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 17b46261-6e70-4314-80b4-d61ecc623e32
📒 Files selected for processing (3)
internal/test/e2e/e2e_test.gopkg/ark-cli/main.gopkg/client-wallet/asset.go
![arkanaai[bot]](https://avatars.githubusercontent.com/u/170820085?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🔍 Arkana Code Review — refactor-client-lib (131 files, +12361/−10128)
Major refactor splitting client-lib into stateless protocol primitives + new client-wallet package. Architecture is sound, but several protocol-critical and security findings require attention before merge.
🔴 CRITICAL
C1. math.Abs(float64(...)) for satoshi change calculation
pkg/client-lib/offchain-tx/utils.go:513
changeAmount = uint64(math.Abs(float64(btcAmountToSelect)))float64 has 53 bits of mantissa. This is fundamentally wrong for money code — the round-trip int64 → float64 → uint64 can silently lose precision. Fix: changeAmount = uint64(-btcAmountToSelect) since this branch only executes when btcAmountToSelect < 0.
C2. Stale countSigningDone counter on partial failure in OnTreeNonces
pkg/client-lib/batch-session/handler/default_handler.go:301-313
When some signer sessions succeed but a later one fails, countSigningDone is incremented for the successes, then the error return aborts the batch. On retry with the same handler instance, the counter retains stale state from the failed attempt, potentially causing countSigningDone == len(h.SignerSessions) to trigger prematurely or never trigger.
🟠 HIGH — Security
H1. FinalizePendingTxs signs server-provided PSBTs without verification
pkg/client-lib/offchain-tx/pending.go:37-45
GetPendingTx returns transactions from the server. finalizeTx calls signTx on them directly — no verifyOffchainTx call. A malicious/compromised server can feed arbitrary PSBTs and get the client to sign them. Must verify against signer pubkey before signing.
H2. VerifySignedCheckpointTxs ignores extra server-injected checkpoints
pkg/client-lib/offchain-tx/verify.go:34-43
Iterates only originalCheckpoints, never checks len(signedCheckpoints) == len(originalCheckpoints). Extra checkpoints from the server are silently passed through to finalizeTx where the client signs ALL of them. Add a length equality check.
H3. verifyOffchainTx accepts any sighash type from server
pkg/client-lib/offchain-tx/utils.go:345
signedInput.SighashType is server-provided. A malicious server could use SIGHASH_NONE or SIGHASH_SINGLE, sign with that sighash, and verification passes. The client should enforce SIGHASH_DEFAULT (0x00) or SIGHASH_ALL to ensure the server's signature commits to all outputs.
H4. Forfeit address decoded with nil network params
pkg/client-lib/batch-session/handler/default_handler.go:507
parsedForfeitAddr, err := btcutil.DecodeAddress(h.ServerInfo.ForfeitAddress, nil)nil network params means no cross-network validation. A mainnet forfeit address on testnet (or vice versa) won't be caught. Use the correct network from h.network.
H5. Missing forfeit transactions when connector tree is nil
pkg/client-lib/batch-session/handler/default_handler.go:337-338, 482-498
If vtxos is non-empty but connectorTree is nil (server error), the code silently skips creating forfeit transactions. validateVtxoTree also passes silently. The client should error when vtxos exist but connector tree is missing — skipping forfeits breaks protocol enforcement.
🟠 HIGH — Explorer/Infra
H6. Explorer listeners drop events and permanently remove slow consumers
pkg/client-lib/explorer/listeners.go:37-39
If a listener channel is momentarily full, the event is dropped AND the listener is permanently removed. GetAddressesEvents() creates unbuffered channels, making this trivially triggerable. In a Bitcoin L2 wallet, missing UTXO events = incorrect balance.
H7. Explorer listeners.broadcast → remove has potential double-close panic
pkg/client-lib/explorer/listeners.go:31-53
broadcast() holds RLock, identifies channels to remove, spawns goroutine calling remove() with Lock. Between RLock release and Lock acquisition, clear() could close the same channels, causing a panic on double-close.
H8. Explorer GetFeeRate checks HTTP status AFTER json.Decode
pkg/client-lib/explorer/mempool/explorer.go:246-248
Non-200 responses with HTML error pages produce confusing JSON parse errors instead of the actual HTTP error. Check resp.StatusCode first, consistent with all other HTTP methods in the file.
🟡 MEDIUM
M1. Nonce reuse risk on batch retry
pkg/client-lib/batch-session/batch_session.go:125-201
signerSessions are created once before the retry loop. If tree.SignerSession has internal nonce state that becomes invalid after a failed batch, retrying with the same sessions could cause nonce reuse. In MuSig2, nonce reuse = private key extraction. Verify that SignerSession implementations are safe for reuse.
M2. Unsafe uint64 → int64 casts for amounts
pkg/client-lib/offchain-tx/utils.go:82, 111
vtxo.Amount and receiver.Amount are uint64. Values exceeding math.MaxInt64 wrap to negative, producing negative-value PSBT inputs/outputs. Add overflow checks.
M3. BatchFailedEvent can be silently swallowed by custom handlers
pkg/client-lib/batch-session/handler/handler.go:100-105
If a custom EventsHandler.OnBatchFailed() returns nil, the event loop continues and waits forever. Consider requiring non-nil return or adding a timeout.
M4. buildAndSignIntent — no bounds check on leafProofs index
pkg/client-lib/batch-session/utils.go:244-253
If leafProofs has fewer entries than proof.Inputs - 1, this panics with index-out-of-bounds. Add a bounds check.
M5. receiver_opts_test.go deleted — WithKeys signing override removed
The WithKeys(keys) cross-cutting option (56 lines) was removed entirely. External callers who used WithKeys to override signing keys for specific scripts will break. Intentional but worth documenting in migration notes.
🟡 MEDIUM — Cross-Repo Breakage
M6. This PR breaks ALL downstream consumers. Coordinated PRs required:
| Consumer | Impact | Files Affected |
|---|---|---|
| go-sdk | 🔴 Won't compile | 30+ files — client-lib/types, client-lib/identity, client/grpc, explorer/mempool, store, wallet all removed/renamed |
| bancod | 🔴 Won't compile | 5 files — client-lib/types, wallet.WalletService. Note: VtxoWithTapTree type removed entirely |
| demos/asset-demos | 🟠 Won't compile | 1 file each — ExistingControlAsset.ID renamed to .Id |
Are companion PRs for go-sdk and bancod already in progress?
🔵 LOW
pkg/client-lib/offchain-tx/args.go:106— Typo: "assset" (triple-s)pkg/client-lib/types/types.go—TxsUpdated.String()returns empty string (missing from map)pkg/client-lib/batch-session/handler/handler_opts.go:28—keysByScriptfield declared but never used (dead code)pkg/client-lib/batch-session/handler/utils.go:47-51— Magic number512should be a named constant (SEQUENCE_LOCKTIME_TYPE_FLAG)pkg/client-lib/explorer/mempool/explorer.go:409— One WebSocket per address is wasteful;addConnection()holds lock during network I/Opkg/client-wallet/wallet.go:13-14— Duplicate import alias (storetypesandtypesboth alias same package)
✅ What looks good
- Clean architectural split: stateless protocol primitives in
client-lib, stateful wallet inclient-wallet - All 18 e2e test functions preserved with correct API migration
- Deleted files all have clear replacements — no logic lost
- Reconnection/retry logic in
internal/utils/stream_retry.gois thorough and well-tested redemption/redeem.goandunrolllogic appear to be clean moves
🏁 Verdict: Request Changes
The architecture is solid, but H1-H5 are protocol-critical security issues that must be addressed before merge. In particular:
- H1 (signing unverified server PSBTs) and H2 (extra checkpoint injection) are exploitable by a malicious server
- H3 (sighash type not enforced) weakens transaction verification
- C1 (float64 for satoshis) is a correctness bug in money code
🤖 Reviewed by Arkana
Contributor
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 (2)
pkg/client-lib/offchain-tx/build.go (1)
189-191:⚠️ Potential issue | 🟠 Major | ⚡ Quick winApply output tap trees in the non-send builders too.
WithTxOutsTaprootTreepopulateso.outputsTapTree, but issuance, reissuance, and burn never calladdOutputsTaprootTree. Passing that option to these builders currently succeeds and then drops the BIP-371 metadata from the emitted outputs.Suggested fix
if err := addExtension(arkPtx, assetPacket, o.extraPackets); err != nil { return nil, err } +if err := addOutputsTaprootTree(arkPtx, o.outputsTapTree); err != nil { + return nil, err +} arkTx, err := arkPtx.B64Encode() if err != nil { return nil, err }Apply the same insertion in
BuildAndSignIssuanceTx,BuildAndSignReissuanceTx, andBuildAndSignBurnTx.Also applies to: 320-322, 403-405
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/client-lib/offchain-tx/build.go` around lines 189 - 191, The non-send builders (BuildAndSignIssuanceTx, BuildAndSignReissuanceTx, BuildAndSignBurnTx) are not applying the outputs taproot tree option even though WithTxOutsTaprootTree sets o.outputsTapTree, so BIP-371 metadata is dropped; fix by calling addOutputsTaprootTree (the same insertion used in the send builders) before addExtension in each of those functions so o.outputsTapTree is propagated into the created outputs, ensuring the taproot tree is applied when o.extraPackets or assetPacket extensions are added.pkg/client-lib/indexer/client.go (1)
787-807:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRestore zero-based paging in
paginatedFetch.This now starts from page
1and only stops whencurrent >= total. Everywhere else in this client,next == totalis treated as the terminal condition, so this implementation skips the first page and then walks one request past the last real page.Suggested fix
- pageIndex := int32(1) + pageIndex := int32(0) reqCount := 0 for { items, page, err := fetch(ctx, &arkv1.IndexerPageRequest{ Size: maxPageSize, Index: pageIndex, }) if err != nil { return nil, err } all = append(all, items...) reqCount++ - if page == nil || page.GetCurrent() >= page.GetTotal() { + if page == nil || page.GetNext() >= page.GetTotal() { break } if reqCount >= maxPages { return nil, fmt.Errorf("too many pages (%d), aborting", maxPages) } pageIndex = page.GetNext()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/client-lib/indexer/client.go` around lines 787 - 807, The paginatedFetch loop currently starts at pageIndex=1 and uses page.GetCurrent() to decide termination, which skips the first (zero) page and can overshoot the last page; change it to start at pageIndex := int32(0) and terminate when page == nil || page.GetNext() >= page.GetTotal() so we respect the client's convention that next==total is terminal. Update the loop in paginatedFetch (the block that calls fetch(ctx, &arkv1.IndexerPageRequest{...}) and references pageIndex, page.GetCurrent(), page.GetNext(), and page.GetTotal()) accordingly while keeping the existing reqCount/maxPages check.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/client-lib/indexer/client.go`:
- Around line 722-744: The current logic appends results from fetchPages for
scripts and outpoints separately into the vtxos slice (see vtxos, appendPages,
fetchPages and the two loops over o.Scripts and o.Outpoints), which can produce
duplicates when the same Vtxo appears in both batches; fix by deduplicating as
you append: maintain a seen map keyed by a stable Vtxo identifier (e.g. combine
the Outpoint fields like Txid+Vout or whatever unique fields exist on
clientlib.Vtxo) and in appendPages only add pageVtxos items whose key is not
already in seen, then return the de-duplicated vtxos in VtxosResponse.
---
Outside diff comments:
In `@pkg/client-lib/indexer/client.go`:
- Around line 787-807: The paginatedFetch loop currently starts at pageIndex=1
and uses page.GetCurrent() to decide termination, which skips the first (zero)
page and can overshoot the last page; change it to start at pageIndex :=
int32(0) and terminate when page == nil || page.GetNext() >= page.GetTotal() so
we respect the client's convention that next==total is terminal. Update the loop
in paginatedFetch (the block that calls fetch(ctx,
&arkv1.IndexerPageRequest{...}) and references pageIndex, page.GetCurrent(),
page.GetNext(), and page.GetTotal()) accordingly while keeping the existing
reqCount/maxPages check.
In `@pkg/client-lib/offchain-tx/build.go`:
- Around line 189-191: The non-send builders (BuildAndSignIssuanceTx,
BuildAndSignReissuanceTx, BuildAndSignBurnTx) are not applying the outputs
taproot tree option even though WithTxOutsTaprootTree sets o.outputsTapTree, so
BIP-371 metadata is dropped; fix by calling addOutputsTaprootTree (the same
insertion used in the send builders) before addExtension in each of those
functions so o.outputsTapTree is propagated into the created outputs, ensuring
the taproot tree is applied when o.extraPackets or assetPacket extensions are
added.
🪄 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: b5652d5d-eea9-4f23-b776-e99c004993a5
📒 Files selected for processing (30)
go.modpkg/client-lib/batch-session/batch_session.gopkg/client-lib/batch-session/batch_session_opts.gopkg/client-lib/batch-session/handler/default_handler.gopkg/client-lib/batch-session/handler/handler.gopkg/client-lib/batch-session/handler/handler_opts.gopkg/client-lib/batch-session/handler/utils.gopkg/client-lib/batch-session/redeem_notes.gopkg/client-lib/batch-session/redeem_notes_test.gopkg/client-lib/batch-session/settle.gopkg/client-lib/batch-session/settle_test.gopkg/client-lib/batch-session/utils.gopkg/client-lib/explorer/service.gopkg/client-lib/explorer/service_test.gopkg/client-lib/indexer/client.gopkg/client-lib/indexer/paginated_fetch_test.gopkg/client-lib/internal/utils/utils.gopkg/client-lib/offchain-tx/args.gopkg/client-lib/offchain-tx/build.gopkg/client-lib/offchain-tx/opts.gopkg/client-lib/offchain-tx/opts_test.gopkg/client-lib/offchain-tx/utils.gopkg/client-lib/offchain-tx/utils_test.gopkg/client-lib/service.gopkg/client-lib/types.gopkg/client-lib/utils.gopkg/client-lib/utils_test.gopkg/client-wallet/asset.gopkg/client-wallet/send.gopkg/client-wallet/unroll.go
💤 Files with no reviewable changes (1)
- pkg/client-lib/internal/utils/utils.go
✅ Files skipped from review due to trivial changes (1)
- pkg/client-lib/indexer/paginated_fetch_test.go
🚧 Files skipped from review as they are similar to previous changes (15)
- pkg/client-lib/batch-session/handler/utils.go
- pkg/client-lib/batch-session/batch_session_opts.go
- pkg/client-lib/batch-session/handler/handler_opts.go
- go.mod
- pkg/client-lib/explorer/service_test.go
- pkg/client-lib/batch-session/settle_test.go
- pkg/client-lib/batch-session/redeem_notes_test.go
- pkg/client-lib/batch-session/redeem_notes.go
- pkg/client-lib/batch-session/settle.go
- pkg/client-lib/offchain-tx/args.go
- pkg/client-lib/batch-session/batch_session.go
- pkg/client-lib/batch-session/utils.go
- pkg/client-lib/batch-session/handler/default_handler.go
- pkg/client-lib/batch-session/handler/handler.go
- pkg/client-lib/explorer/service.go
Comment on lines
722
to
+744
| @@ -735,10 +741,10 @@ func (a *grpcClient) paginatedGetVtxos( | |||
| return nil, err | |||
| } | |||
| } | |||
| return &indexer.VtxosResponse{Vtxos: vtxos}, nil | |||
| return &clientlib.VtxosResponse{Vtxos: vtxos}, nil | |||
Contributor
There was a problem hiding this comment.
Deduplicate results across script/outpoint batches.
The paginated path now queries scripts and outpoints separately and blindly appends both result sets. If a requested outpoint also belongs to one of the requested scripts, the same VTXO will be returned twice here even though the non-paginated path sends a single combined request.
Suggested fix
- var vtxos []clientlib.Vtxo
+ var vtxos []clientlib.Vtxo
+ seen := make(map[string]struct{})
appendPages := func(scripts []string, outpoints []clientlib.Outpoint) error {
pageVtxos, err := fetchPages(scripts, outpoints)
if err != nil {
return err
}
- vtxos = append(vtxos, pageVtxos...)
+ for _, vtxo := range pageVtxos {
+ key := fmt.Sprintf("%s:%d", vtxo.Outpoint.Txid, vtxo.Outpoint.VOut)
+ if _, ok := seen[key]; ok {
+ continue
+ }
+ seen[key] = struct{}{}
+ vtxos = append(vtxos, vtxo)
+ }
return nil
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/client-lib/indexer/client.go` around lines 722 - 744, The current logic
appends results from fetchPages for scripts and outpoints separately into the
vtxos slice (see vtxos, appendPages, fetchPages and the two loops over o.Scripts
and o.Outpoints), which can produce duplicates when the same Vtxo appears in
both batches; fix by deduplicating as you append: maintain a seen map keyed by a
stable Vtxo identifier (e.g. combine the Outpoint fields like Txid+Vout or
whatever unique fields exist on clientlib.Vtxo) and in appendPages only add
pageVtxos items whose key is not already in seen, then return the de-duplicated
vtxos in VtxosResponse.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR refactors the client-lib and extracts the stateless client wallet used for integration tests and as the base for the client-facing CLI.
The main purpose of this refactor is to break the dependency between the stateful client (go-sdk) and the stateless one (what's now pkg/client-wallet), and make them reuse shared types, functions, and interfaces instead for interoperability instead.
The secondary goal was to make the shared code agnostic to the tx signing so that funds locked by any kind of script can be provided. For this the original
VtxoandVtxoWithTaptreetypes have been merged into the first one, and a newSigningClosure script.Closurehas been added. This field is required when using the vtxo/utxo as input of a tx and represents the very tapleaf of the givenTapscriptsused for signing. The typetype SignTx func(ctx context.Context, tx string) (string, error)is also introduced to generalize and abstract the signing of a tx that ust always be provided by callers.In client-lib, all types and interfaces are now at "root" level, so, for example, they can be imported with
clientlib.Vtxoinstead oftypes.Vtxo.In client-lib's folders we can find:
client/- grpc implementation of theClientinterfaceindexer/- grpc implementation of theIndexerinterfaceexplorer/- mempool implementation of theExplorerinterfacebatch-session/- collection of functions to join a batch (low-levelJoinBatch,RegisterIntent,DeleteIntentand high-level orchestratorsSettle,CollaborativeExit,RedeemNotes)offchain-tx/- collection of functions to spend offchain (low-level builders for any kind of tx, high-level orchestrators for any tx)This adds a brand new
pkg/walletthat contains the stateless client wallet built on top of the client-lib types, interfaces and functions and that is used for integration tests and as base for the client-dacing CLI.Please @louisinger @sekulicd review.
Summary by CodeRabbit
New Features
Refactor