Chain Swap #369
Chain Swap #369
Conversation
|
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:
📝 WalkthroughWalkthroughAdds Ark↔BTC chain-swap functionality: new API endpoints and protobuf RPCs, domain model and SQLite persistence, Boltz client chain-swap methods, swap monitoring/handlers (MuSig2 & Taproot claim paths), validation/explorer utilities, recovery/refund flows, extensive E2E tests and a mock Boltz server. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant FulmineService
participant BoltzAPI
participant BTCNetwork
participant ARKNetwork
rect rgba(200,150,255,0.5)
Note over Client,FulmineService: Ark→BTC flow
Client->>FulmineService: CreateChainSwap(amount, btcAddress)
FulmineService->>BoltzAPI: CreateChainSwap(Ark→BTC)
BoltzAPI-->>FulmineService: {id, preimage, lockupAddr,...}
FulmineService->>ARKNetwork: Fund VHTLC
FulmineService->>FulmineService: Monitor swap
BoltzAPI->>FulmineService: UserLocked
FulmineService->>BoltzAPI: Get claim details / submit partial sigs
BoltzAPI->>BTCNetwork: Server locks BTC
FulmineService->>BTCNetwork: Broadcast claim tx
BTCNetwork-->>FulmineService: Confirmations
end
rect rgba(150,200,255,0.5)
Note over Client,FulmineService: BTC→Ark flow
Client->>FulmineService: CreateChainSwap(amount)
FulmineService->>BoltzAPI: CreateChainSwap(BTC→Ark)
BoltzAPI-->>FulmineService: {id, preimage,...}
Client->>BTCNetwork: Lock BTC to address
BoltzAPI->>FulmineService: UserLocked
FulmineService->>ARKNetwork: Claim VTXO
FulmineService->>BoltzAPI: Submit cooperative signature
BoltzAPI-->>FulmineService: Claim finalized
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 20
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
pkg/boltz/ws.go (1)
233-247:⚠️ Potential issue | 🟠 MajorGuard against zero/negative
retryIntervalto avoid tight retry loops.
Retryusestime.After(interval), soretryInterval <= 0will effectively spin and hammer the backend/CPU. Add a guard (default or error) before the first retry loop.🛠️ Suggested fix
func (boltz *Websocket) ConnectAndSubscribe(ctx context.Context, swapIds []string, retryInterval time.Duration) error { + if retryInterval <= 0 { + retryInterval = 5 * time.Second + } err := Retry(ctx, retryInterval, func(ctx context.Context) (bool, error) { err := boltz.Connect() if err != nil { return false, nilpkg/swap/go.mod (1)
10-15:⚠️ Potential issue | 🔴 CriticalDo not use unreleased go-sdk versions without documented justification.
This commit locks
pkg/swapto an unreleased pseudo-version (v0.8.2-0.20260121113217-12e71e4566e6, dated 2026-01-21) of go-sdk, yet the latest published tag is onlyv0.8.1(Oct 2025). The pseudo-version has no release notes, changelog, or documented reason for the bump. While go-sdk is actively used across swap handlers (swap.go, utils.go, batch_handler.go, chainswap_arkt_btc_handler.go) for client, indexer, and types imports, pinning to an undocumented unreleased commit creates an unmaintainable and unverifiable dependency. Either release the pinned go-sdk version with documented changes, or revert to the latest stable release (v0.8.1) and defer the bump to a future PR with clear justification.pkg/swap/swap.go (2)
71-94:⚠️ Potential issue | 🟠 MajorValidate
privateKeybefore callingPubKey()to prevent panic.
privateKey.PubKey()will panic ifprivateKeyis nil. Add a guard and return an explicit error.🛠️ Proposed fix
func NewSwapHandler( arkClient arksdk.ArkClient, transportClient client.TransportClient, indexerClient indexer.Indexer, boltzSvc *boltz.Api, esploraURL string, privateKey *btcec.PrivateKey, timeout uint32, ) (*SwapHandler, error) { + if privateKey == nil { + return nil, fmt.Errorf("private key is required") + } cfg, err := arkClient.GetConfigData(context.Background()) if err != nil { return nil, fmt.Errorf("failed to get config data: %w", err) }
1083-1113:⚠️ Potential issue | 🟠 MajorHandle errors/unknown swap types before dereferencing
refundResp.
refundRespcan be nil if the RPC call fails or ifswapTypeis unsupported, leading to a nil deref. Checkerrand add a default case.🛠️ Proposed fix
switch swapType { case SwapTypeSubmarine: refundResp, err = h.boltzSvc.RefundSubmarine(swapId, boltz.RefundSwapRequest{ Transaction: refundTx, Checkpoint: checkpointTx, }) case SwapTypeChain: refundResp, err = h.boltzSvc.RefundChainSwap(swapId, boltz.RefundSwapRequest{ Transaction: refundTx, Checkpoint: checkpointTx, }) + default: + return nil, nil, fmt.Errorf("unknown swapType %q", swapType) } + if err != nil { + return nil, nil, err + } + if refundResp == nil { + return nil, nil, fmt.Errorf("empty refund response") + }internal/infrastructure/db/service.go (2)
214-220:⚠️ Potential issue | 🟠 MajorMissing
chainSwapRepo.Close()in theClose()method.The
ChainSwapRepositoryinterface includes aClose()method, butservice.Close()doesn't call it. This could lead to resource leaks.🐛 Proposed fix
func (s *service) Close() { s.settingsRepo.Close() s.vhtlcRepo.Close() s.vtxoRolloverRepo.Close() s.swapRepo.Close() s.subscribedScriptRepo.Close() + s.chainSwapRepo.Close() }
57-93:⚠️ Potential issue | 🟠 MajorMissing
chainSwapRepoinitialization for badger database type will cause nil pointer dereference.The badger case (lines 58-93) initializes all repositories except
chainSwapRepo, leaving itnil. The sqlite case properly initializes it (line 171), but badger lacks both the initialization and any correspondingNewChainSwapRepositoryimplementation. WhenChainSwaps()is called with badger as the database type, it will returnnil, causing crashes in any consuming code.Either implement
NewChainSwapRepositoryfor badger or return an error during initialization if chain swaps are required.
🤖 Fix all issues with AI agents
In `@internal/core/application/service.go`:
- Around line 1437-1446: The function CreateChainSwapArkToBtc currently discards
the caller's context by creating context.Background(); instead, use the
passed-in context parameter so cancellation, deadlines, and tracing propagate.
Replace ctx := context.Background() with using the existing context argument for
calls such as s.isInitializedAndUnlocked(ctx) and any downstream operations
inside CreateChainSwapArkToBtc (and ensure any nested goroutines derive Contexts
via ctx). Also update any references within the method that expect a local ctx
to use the passed context variable name to preserve caller signals.
- Around line 1629-1636: The RefundChainSwap method on type Service is currently
a no-op; replace the TODO with either a proper implementation following the
listed flow (load chain swap from DB, locate the BTX transaction, claim it,
forward to boarding address, and call settle) using existing helpers (e.g.,
functions/methods that interact with the DB, find BTX txs, claim transactions,
send to boarding address, and settle) or, if the behavior is intentionally
unimplemented, return an explicit sentinel error (e.g.,
errors.New("RefundChainSwap: not implemented")) instead of nil so callers
receive a clear failure; update RefundChainSwap to return that error and ensure
callers handle it.
- Around line 2155-2164: The log and failure message when vtxos[0].Spent is true
are misleading; update the message used in log.Warn and chainSwap.Failed to
reflect ambiguity (e.g., "VTXO already spent; swap may have been claimed by
counterparty or refund failed") so it doesn't assume a refund failure, and keep
the existing DB update call via s.dbSvc.ChainSwaps().Update to persist that
clarified status; locate the branch handling vtxos[0].Spent (references: vtxos,
chainSwap.Failed, log.Warn, s.dbSvc.ChainSwaps().Update) and replace the current
hardcoded "failed to refund chain swap" text with the clarified message.
In `@internal/core/domain/chainswap.go`:
- Around line 48-65: IsComplete currently only marks ChainSwapClaimed,
ChainSwapRefunded, and ChainSwapFailed as terminal, but omits
ChainSwapRefundFailed and ChainSwapUserLockedFailed; update the
ChainSwap.IsComplete method to also return true for ChainSwapRefundFailed and
ChainSwapUserLockedFailed so these failure states are treated as terminal (this
will automatically make IsPending() return false for them); ensure this change
is made only in IsComplete and that CanRefund/other logic still behaves as
intended given these states.
In `@internal/infrastructure/db/sqlite/chainswap_repo.go`:
- Around line 154-158: The repository Close() method
(chainSwapRepository.Close()) incorrectly closes the shared *sql.DB used by
multiple repositories (settings, VHTLC, VtxoRollover, Swap, SubscribedScript,
ChainSwap); remove or change these per-repo Close implementations so they do not
call r.db.Close() and instead have the service-level Close() perform the single
db.Close(), or implement reference counting if independent lifecycles are
required; specifically, update chainSwapRepository.Close() (and the other repo
Close methods) to be no-ops or delegates and ensure only the service.Close()
invokes db.Close() once.
In `@internal/interface/grpc/handlers/service_handler.go`:
- Line 671: TimeoutBlockHeight is being hardcoded to 0 for BTC→ARK swaps;
instead read the real timeout from the swap details and set it on the response.
Locate where TimeoutBlockHeight is assigned in the BTC→ARK handling path (the
code that builds the response struct containing TimeoutBlockHeight) and replace
the literal 0 with the swap's timeout field (e.g., swap.TimeoutBlockHeight or
swapDetails.TimeoutBlockHeight), including a safe nil/zero check so you only set
a value when present. Ensure the response uses that extracted value so clients
receive the actual lockup expiry.
- Around line 645-650: The CreateChainSwapArkToBtc error path currently returns
a successful gRPC response with an Error string (CreateChainSwapArkToBtc
branch), which is inconsistent with the BTC_TO_ARK branch that returns a gRPC
status error; change the Ark->BTC branch to return a proper gRPC error (e.g.,
status.Error with appropriate codes.Internal and the err message) instead of
embedding the error in CreateChainSwapResponse so both directions use the same
status.Error pattern for failures.
In `@internal/test/e2e/chainswap_test.go`:
- Around line 30-42: The helper getBtcAddressBalance currently does an unsafe
type assertion raw["total_amount"].(float64) which can panic if the key is
missing or not a float64; update getBtcAddressBalance to safely read the value
using the comma-ok idiom or a type switch: check that raw["total_amount"]
exists, assert it to float64 (or handle numeric types like json.Number/int by
converting), and return a descriptive error if the key is missing or has an
unexpected type instead of letting the test panic.
In `@internal/test/e2e/main_test.go`:
- Around line 14-30: The commented-out TestMain disables test setup; restore
TestMain by uncommenting the function so that refillArkd, refillFulmineBoltz,
and refillFulmineClient run before tests (calling os.Exit(m.Run()) at the end),
or if you intentionally removed it delete the dead code; ensure the TestMain
signature (func TestMain(m *testing.M)) and required imports (context, os, log)
remain so failures call log.Fatalf with the error messages from refillArkd,
refillFulmineBoltz, and refillFulmineClient.
In `@pkg/boltz/types.go`:
- Around line 182-213: GetSwapTree currently dereferences ClaimDetails.SwapTree
and LockupDetails.SwapTree and can panic if those are nil; update
CreateChainSwapResponse.GetSwapTree to first check the relevant SwapTree
pointers (c.ClaimDetails.SwapTree and c.LockupDetails.SwapTree) and their
ClaimLeaf/RefundLeaf before accessing fields, and if any are nil return a
zero-value SwapTree (or a clearly empty SwapTree) instead of panicking; keep the
isArkToBtc branch and ensure you reference SwapTree, SwapTreeLeaf, ClaimDetails,
LockupDetails, ClaimLeaf and RefundLeaf when adding the checks so the guard
covers all dereferences.
In `@pkg/swap/btc_util.go`:
- Around line 30-79: The ConstructClaimTransaction function incorrectly computes
feeAmount using computeVSize(tx) which underestimates pre-signing (witness not
present) and then subtracts feeAmount from params.LockupAmount without guarding
against underflow; update the fee estimation to include an estimated witness
size (add typical witness bytes for the redeeming input) when calling
computeVSize or by adding a fixed witness overhead before multiplying by
feeRate, and add a defensive check after computing feeAmount to ensure feeAmount
< params.LockupAmount (return an error like "insufficient funds to cover network
fees" if not) before performing params.LockupAmount - feeAmount and assigning
tx.TxOut[0].Value; reference ConstructClaimTransaction, computeVSize,
explorerClient.GetFeeRate, feeAmount and tx.TxOut in your change.
In `@pkg/swap/chainswap_arkt_btc_handler.go`:
- Around line 62-66: The handler currently only logs and returns; update
arkToBtcHandler.HandleLockupFailed to transition the swap state to a
terminal/failed lockup state on h.chainSwapState (e.g., "lockup_failed"),
persist that state, and kick off the refund flow instead of returning nil—call
or add a helper like h.triggerRefund(ctx, update) or h.startRefundProcess(ctx,
update) to enqueue the refund operations and surface the error by returning a
non-nil error; keep the log but include the swap id from h.chainSwapState.SwapID
and any underlying error details when available.
In `@pkg/swap/chainswap_btc_ark_handler.go`:
- Around line 175-252: In signBoltzBtcClaim ensure the decoded transaction hash
is exactly 32 bytes before copying into msg: after txHashBytes, validate
len(txHashBytes)==32 and return a clear error (e.g., "invalid transaction hash
length") if not; only then copy into var msg [32]byte and proceed with signing
so our partial signature generation (OurPartialSign, AggregateNonces, etc.)
never uses a truncated/padded hash.
- Around line 64-66: The server-confirmed lock handler currently is a no-op;
change btcToArkHandler.HandleServerLocked to delegate to the existing mempool
lock handler so confirmed events trigger the same claim flow (e.g., call
b.HandleMempoolLocked(ctx, update) or otherwise invoke the same logic used for
mempool updates) ensuring Ark VTXOs are claimed when only the confirmed event
arrives.
In `@pkg/swap/chainswap_monitor.go`:
- Around line 32-37: Replace the direct status assignment on pubkey-parse
failure with the swap failure helper so failure hooks/events run: when
parsePubkey(swapResp.ClaimDetails.ServerPublicKey) returns an error, call
swap.Fail(...) (passing the error or a descriptive message including swapId)
instead of setting swap.Status = ChainSwapFailed, and then return; this keeps
transitions consistent and preserves error context for logging and downstream
handlers.
- Around line 95-155: Replace the direct status set and missing error details
when the WebSocket connect fails: instead of assigning ChainSwapFailed, call
chainSwapState.Swap.Fail(fmt.Sprintf("websocket subscribe failed: %v", err))
after ws.ConnectAndSubscribe fails (keep the same context and return). Also
change the monitor loop receive to check for channel closure by using the ok
pattern on ws.Updates (e.g., update, ok := <-ws.Updates) and handle closed
channel by logging/Failing the swap and returning; retain existing handler
switch and error handling after successful receives.
In `@pkg/swap/chainswap_validate.go`:
- Around line 20-83: The VHTLC mismatch error in validateVHTLC prints
swapResp.LockupDetails.LockupAddress regardless of isArkToBtc, which can be
wrong; update the fmt.Errorf call that constructs the "VHTLC address mismatch"
message to use the vhtlcAddr variable (the value actually compared) as the "Got"
value instead of swapResp.LockupDetails.LockupAddress so the error reports the
real compared address (refer to validateVHTLC, vhtlcAddr and vhtlcAddress).
In `@pkg/swap/explorer.go`:
- Around line 25-29: NewExplorerClient currently constructs an http.Client with
no timeout and GetFeeRate bypasses that client and decodes body before checking
status and assumes response["1"] exists; fix by setting a sensible timeout when
creating explorerClient in NewExplorerClient (e.g., time.Second * N), update
GetFeeRate to call e.client.Get(...) instead of http.Get, check resp.StatusCode
immediately and return an error on non-200 before decoding the body, and
validate the map key (e.g., check presence of "1" and handle missing/invalid
values) before converting to a fee rate.
- Around line 15-26: Add Go doc comments for the exported interface
ExplorerClient and the constructor NewExplorerClient: place a comment
immediately above the ExplorerClient declaration describing its purpose (e.g.,
"ExplorerClient provides methods to broadcast transactions and query fee rates
from a blockchain explorer") and a comment above NewExplorerClient describing
what it returns and the meaning of the baseURL parameter (e.g.,
"NewExplorerClient creates an ExplorerClient that talks to the explorer at
baseURL"). Ensure both comments are full sentences and start with the exported
name.
In `@test.docker-compose.yml`:
- Around line 74-77: The ARKD_VTXO_TREE_EXPIRY was reduced from 1024 to 512
which may let VTXOs expire before long-running claim/refund flows; restore or
increase ARKD_VTXO_TREE_EXPIRY so it is >= ARKD_UNILATERAL_EXIT_DELAY and also
validate it against FULMINE_SWAP_TIMEOUT (ensure ARKD_VTXO_TREE_EXPIRY covers
the maximum of ARKD_UNILATERAL_EXIT_DELAY and FULMINE_SWAP_TIMEOUT plus safety
margin); update the docker-compose env block where ARKD_VTXO_TREE_EXPIRY and
ARKD_UNILATERAL_EXIT_DELAY are defined and adjust FULMINE_SWAP_TIMEOUT if needed
to keep all three aligned.
🧹 Nitpick comments (17)
pkg/swap/batch_handler.go (1)
106-106: Clarify intentional no‑op handler.Consider a short comment to document why this hook does nothing (e.g., interface requirement), to prevent future “missing logic” confusion.
💬 Possible clarification
-func (h *batchSessionHandler) OnStreamStartedEvent(event client.StreamStartedEvent) {} +// OnStreamStartedEvent is a no-op; required by the stream handler interface. +func (h *batchSessionHandler) OnStreamStartedEvent(event client.StreamStartedEvent) {}pkg/boltz/ws.go (1)
233-233: Add a Go doc comment for exportedConnectAndSubscribe.This exported method lacks a doc comment.
✍️ Suggested addition
+// ConnectAndSubscribe connects to the Boltz websocket and subscribes to swap updates, retrying until ctx is done. func (boltz *Websocket) ConnectAndSubscribe(ctx context.Context, swapIds []string, retryInterval time.Duration) error {As per coding guidelines "Document exported functions, types, and complex logic with comments in Go code".
test.docker-compose.yml (1)
185-187: Pin the Boltz image to a specific stable release instead of:latest.Line 186 uses
:latest, which is a moving tag that makes tests non-reproducible and risks silent breakage when new releases are published. The current stable release isv3.12.1(Jan 9, 2026); pin this or another specific semver tag.Note: The
arktag is a development branch, not a stable release, and would not improve reproducibility.📌 Suggested pin
- image: boltz/boltz:latest + image: boltz/boltz:v3.12.1pkg/swap/musig.go (1)
241-301: Add GoDoc comments for exported helper functions.
ParsePubNonce,SerializePubNonce,ParsePartialSignatureScalar32,SerializePartialSignatureScalar32, andNewPrevOutputFetcherneed doc comments to satisfy GoDoc expectations.As per coding guidelines, Document exported functions, types, and complex logic with comments in Go code.
pkg/boltz/types.go (1)
144-287: Add GoDoc comments for newly exported chain-swap types.
The new public structs (e.g.,CreateChainSwapRequest,ChainSwapLockupDetails,SwapTree,SwapLeg,ChainSwapClaimDetailsResponse, etc.) should have GoDoc comments.As per coding guidelines, Document exported functions, types, and complex logic with comments in Go code.
pkg/swap/chainswap.go (1)
20-59: Add GoDoc comments for exported types.
ChainSwapStatusandChainSwapare exported but undocumented.As per coding guidelines, Document exported functions, types, and complex logic with comments in Go code.
internal/test/e2e/chainswap_test.go (2)
67-67: Consider polling with timeout instead of fixed sleep.Fixed
time.Sleepcalls can cause flaky tests. A polling loop with timeout would be more reliable:require.Eventually(t, func() bool { swaps, err := client.ListChainSwaps(ctx, &pb.ListChainSwapsRequest{SwapIds: []string{swapID}}) return err == nil && swaps.GetSwaps()[0].GetStatus() == "claimed" }, 30*time.Second, 1*time.Second, "swap should reach claimed status")This pattern applies to lines 67, 107, and 151.
123-165: Test is nearly identical toTestChainSwapBTCtoARK.The only difference from
TestChainSwapBTCtoARKis the faucet amount (0.00015500 vs 0.00003000). Consider consolidating into a single test with subtests or a table-driven approach to reduce duplication.♻️ Example consolidation
func TestChainSwapBTCtoARK(t *testing.T) { tests := []struct { name string faucetAmount float64 }{ {"standard", 0.00003000}, {"with_quote", 0.00015500}, } for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { // ... shared test logic with tc.faucetAmount }) } }internal/infrastructure/db/sqlite/chainswap_repo.go (2)
162-178: SimplifytoChainSwap— error return is unused.The function signature returns
(*domain.ChainSwap, error)but the error is alwaysnil. Either:
- Remove the error from the return type
- Add validation that could return an error (e.g., validating currency values)
Option 1: Remove unused error return
-func toChainSwap(row queries.ChainSwap) (*domain.ChainSwap, error) { +func toChainSwap(row queries.ChainSwap) *domain.ChainSwap { return &domain.ChainSwap{ Id: row.ID, From: boltz.Currency(row.FromCurrency), To: boltz.Currency(row.ToCurrency), Amount: uint64(row.Amount), Status: domain.ChainSwapStatus(row.Status), UserLockupTxId: fromNullableString(row.UserLockupTxID), ServerLockupTxId: fromNullableString(row.ServerLockupTxID), ClaimTxId: fromNullableString(row.ClaimTxID), ClaimPreimage: row.ClaimPreimage, RefundTxId: fromNullableString(row.RefundTxID), UserBtcLockupAddress: fromNullableString(row.UserBtcLockupAddress), ErrorMessage: fromNullableString(row.ErrorMessage), CreatedAt: fromNullableInt64(row.CreatedAt), - }, nil + } }
72-83: Silent error skipping may hide data corruption.When
toChainSwapfails, the code logs a warning and skips the row. While this provides resilience, it could mask underlying data issues. The same pattern appears inGetByIDsandGetByStatus.Consider returning an error or at least including a count of skipped rows in the response to alert callers.
api-spec/openapi/swagger/fulmine/v1/service.swagger.json (2)
65-96: Consider addingconsumesfield for POST operation.Static analysis (Checkov CKV_OPENAPI_17) flags that the POST operation is missing a
consumesfield. While the globalconsumes: ["application/json"]at the document level (lines 12-14) typically applies, explicitly adding it to this operation improves clarity.
155-165: Consider addingmaxItemsto theswapIdsarray parameter.Static analysis (Checkov CKV_OPENAPI_21) recommends defining a maximum number of items for array parameters to prevent unbounded queries that could impact performance.
Suggested fix
{ "name": "swapIds", "in": "query", "required": false, "type": "array", "items": { "type": "string" }, - "collectionFormat": "multi" + "collectionFormat": "multi", + "maxItems": 100 }internal/core/domain/chainswap.go (1)
67-107: State transition methods don't validate preconditions.The transition methods (e.g.,
UserLocked,Claimed) unconditionally update the state without checking if the transition is valid from the current state. For example,Claimed()could be called even when the swap is alreadyChainSwapRefunded.While this may be acceptable if callers are trusted to call methods in the correct order, adding validation would make the domain model more robust.
pkg/swap/chainswap_btc_ark_handler.go (1)
27-41: Add a Go doc comment forNewBtcToArkHandler.✏️ Suggested doc comment
-func NewBtcToArkHandler( +// NewBtcToArkHandler builds a ChainSwapEventHandler for BTC→ARK swaps. +func NewBtcToArkHandler(As per coding guidelines: Document exported functions, types, and complex logic with comments in Go code.
pkg/swap/chainswap_arkt_btc_handler.go (1)
33-56: Add a Go doc comment forNewArkToBtcHandler.✏️ Suggested doc comment
-func NewArkToBtcHandler( +// NewArkToBtcHandler builds a ChainSwapEventHandler for ARK→BTC swaps. +func NewArkToBtcHandler(As per coding guidelines: Document exported functions, types, and complex logic with comments in Go code.
api-spec/protobuf/fulmine/v1/service.proto (1)
173-192: Consider a server‑streaming RPC for real‑time swap updates.Chain swaps are monitored live; a streaming gRPC endpoint would give clients a native, real‑time option without polling.
💡 Possible shape
service Service { + // WatchChainSwap streams status updates for a swap. + rpc WatchChainSwap(WatchChainSwapRequest) returns (stream ChainSwapResponse); } + +message WatchChainSwapRequest { + string id = 1; +}As per coding guidelines: Use streaming in gRPC services where appropriate for real-time data.
pkg/swap/chainswap_monitor.go (1)
202-208: Add a Go doc comment forChainSwapState.✏️ Suggested doc comment
-type ChainSwapState struct { +// ChainSwapState holds swap metadata and callbacks used by the monitor loop. +type ChainSwapState struct {As per coding guidelines: Document exported functions, types, and complex logic with comments in Go code.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@pkg/swap/chainswap_arkt_btc_handler.go`:
- Around line 33-56: Add a Go doc comment above the exported constructor
NewArkToBtcHandler describing what the constructor returns and its purpose
(e.g., constructs and initializes an arkToBtcHandler for handling ARK→BTC
swaps), and briefly mention key parameters like swapHandler, state, network, and
swapResp to clarify intent; ensure the comment starts with "NewArkToBtcHandler
..." to follow Go documentation conventions.
altafan
left a comment
altafan
left a comment
There was a problem hiding this comment.
please revert all changes to pkg/vhtlc as they are not necessary
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
pkg/swap/swap.go (2)
71-95:⚠️ Potential issue | 🟠 MajorAdd nil check for
privateKeyto prevent nil pointer dereference.If
privateKeyis nil, line 91 will panic when callingprivateKey.PubKey(). Add validation at the start of the function.🛡️ Proposed fix
func NewSwapHandler( arkClient arksdk.ArkClient, transportClient client.TransportClient, indexerClient indexer.Indexer, boltzSvc *boltz.Api, esploraURL string, privateKey *btcec.PrivateKey, timeout uint32, ) (*SwapHandler, error) { + if privateKey == nil { + return nil, fmt.Errorf("privateKey cannot be nil") + } cfg, err := arkClient.GetConfigData(context.Background())
1092-1114:⚠️ Potential issue | 🟠 MajorCheck
refundResp.Errorbefore using response fields.Per the
RefundSwapResponsetype, the response includes anErrorfield. If Boltz returns an error,TransactionandCheckpointmay be empty or invalid, leading to confusing parse errors.🐛 Proposed fix
refundResp, err := requestRefund(swapId, boltz.RefundSwapRequest{ Transaction: refundTx, Checkpoint: checkpointTx, }) if err != nil { return nil, nil, err } + if refundResp.Error != "" { + return nil, nil, fmt.Errorf("boltz refund failed: %s", refundResp.Error) + } refundPtx, err := psbt.NewFromRawBytes(strings.NewReader(refundResp.Transaction), true)
🤖 Fix all issues with AI agents
In `@pkg/swap/chainswap_validate.go`:
- Around line 20-83: In validateVHTLC, add a hard length check for
preimageHashHASH160 (expecting 20 bytes for HASH160) before using it or passing
to h.getVHTLC; if len(preimageHashHASH160) != 20 return an explanatory error
(e.g. "invalid preimage hash length") so malformed inputs fail fast and avoid
misleading address derivation or panics. Ensure the check is placed at the start
of validateVHTLC (before the call to h.getVHTLC and before slicing
preimageHashHASH160[:]) and reference the same variable name and function
getVHTLC in the error path.
- Around line 508-549: In validateBtcLockupAddress, add a defensive nil check
for the clientPubKey before calling musig2.AggregateKeys: if clientPubKey == nil
return a descriptive error (e.g., "client pubkey is nil") so the function fails
fast instead of allowing musig2.AggregateKeys to panic; follow the same error
style used in NewMuSigContext (wrap/return fmt.Errorf) and keep the check placed
before the AggregateKeys call that uses clientPubKey and serverPubKey.
In `@pkg/swap/swap.go`:
- Around line 447-457: The switch that sets refundFunc for SwapTypeSubmarine and
SwapTypeChain can leave refundFunc nil for unknown swapType, causing a panic
when calling h.collaborativeRefund; update the switch in the handler to handle
other values (e.g., add a default case that returns a clear error) or check for
nil refundFunc before calling collaborativeRefund and return an error; reference
the refundFunc variable, the switch on swapType, the constants
SwapTypeSubmarine/SwapTypeChain, and the call to h.collaborativeRefund to locate
and fix the code.
🧹 Nitpick comments (3)
pkg/swap/swap.go (1)
1087-1090: Consider moving swap type constants near other constants at the top of the file.For consistency with
SwapStatusconstants (lines 52-56), consider relocatingSwapTypeSubmarineandSwapTypeChainto the same area.internal/core/application/service.go (2)
535-541: Track the crash-recovery TODO for chain swaps.This TODO leaves pending swaps in limbo after restart. Consider creating an issue or implementing recovery now.
If you want, I can sketch a recovery flow aligned with the current chain-swap state model.
1617-1627: Add a doc comment for the exported ListChainSwaps method.Public APIs should have GoDoc comments for clarity.
🛠️ Suggested comment
+// ListChainSwaps returns all chain swaps, optionally filtered by IDs. func (s *Service) ListChainSwaps(ctx context.Context, swapIDs []string) ([]domain.ChainSwap, error) {As per coding guidelines, Document exported functions, types, and complex logic with comments in Go code.
There was a problem hiding this comment.
Actionable comments posted: 13
🤖 Fix all issues with AI agents
In `@internal/core/application/service.go`:
- Around line 1749-1757: The parsePubkey function currently returns (nil, nil)
for empty input which allows silent nil propagation; modify parsePubkey to
return a descriptive error (e.g., fmt.Errorf("empty pubkey")) when pubkey == ""
instead of nil, nil, and keep the existing hex.DecodeString error handling; then
update callers (e.g., in service_handler.go, swap.go, service.go) to handle the
returned error path rather than assuming a nil PublicKey is acceptable so they
surface a clear "empty pubkey" error upstream.
- Around line 1458-1486: The swap is persisted only after calling
swapHandler.ChainSwapArkToBtc which starts monitoring goroutines that may invoke
handleChainSwapEvent before the DB record exists; to fix, ensure persistence
happens before any background monitors start: either move the call to
s.dbSvc.ChainSwaps().Add(...) into the swap handler implementations
(ChainSwapArkToBtc and ChainSwapBtcToArk) so they persist the domain.ChainSwap
before launching goroutines, or modify the service layer to persist the swap
immediately after receiving chainSwap and before passing/attaching
eventCallback/unilateralRefund or before any goroutine startup; update
references: handleChainSwapEvent, swapHandler.ChainSwapArkToBtc,
swapHandler.ChainSwapBtcToArk, and dbSvc.ChainSwaps().Add.
In `@internal/infrastructure/db/sqlite/sqlc/schema.sql`:
- Around line 55-70: The status CHECK on the chain_swap table currently
restricts status to 0–7, but the domain includes value 8
(ChainSwapRefundedUnilaterally); update the CHECK constraint on the status
column in the chain_swap CREATE TABLE (status INTEGER NOT NULL CHECK(...)) to
allow value 8 (i.e., include 8 in the IN list) so the
ChainSwapRefundedUnilaterally enum can be persisted.
In `@internal/test/mockboltz/server.go`:
- Around line 675-685: The code mutates client.subs while only holding s.wsMu,
which protects s.wsClients but not the nested map; fix by acquiring the client's
own mutex (client.mu) before modifying client.subs in the subscribe branch
(where msg.Op == "subscribe" && msg.Channel == "swap.update") and release it
after updates, and ensure all reads of client.subs (e.g., the access at the
earlier read site around where client.subs is iterated or checked) also take
client.mu; keep using s.wsMu only to safely access s.wsClients but always lock
client.mu around accesses to the subs map.
In `@MockBoltz.Dockerfile`:
- Around line 1-25: The dockerfile name casing mismatches the docker-compose
reference: the repo has MockBoltz.Dockerfile but test.docker-compose.yml
references mockboltz.Dockerfile; on case-sensitive systems the build will fail.
Fix by renaming MockBoltz.Dockerfile to mockboltz.Dockerfile (or alternatively
update the reference in test.docker-compose.yml to match exactly
"MockBoltz.Dockerfile"), ensuring the filename used by docker-compose and the
actual file name are byte-for-byte identical.
- Line 1: The docker-compose reference and Dockerfile name mismatch: update
either test.docker-compose.yml to reference "MockBoltz.Dockerfile" (matching the
current file) or rename "MockBoltz.Dockerfile" to "mockboltz.Dockerfile" so the
case matches on case-sensitive CI; also verify and fix the base image in the
Dockerfile by replacing FROM golang:1.25.5-alpine (which may not exist) with a
valid Go image (e.g., a supported 1.25.x or 1.24.x tag) that meets the project
requirement (Go >=1.24.6).
In `@pkg/boltz/ws.go`:
- Around line 240-242: In ConnectAndSubscribe, guard against zero or negative
retryInterval before calling Retry by clamping it to a sane default (e.g.,
time.Second or a configured constant) so Retry's time.After isn't driven with
<=0; update the ConnectAndSubscribe method to check retryInterval (and if <=0
set retryInterval = defaultRetryInterval) and then pass the clamped value into
Retry to prevent tight CPU loops.
In `@pkg/swap/btc_util.go`:
- Around line 209-233: The control block is using the hardcoded BaseLeafVersion
and the TapLeaf created via NewBaseTapLeaf, which mismatches the merkle root
that used actual leaf versions; update the control block construction to use the
real leaf version from swapTree (swapTree.ClaimLeaf.Version or
swapTree.RefundLeaf.Version depending on isClaimPath) when building
leafVersionByte (leafVersionByte := byte(actualVersion) | parity) and compute
siblingHash with the same version via tapLeafHash(actualVersion, siblingScript)
instead of siblingLeaf.TapHash(); keep using internalKeyBytes and parity as
before so the control block and merkle root use the same leaf versions.
In `@pkg/swap/chainswap_monitor.go`:
- Around line 134-136: The constant boltz.TransactionServerMempoool is
misspelled (extra 'o'); rename the constant to boltz.TransactionServerMempool in
its definition (pkg/boltz/status.go) and update all references (e.g., switch
cases like the one calling handler.HandleServerLockedMempool, tests, imports,
and any string mappings) to use the correct identifier TransactionServerMempool
so it still maps to "transaction.server.mempool"; ensure you run tests/compile
to catch any remaining references.
In `@pkg/swap/chainswap.go`:
- Around line 573-595: The RefundArkToBTCSwap function currently calls
RefundSwap with context.Background(), ignoring the provided ctx; update the call
to pass the incoming ctx so cancellation/timeouts propagate (i.e., change the
RefundSwap call in RefundArkToBTCSwap to use ctx instead of
context.Background()), leaving the rest of the parameters (SwapTypeChain,
swapId, true, vhtlcOpts) and the unilateralRefundCallback handling unchanged.
- Around line 159-169: The RefundUnilaterally method currently sets s.Status =
ChainSwapRefundedUnilaterally but emits a generic RefundEvent; update the
emission to emit the unilateral-specific event type (RefundEventUnilaterally) so
listeners can distinguish it. In the RefundUnilaterally method of ChainSwap,
replace the onEvent(RefundEvent{...}) call with
onEvent(RefundEventUnilaterally{SwapID: s.Id, TxID: txid}) (preserving the
SwapID and TxID fields) and keep the existing status and RefundTxid assignment.
In `@pkg/swap/explorer.go`:
- Around line 111-206: The three methods GetCurrentBlockHeight,
GetTransactionStatus, and GetTransaction are calling http.Get directly (lines
shown in the diff) which bypasses the configured e.client (with its timeout and
transport); replace those http.Get(...) calls with e.client.Get(...) and keep
the existing error handling and resp.Body.Close defer logic (ensure you only
defer after err==nil and resp non-nil) so all explorer requests use the shared
HTTP client created in NewExplorerClient.
In `@scripts/setup`:
- Around line 355-366: The addr variable assigned from the onboard POST
(addr=$(curl -s -X POST ... | jq -r .address)) is unused and the script claims
funding even though no funding command is run; either use addr to perform the
funding step (call nigiri faucet $addr 0.001 after obtaining addr and before the
settle check) and keep the existing success echo, or remove addr and change the
success message to not claim funding; also ensure the error handling around the
curl calls uses the curl exit status and/or response fields consistently
(replace the current bogus status/err references) and apply the same fix to the
Client Fulmine flow which mirrors this block.
🧹 Nitpick comments (13)
test.docker-compose.yml (1)
236-236: Pin to a specific image version tag instead oflatestfor reproducible test behavior.The
boltz/boltz:latesttag is mutable and can cause inconsistent test results as upstream updates are released. Multiple stable versions are available (e.g.,v3.12.1,v3.12.0,v3.11.0); pin the image to one of these to ensure reproducible builds.pkg/swap/btc_util.go (1)
19-25: Add Go doc for exportedClaimTransactionParams.
Exported types should have a doc comment for Go tooling and readability.✍️ Suggested fix
-type ClaimTransactionParams struct { +// ClaimTransactionParams holds inputs for building a BTC claim transaction. +type ClaimTransactionParams struct { LockupTxid string LockupVout uint32 LockupAmount uint64 DestinationAddr string Network *chaincfg.Params }As per coding guidelines, Document exported functions, types, and complex logic with comments in Go code.
pkg/boltz/go.mod (1)
8-11: Confirm logrus/x/sys pins are intentional and consistent.Line 11 pins golang.org/x/sys v0.13.0, which is far behind the root module’s v0.37.0; this kind of skew can cause confusing resolutions and miss security fixes when this submodule is replaced into the root build. Please verify the older pin is required; otherwise align it with the root version.
go.mod (1)
13-40: Verify dependency bumps and theltcdreplace are compatible.The new replace directive plus multiple version bumps (logrus, btcec/v2, blake3, websocket) can impact transitive resolution. Please confirm these versions are intentional and compatible across the repo (especially when
pkg/*modules are replaced into the root build).Also applies to: 73-74, 183-183, 200-200
pkg/swap/go.mod (1)
7-14: Confirm submodule dependency versions align with root go.mod.These bumps (logrus, vhtlc, go-sdk, btcec/v2) should match the root module’s versions to avoid resolution skew when using
replace. Please verify this alignment and compatibility.Also applies to: 68-68
pkg/swap/chainswap.go (2)
27-210: Add doc comments for exported ChainSwap API surface.
ChainSwapStatus,ChainSwap,NewChainSwap, the exported state mutators (UserLock/ServerLock/Claim/Refund/RefundUnilaterally/Fail/RefundFailed/UserLockedFailed), andRefundArkToBTCSwapare exported but lack doc comments.As per coding guidelines, Document exported functions, types, and complex logic with comments in Go code.
Also applies to: 573-595
796-817: Allow ctx cancellation during confirmation polling.The refund confirmation loop can block for up to 2 hours even if
ctxis canceled. Consider checkingctx.Done()in the loop to allow prompt cancellation.internal/core/application/service.go (2)
61-75: Avoid silently defaulting unknown networks to regtest.Falling back to regtest can lead to signing/broadcasting on the wrong network if a new or misconfigured network name appears. Prefer returning an error and stopping swap creation.
🛠️ Safer mapping with explicit error
-// networkNameToParams converts arklib network name to chaincfg.Params -func networkNameToParams(networkName string) *chaincfg.Params { +// networkNameToParams converts arklib network name to chaincfg.Params. +// Returns an error for unknown networks to avoid silent misconfiguration. +func networkNameToParams(networkName string) (*chaincfg.Params, error) { switch networkName { case arklib.Bitcoin.Name: - return &chaincfg.MainNetParams + return &chaincfg.MainNetParams, nil case arklib.BitcoinTestNet.Name: - return &chaincfg.TestNet3Params + return &chaincfg.TestNet3Params, nil case arklib.BitcoinRegTest.Name: - return &chaincfg.RegressionNetParams + return &chaincfg.RegressionNetParams, nil case arklib.BitcoinSigNet.Name, arklib.BitcoinMutinyNet.Name: - return &chaincfg.SigNetParams + return &chaincfg.SigNetParams, nil default: - // Default to regtest for safety - return &chaincfg.RegressionNetParams + return nil, fmt.Errorf("unsupported network: %s", networkName) } }- network := networkNameToParams(config.Network.Name) + network, err := networkNameToParams(config.Network.Name) + if err != nil { + return nil, err + }- network := networkNameToParams(config.Network.Name) + network, err := networkNameToParams(config.Network.Name) + if err != nil { + return nil, err + }Also applies to: 1456-1507
1634-1647: Add Go doc comments for exported chain-swap APIs.
ListChainSwapsandRefundChainSwapare exported but undocumented.As per coding guidelines, Document exported functions, types, and complex logic with comments in Go code.✏️ Suggested doc comments
+// ListChainSwaps returns chain swaps, optionally filtered by IDs. func (s *Service) ListChainSwaps(ctx context.Context, swapIDs []string) ([]domain.ChainSwap, error) { if err := s.isInitializedAndUnlocked(ctx); err != nil { return nil, err } @@ return s.dbSvc.ChainSwaps().GetByIDs(ctx, swapIDs) } +// RefundChainSwap triggers a refund flow for the specified chain swap. func (s *Service) RefundChainSwap(ctx context.Context, id string) error { if err := s.isInitializedAndUnlocked(ctx); err != nil { return err }pkg/swap/chainswap_btc_ark_handler.go (1)
27-33: Add a Go doc comment for the exported constructor.As per coding guidelines, Document exported functions, types, and complex logic with comments in Go code.✏️ Suggested comment
+// NewBtcToArkHandler constructs a ChainSwapEventHandler for BTC→ARK swaps. func NewBtcToArkHandler( swapHandler *SwapHandler, chainSwapState ChainSwapState, preimage []byte,internal/test/mockboltz/server.go (1)
48-267: Add Go doc comments for exported types and methods.
Config,Server,New,Start,Stop, andStringare exported but undocumented.As per coding guidelines, Document exported functions, types, and complex logic with comments in Go code.✏️ Suggested doc comments
+// Config defines runtime settings for the mock Boltz server. type Config struct { ListenAddr string ArkdURL string @@ } +// Server is an in-process mock Boltz API server used in tests. type Server struct { cfg Config @@ } +// New constructs a new mock Boltz server with the provided configuration. func New(cfg Config) (*Server, error) { @@ } +// Start launches the HTTP and WebSocket endpoints. func (s *Server) Start() error { @@ } +// Stop gracefully shuts down the mock server. func (s *Server) Stop() error { @@ } @@ +// String returns the base URL for the running mock server. func (s *Server) String() string {Also applies to: 1384-1400
internal/test/e2e/chainswap_test.go (2)
444-467: Well-structured polling helper with clear timeout handling.The
waitChainSwapStatushelper properly implements polling with a deadline and provides a clear final assertion with the actual status for debugging. Consider adding a small log statement when the expected status is reached to aid debugging in CI.💡 Optional: Add success logging
if err == nil && len(resp.GetSwaps()) > 0 { if resp.GetSwaps()[0].GetStatus() == expected { + t.Logf("Swap %s reached expected status: %s", swapID, expected) return } }
536-548: HTTP request without context.The
http.Getcall uses//nolint:noctxbut passing context is preferred for consistency with the rest of the codebase and proper cancellation support.💡 Optional: Use context-aware HTTP request
+func waitForEsploraTxIndexed(t *testing.T, ctx context.Context, txID string, timeout time.Duration) { -func waitForEsploraTxIndexed(t *testing.T, txID string, timeout time.Duration) { t.Helper() require.NotEmpty(t, txID) ... for time.Now().Before(deadline) { - resp, err := http.Get(endpoint) //nolint:noctx + req, err := http.NewRequestWithContext(ctx, http.MethodGet, endpoint, nil) + require.NoError(t, err) + resp, err := http.DefaultClient.Do(req)
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@internal/core/application/service.go`:
- Around line 1688-1711: GetConfigData's error is being ignored which can leave
cfg nil and cause a panic when accessing cfg.SignerPubKey; change the call in
the function to capture and check the error from s.GetConfigData(ctx) (e.g. cfg,
err := s.GetConfigData(ctx)) and if err != nil return a descriptive error (for
example "failed to get config data: %w"); ensure subsequent use of cfg (used to
populate vhtlc.Opts.Server) only happens after this error check.
In `@internal/interface/grpc/handlers/service_handler.go`:
- Around line 750-753: The switch handling in service_handler.go maps
domain.ChainSwapRefunded and domain.ChainSwapRefundedUnilaterally to the same
string ("refunded"), preventing clients from distinguishing cooperative vs
unilateral refunds; update the case for domain.ChainSwapRefundedUnilaterally in
the function that returns the status string (the switch handling these domain
constants) to return a distinct value such as "refunded_unilaterally" while
leaving domain.ChainSwapRefunded returning "refunded", and update any related
documentation/tests that expect the status string.
- Around line 652-658: The response is sending the raw hex-encoded preimage into
the proto field named PreimageHash; fix by computing the HTLC preimage hash
(SHA-256) and send that instead: in the code that builds the
CreateChainSwapResponse (the block that constructs
&pb.CreateChainSwapResponse{... PreimageHash: ...}) decode
chainSwap.ClaimPreimage from hex, compute sha256.Sum256(decodedPreimage),
hex-encode the hash and assign that value to PreimageHash; ensure you import
crypto/sha256 and encoding/hex and handle decoding errors where
chainSwap.ClaimPreimage is used (matching the places using
hex.EncodeToString/hex.DecodeString for chainSwap.Preimage).
In `@internal/test/e2e/main_test.go`:
- Around line 30-40: The log messages for the refillFulmine calls are swapped
and one call has a formatting issue: when calling refillFulmine(ctx,
clientFulmineURL) update the log.Fatalf message to say "failed to refill Fulmine
used by Client" (using clientFulmineURL) and when calling refillFulmine(ctx,
boltzFulmineURL) update its log.Fatalf to say "failed to refill Fulmine used by
Boltz" (using boltzFulmineURL); also fix the missing space after the comma in
the second call's argument list. Ensure you update the three sites referencing
refillFulmine, clientFulmineURL, boltzFulmineURL, mockFulmineURL and log.Fatalf
accordingly.
In `@pkg/swap/chainswap_btc_ark_handler.go`:
- Around line 59-61: Fix the two comment typos: in the comment inside the
btcToArkHandler.HandleServerLockedMempool method change "out BTC lockup" to "our
BTC lockup", and elsewhere where the comment reads "locup" change it to "lockup"
(search for occurrences near handleBtcToArkServerLocked and other BTC
lockup-related comments). Keep the comment text otherwise unchanged.
🧹 Nitpick comments (10)
internal/test/e2e/main_test.go (1)
17-21: Inconsistent alignment in const block.
mockFulmineURLhas extra whitespace for alignment butclientFulmineURLandboltzFulmineURLdo not use the same padding. This is a nitpick — consider aligning all three consistently or using no alignment.internal/infrastructure/db/sqlite/migration/20260120000000_add_chain_swaps_table.up.sql (1)
6-6: Hardcoded status range in CHECK constraint couples schema to application enum.
CHECK(status IN(0,1,2,3,4,5,6,7,8))must be kept in sync with the GoChainSwapStatusiota. Adding a new status requires a new migration to alter this constraint. This is acceptable for SQLite migrations but worth noting — consider a comment in the migration file documenting which status each integer maps to.test.docker-compose.yml (1)
234-234: Usingboltz/boltz:latesttag is non-deterministic for CI.Changing from a pinned tag (
boltz/boltz:ark) tolatestmeans test results can vary depending on when the image was pulled. Consider pinning to a specific version or digest for reproducible builds.pkg/swap/chainswap_btc_ark_handler.go (2)
126-126: Hardcodedtime.Sleep(5 * time.Second)in the claim path.This blocks the goroutine for 5 seconds between claiming VTXOs and providing the cooperative signature. If the intent is to wait for the claim to propagate, consider polling for confirmation or using an event-driven mechanism instead. A hardcoded sleep is fragile — too short on slow networks, unnecessarily slow on fast ones.
27-41: Missing doc comment on exported constructorNewBtcToArkHandler.Per Go conventions, exported functions should have a doc comment. As per coding guidelines, exported functions should be documented with comments.
internal/test/e2e/utils_test.go (2)
38-45: Duplicated gRPC connection logic; connections are never closed.
newFulmineWalletClientis nearly identical tonewFulmineClient(lines 29–36). Consider extracting a shareddialFulmine(url) (*grpc.ClientConn, error)helper that both can use, which also makes it easier to defer-close the connection at call sites.Sketch
func dialFulmine(url string) (*grpc.ClientConn, error) { return grpc.NewClient(url, grpc.WithTransportCredentials(insecure.NewCredentials())) } func newFulmineClient(url string) (pb.ServiceClient, error) { conn, err := dialFulmine(url) if err != nil { return nil, err } return pb.NewServiceClient(conn), nil } func newFulmineWalletClient(url string) (pb.WalletServiceClient, error) { conn, err := dialFulmine(url) if err != nil { return nil, err } return pb.NewWalletServiceClient(conn), nil }
178-197:unlockAndSettleopens two separate gRPC connections to the same address.Both
newFulmineWalletClient(addr)andnewFulmineClient(addr)dial the same URL. A singlegrpc.ClientConncan serve bothWalletServiceClientandServiceClient.Sketch
func unlockAndSettle(addr string, pass string) error { - walletClient, err := newFulmineWalletClient(addr) + conn, err := dialFulmine(addr) if err != nil { - return fmt.Errorf("wallet client connect %s: %w", addr, err) + return fmt.Errorf("connect %s: %w", addr, err) } + defer conn.Close() + walletClient := pb.NewWalletServiceClient(conn) if _, err = walletClient.Unlock(context.Background(), &pb.UnlockRequest{Password: pass}); err != nil { return err } - serviceClient, err := newFulmineClient(addr) - if err != nil { - return err - } - + serviceClient := pb.NewServiceClient(conn) if _, err = serviceClient.Settle(context.Background(), &pb.SettleRequest{}); err != nil { return err } return nil }pkg/swap/chainswap_resume.go (1)
82-100: Inline closures for address/key selection add unnecessary complexity.The two
func() string { ... }()IIFEs on lines 84–89 and 90–95 could be replaced with a simpleif/elseblock before the call, making the code easier to read.Sketch
+ var lockupAddr, serverPubKey string + if arkToBtc { + lockupAddr = swapResp.ClaimDetails.LockupAddress + serverPubKey = swapResp.ClaimDetails.ServerPublicKey + } else { + lockupAddr = swapResp.LockupDetails.LockupAddress + serverPubKey = swapResp.LockupDetails.ServerPublicKey + } + if err := validateBtcLockupAddress( params.Network, - func() string { - if arkToBtc { - return swapResp.ClaimDetails.LockupAddress - } - return swapResp.LockupDetails.LockupAddress - }(), - func() string { - if arkToBtc { - return swapResp.ClaimDetails.ServerPublicKey - } - return swapResp.LockupDetails.ServerPublicKey - }(), + lockupAddr, + serverPubKey, h.privateKey.PubKey(), swapResp.GetSwapTree(arkToBtc), ); err != nil {internal/core/application/service.go (1)
1536-1632:handleChainSwapEventcontains heavy boilerplate — consider a small helper.All eight cases follow the identical get→apply→update pattern. A helper like the one below would cut ~70 lines and centralise the error-handling logic.
Suggested helper
// applyChainSwapMutation fetches, mutates, and persists a chain swap in one step. func (s *Service) applyChainSwapMutation(ctx context.Context, swapID, eventName string, mutate func(*domain.ChainSwap)) { cs, err := s.dbSvc.ChainSwaps().Get(ctx, swapID) if err != nil { log.WithError(err).Errorf("Failed to get chain swap %s for %s", swapID, eventName) return } mutate(cs) if err := s.dbSvc.ChainSwaps().Update(ctx, *cs); err != nil { log.WithError(err).Errorf("Failed to update chain swap %s after %s", swapID, eventName) } }Each case then becomes:
case swap.UserLockEvent: s.applyChainSwapMutation(ctx, e.SwapID, "UserLockEvent", func(cs *domain.ChainSwap) { cs.UserLocked(e.TxID) })internal/core/domain/chainswap.go (1)
77-123: Consider adding lightweight transition guards on mutation methods.The mutation methods accept any current
Status, so nothing prevents illogical transitions (e.g., callingClaimed()afterRefunded()). In practice, the handler enforces sequencing, but a simple guard in the domain layer would make invariants self-documenting and catch integration bugs early.Example guard (one method shown)
func (cs *ChainSwap) Claimed(txid string) { + if IsTerminalChainSwapStatus(cs.Status) { + return // already in terminal state, ignore + } cs.ClaimTxId = txid cs.Status = ChainSwapClaimed }
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@internal/test/e2e/chainswap_test.go`:
- Around line 531-533: The call to unlockAndSettle is ignoring its returned
error—replace the incorrect assertion (require.NotEmpty(t, swapID)) after
restartDockerComposeServices with an explicit check of the error from
unlockAndSettle: assert no error (e.g., require.NoError or require.Nil) on the
variable err returned by unlockAndSettle(mockFulmineURL, fulminePass); keep or
remove the swapID presence check as appropriate since swapID was already
asserted earlier. Ensure you reference the unlockAndSettle call and its err
variable so failures during recovery fail the test.
🧹 Nitpick comments (4)
internal/test/e2e/chainswap_test.go (4)
25-59: No assertion on the balance difference after swap.The test logs the BTC balance before and after but only asserts
> 0. Consider also asserting the final swap status more defensively (e.g., check the full list length) or verifying the balance delta is within an expected range to catch fee regressions.
96-98: Balance diff is computed but never asserted.
diffis logged but not checked. If the intent is to verify the user received funds, add an assertion (e.g.,require.Greater(t, diff, int64(0))). Same applies toTestChainSwapBTCtoARKWithQuoteat Line 136.
542-565: Polling loop does not respect context cancellation.If the parent context is cancelled before
deadline, the loop continues polling with a dead context. The gRPC calls will fail, but the final assertion at Line 564 will report a misleading gRPC error instead of a clear timeout message. Consider adding actx.Done()check:select { case <-ctx.Done(): require.Failf(t, "context cancelled", "while waiting for swap %s to reach %q", swapID, expected) default: }Same pattern applies to
refundChainSwapRPCWithRetry(Line 579) andwaitForEsploraTxIndexed(Line 620).
736-739: Float-to-int truncation may cause off-by-one in sat conversion.
int(float64 * 100_000_000)truncates toward zero. Due to IEEE 754, values like0.00003 * 1e8could become2999instead of3000. Usemath.Roundfor safety:- return int(nigiriScanAddressBalanceBTC(t, ctx, addr) * 100_000_000) + return int(math.Round(nigiriScanAddressBalanceBTC(t, ctx, addr) * 100_000_000))
3 participants
Overview
RPCs Added
Service RPCs
CreateChainSwap— start Ark→BTC or BTC→Ark swapListChainSwaps— list chain swap recordsRefundChainSwap— initiate refundDomain & Persistence
chainswap.go:pending,user_locked,server_locked,claimed,failed,refund_failed,refunded,refunded_unilaterally.IsTerminalChainSwapStatus,ShouldRefundChainSwapStatus,ShouldResumeChainSwapStatus.chain_swaptable + SQLC repo:8(refunded_unilaterally).Core App Logic
Create flow
CreateChainSwapArkToBtcandCreateBtcToArkChainSwapinservice.go.Refund flow
RefundChainSwapsupports:Recovery on restart
failed/refund_failed/user_locked_failed.Decision Points
Chain Swap (
pkg/swap)btc_util.go,explorer.gofor tx building, fee calc, and esplora access.chainswap_resume.go: rehydrate swap from DB and re‑start monitoring.Boltz Integration (
pkg/boltz)Mock Boltz
server.go):mockboltz.Dockerfile.Tests
New e2e chain swap suite in
chainswap_test.go:Tests added:
ark to btc happy path with cooperative claim
ark to btc happy path with unilateral claim via script path spend
ark to btc swap fails with cooperative refund
ark to btc with unilaterl refund which is scheduled after some timeout depending on transaction locktime
ark to btc with wrong amount locked up by user which triggers quote negotiation flow
btc to ark flow happy path
btc to ark unilateral refund
Test utilities:
Infra / Docker / Setup
test.docker-compose.yml:scripts/setupexpanded to provision mock‑boltz + fulmine‑mock.arkd.Dockerfile/arkdwallet.Dockerfileupdated to Go 1.25.6 (aligns with arkd go.mod).Misc
.gitignorenow ignores.worktrees(local dev).@altafan @louisinger @bordalix please review.
Summary by CodeRabbit