add FinalizePendingTxs #60
Conversation
WalkthroughThe PR adds support for finalizing pending transactions and querying pending offchain transactions across the SDK. It introduces the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant arkClient
participant Wallet
participant Indexer
participant TransportClient
User->>arkClient: FinalizePendingTxs(ctx, createdAfter)
arkClient->>arkClient: listPendingSpentVtxosFromIndexer()
arkClient->>Indexer: GetVtxos(pendingOnly=true, createdAfter)
Indexer-->>arkClient: pending vtxos
loop for each pending vtxo
arkClient->>arkClient: resolve tapscripts & construct inputs
arkClient->>arkClient: makeGetPendingTxIntent(inputs, leafProofs, arkFields)
arkClient->>Wallet: sign checkpoint transaction
Wallet-->>arkClient: signed checkpoint
arkClient->>TransportClient: GetPendingTx(proof, message)
TransportClient-->>arkClient: AcceptedOffchainTx
arkClient->>arkClient: finalizeTx(ctx, acceptedTx)
arkClient->>TransportClient: SubmitTx / finalize
TransportClient-->>arkClient: arkTxid
end
arkClient-->>User: []finalized txids
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
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 |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
client.go (1)
834-902:FinalizePendingTxsflow is reasonable but hides per-tx failures.Logic looks sound: you gather non-swept vtxos (optionally filtered by
createdAfter), enrich them with tapscripts, build aGetPendingTxintent, and finalize each returnedAcceptedOffchainTx. However, anyfinalizeTxerror is only logged and the method still returns successfully with a (possibly partial) set oftxids.If the caller needs to know that some pending txs failed to finalize, consider:
- returning both
[]stringand an aggregated error (e.g.,fmt.Errorf("finalized %d/%d pending txs: %w", ...)), or- at least returning a non-nil error when all finalizations fail.
This can be a follow-up improvement if current API is intentionally “best-effort”.
client/client.go (2)
20-24: Add documentation for the exported struct.The
AcceptedOffchainTxstruct is exported and part of the public API. Consider adding a godoc comment explaining its purpose and documenting each field to improve API usability.Example documentation:
+// AcceptedOffchainTx represents an accepted off-chain transaction with its +// finalization details. type AcceptedOffchainTx struct { + // Txid is the transaction identifier Txid string + // FinalArkTx is the finalized Ark transaction FinalArkTx string + // SignedCheckpointTxs contains the signed checkpoint transactions SignedCheckpointTxs []string }
52-52: Add documentation for the interface method.The
GetPendingTxmethod is part of the publicTransportClientinterface. Consider adding a godoc comment explaining its purpose and parameters.Example documentation:
+ // GetPendingTx retrieves pending transactions for the given proof and message. GetPendingTx(ctx context.Context, proof, message string) ([]AcceptedOffchainTx, error)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (6)
ark_sdk.go(2 hunks)client.go(3 hunks)client/client.go(2 hunks)client/grpc/client.go(1 hunks)client/rest/client.go(1 hunks)go.mod(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-13T07:46:07.938Z
Learnt from: louisinger
Repo: arkade-os/go-sdk PR: 6
File: client.go:1194-1211
Timestamp: 2025-08-13T07:46:07.938Z
Learning: The listenForBoardingTxs() function in client.go is started from the Unlock method in base_client.go when WithTransactionFeed is enabled and UtxoMaxAmount is non-zero, not from the LoadArkClient methods.
Applied to files:
client.go
🧬 Code graph analysis (2)
client/grpc/client.go (4)
client/client.go (1)
AcceptedOffchainTx(20-24)client/rest/service/model_get_pending_tx_request.go (1)
GetPendingTxRequest(21-23)api-spec/protobuf/gen/ark/v1/service.pb.go (3)
GetPendingTxRequest(1237-1242)GetPendingTxRequest(1255-1255)GetPendingTxRequest(1270-1272)api-spec/protobuf/gen/ark/v1/types.pb.go (3)
Intent(441-447)Intent(460-460)Intent(475-477)
client.go (3)
client/client.go (1)
AcceptedOffchainTx(20-24)types/types.go (1)
Vtxo(80-94)store/sql/sqlc/queries/models.go (1)
Vtxo(37-52)
🔇 Additional comments (6)
go.mod (1)
7-9: Ark-lib version bump looks fine; ensure tests against new API surface.The dependency update is straightforward; just make sure all ark-lib–related changes (intents, pending-tx, finalize APIs) are covered by tests/integration runs with this exact version.
client.go (2)
354-391: Good reuse offinalizeTxinSendOffChain.Centralizing the “sign checkpoints + finalize on server” logic via
a.finalizeTxkeeps the off-chain send path consistent with the new pending-tx flow; the surrounding verification (verifySignedArk/verifySignedCheckpoints) is preserved.
2206-2220:makeGetPendingTxIntentTTL choice and structure look fine.Building a dedicated
IntentMessageTypeGetPendingTxwith a 10‑minuteExpireAtand reusingmakeIntent(no outputs) is consistent with the other intent helpers and keeps the request lightweight; no issues spotted here.ark_sdk.go (1)
3-8: Interface extension forFinalizePendingTxsis coherent with implementation.Adding
FinalizePendingTxs(ctx context.Context, createdAfter *time.Time) ([]string, error)toArkClient(with a*time.Timefilter) aligns with thearkClientimplementation and provides a clear, optional “created-after” cutoff for finalizing pending txs. No further changes needed at the interface layer.Also applies to: 12-57
client/rest/client.go (1)
379-405: RESTGetPendingTximplementation correctly maps toAcceptedOffchainTx.The request wiring (
Intent{Message, Proof}) and response mapping (ArkTxid,FinalArkTx,SignedCheckpointTxs→client.AcceptedOffchainTx) look correct and consistent with the transport interface and gRPC implementation.client/grpc/client.go (1)
379-404: gRPCGetPendingTxcorrectly wires request/response toAcceptedOffchainTx.The construction of
GetPendingTxRequestwith an embeddedIntentand the mapping ofresp.GetPendingTxs()intoclient.AcceptedOffchainTx(ArkTxid, FinalArkTx, SignedCheckpointTxs) is straightforward and matches the REST client, so the transport layer remains consistent.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
client.go (1)
834-938: Consider surfacing partial failures fromFinalizePendingTxsThe overall flow—gathering (non‑swept, optionally date‑filtered) vtxos, chunking into intents, calling
GetPendingTx, and then reusingfinalizeTx—is coherent and matches the intent‑based design.One behavioral concern: when
finalizeTxfails for a given pending tx, the error is only logged and the loop continues, whileFinalizePendingTxsstill returns([]string{successfulTxids}, nil). Callers have no way to tell that some eligible pending txs failed to finalize.Consider returning richer information, for example:
- a slice of successes and a slice of txids that failed, and/or
- an aggregated error when at least one finalize call failed,
so that callers can react (retry, notify the user, etc.) instead of relying solely on logs.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
api-spec/protobuf/gen/ark/v1/service.pb.gois excluded by!**/*.pb.go,!**/gen/**
📒 Files selected for processing (4)
buf.gen.yaml(1 hunks)client.go(3 hunks)client/client.go(2 hunks)client/grpc/client.go(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- buf.gen.yaml
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-13T07:46:07.938Z
Learnt from: louisinger
Repo: arkade-os/go-sdk PR: 6
File: client.go:1194-1211
Timestamp: 2025-08-13T07:46:07.938Z
Learning: The listenForBoardingTxs() function in client.go is started from the Unlock method in base_client.go when WithTransactionFeed is enabled and UtxoMaxAmount is non-zero, not from the LoadArkClient methods.
Applied to files:
client.go
🧬 Code graph analysis (2)
client/grpc/client.go (3)
client/client.go (1)
AcceptedOffchainTx(20-24)api-spec/protobuf/gen/ark/v1/service.pb.go (5)
GetPendingTxRequest(1237-1245)GetPendingTxRequest(1258-1258)GetPendingTxRequest(1273-1275)GetPendingTxRequest_Intent(1297-1299)GetPendingTxRequest_Intent(1301-1301)api-spec/protobuf/gen/ark/v1/types.pb.go (3)
Intent(441-447)Intent(460-460)Intent(475-477)
client.go (5)
client/client.go (2)
AcceptedOffchainTx(20-24)Input(93-96)types/types.go (1)
Vtxo(80-94)api-spec/protobuf/gen/ark/v1/types.pb.go (6)
Vtxo(128-145)Vtxo(158-158)Vtxo(173-175)Input(76-82)Input(95-95)Input(110-112)store/sql/sqlc/queries/models.go (1)
Vtxo(37-52)explorer/service.go (1)
Input(86-90)
🔇 Additional comments (4)
client/grpc/client.go (1)
379-406: GetPendingTx gRPC wiring and mapping look correctThe request correctly wraps the intent in
GetPendingTxRequest_Intent, and the response is mapped cleanly into[]client.AcceptedOffchainTx. No issues from a correctness or style standpoint.client/client.go (1)
20-24: AcceptedOffchainTx and TransportClient.GetPendingTx API look well-shapedThe
AcceptedOffchainTxstruct cleanly models the off-chain acceptance data, and extendingTransportClientwithGetPendingTxis consistent with the new finalization flow. The SubmitTx TODO comment documents the intended future consolidation.Also applies to: 48-52
client.go (2)
380-390: Centralizing off-chain finalization viafinalizeTxin SendOffChain looks goodReusing
finalizeTxhere keeps the checkpoint-signing +FinalizeTxRPC logic in one place and preserves the existingWithTransactionFeedshort‑circuit behavior.
2221-2235:makeGetPendingTxIntentmatches existing intent helper patternsEncoding a
GetPendingTxMessagewith a short (10‑minute) expiry and reusingmakeIntentfor proof construction is consistent with the other intent helpers (makeRegisterIntent,makeDeleteIntent) and looks correct.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
base_client.go (1)
709-741: Consider factoring out shared address→script logic.
listPendingSpentVtxosFromIndexerduplicates the offchain address decoding and P2TR script construction fromlistVtxosFromIndexer. A small helper (e.g.,offchainVtxoScripts(ctx)returning[]string) would reduce duplication and keep the two call sites in sync if address handling ever changes.client.go (1)
834-909: Clarify swept-vtxo handling and partial-failure behavior inFinalizePendingTxs.Two small points:
- The comment says “filter out swept vtxos”, but the loop only filters by
createdAfter. Either add an explicitif vtxo.Swept { continue }or update the comment so it matches behavior.- On
finalizeTxfailures you log and continue, returning only successfully finalized txids. That’s reasonable, but it might be worth documenting in the method comment or API docs so callers know errors are best-effort and not fatal for the whole batch.indexer/opts.go (1)
22-30:pendingOnlyflag and accessors are straightforward; consider optional validation.The new
pendingOnlyfield withWithPendingOnly/GetPendingOnlyis consistent with the existing spendable/spent/recoverable flags. If you expectpendingOnlyto be combined only with certain modes (e.g., implicitly “spent”), an optional guard or doc comment here could prevent accidental incompatible combinations, but it’s not strictly required since the server enforces semantics.Also applies to: 90-96
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
api-spec/protobuf/gen/ark/v1/indexer.pb.gois excluded by!**/*.pb.go,!**/gen/**go.sumis excluded by!**/*.sum
📒 Files selected for processing (10)
api-spec/openapi/swagger/ark/v1/indexer.openapi.json(2 hunks)base_client.go(1 hunks)buf.gen.yaml(1 hunks)client.go(3 hunks)go.mod(1 hunks)indexer/grpc/client.go(1 hunks)indexer/opts.go(2 hunks)indexer/rest/client.go(1 hunks)indexer/rest/service/api_indexer_service.go(3 hunks)indexer/rest/service/model_get_vtxos_request.go(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- buf.gen.yaml
- go.mod
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-13T07:46:07.938Z
Learnt from: louisinger
Repo: arkade-os/go-sdk PR: 6
File: client.go:1194-1211
Timestamp: 2025-08-13T07:46:07.938Z
Learning: The listenForBoardingTxs() function in client.go is started from the Unlock method in base_client.go when WithTransactionFeed is enabled and UtxoMaxAmount is non-zero, not from the LoadArkClient methods.
Applied to files:
base_client.goclient.go
🧬 Code graph analysis (2)
base_client.go (4)
types/types.go (1)
Vtxo(80-94)api-spec/protobuf/gen/ark/v1/types.pb.go (3)
Vtxo(128-145)Vtxo(158-158)Vtxo(173-175)store/sql/sqlc/queries/models.go (1)
Vtxo(37-52)indexer/opts.go (1)
GetVtxosRequestOption(22-30)
client.go (5)
client/client.go (2)
AcceptedOffchainTx(20-24)Input(93-96)types/types.go (1)
Vtxo(80-94)api-spec/protobuf/gen/ark/v1/types.pb.go (6)
Vtxo(128-145)Vtxo(158-158)Vtxo(173-175)Input(76-82)Input(95-95)Input(110-112)store/sql/sqlc/queries/models.go (1)
Vtxo(37-52)explorer/service.go (1)
Input(86-90)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Sdk unit tests
- GitHub Check: Sdk integration tests
🔇 Additional comments (8)
client.go (3)
380-391: Centralizing finalization viafinalizeTxlooks good.Reusing
finalizeTxhere removes duplication and ensures SendOffChain uses the same signing/finalization path as pending-tx finalization. Using the returnedtxidfor the non-feed case is consistent, given it equalsarkTxid.
911-930:finalizeTxerror handling is now correct.Signing failures now propagate as errors instead of returning
("", nil), so callers won’t treat failed finalizations as success. The helper cleanly encapsulates the finalize flow for both immediate and deferred use cases.
2213-2227:makeGetPendingTxIntentis consistent with existing intent builders.The new GetPendingTx message wiring (message type + 10‑minute expiry) and delegation to
makeIntentmatches the pattern used for register/delete intents, so it keeps the intent construction logic centralized.indexer/grpc/client.go (1)
352-360:PendingOnlywiring in gRPC GetVtxos is consistent.Propagating
opt.GetPendingOnly()into theGetVtxosRequest.PendingOnlyfield aligns with the existing spendable/spent/recoverable flags and correctly exposes the new filter over gRPC.indexer/rest/client.go (1)
248-253: REST GetVtxos now correctly forwardsPendingOnly.Chaining
.RecoverableOnly(opt.GetRecoverableOnly()).PendingOnly(opt.GetPendingOnly())keeps the REST client in sync with the option struct and gRPC client, so all transports now support the pending-only filter.indexer/rest/service/api_indexer_service.go (1)
1176-1187: Pending-only support in REST service client is wired correctly.Adding
pendingOnly *booltoApiIndexerServiceGetVtxosRequest, thePendingOnly(bool)builder, and emitting the"pendingOnly"query parameter when non-nil mirrors the existing boolean filters and matches the OpenAPI contract.Also applies to: 1219-1223, 1292-1294
api-spec/openapi/swagger/ark/v1/indexer.openapi.json (1)
654-661: OpenAPIpendingOnlyadditions are coherent with the new behavior.Defining a
pendingOnlyquery parameter on/v1/indexer/vtxosand adding the correspondingGetVtxosRequest.pendingOnlyboolean property (both described as “Include only spent vtxos that are not finalized.”) cleanly exposes the new filter at the API level and aligns with the client implementations.Also applies to: 982-985
indexer/rest/service/model_get_vtxos_request.go (1)
21-35:PendingOnlyfield and accessors follow the existing generated pattern.The new
PendingOnly *boolfield, its Get*/Has*/Set* helpers, and the ToMap serialization branch are consistent with the other optional bool flags on GetVtxosRequest, so the model correctly supports emittingpendingOnlyonly when set.Also applies to: 118-148, 286-296
implements arkade-os/arkd#832
@altafan please review
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.