Conversation
WalkthroughAdds Boltz chain-swap support (ARK ↔ BTC): new chain-swap types and client APIs, persistence (migration + storage trait/impls), CLI commands and justfile targets, and updated swap-status handling for chain-swap-specific states. Changes
Sequence DiagramssequenceDiagram
participant User
participant Client
participant BoltzAPI as Boltz API
participant ArkNet as ARK Network
participant BTCNet as BTC Network
participant Storage
User->>Client: create_chain_swap(ArkToBtc, amount)
Client->>BoltzAPI: POST /v2/swap/chain (from: ARK,to:BTC)
BoltzAPI-->>Client: ChainSwapResult (swap_id, lockupDetails, claimDetails)
Client->>Storage: insert_chain(swap_id, ChainSwapData)
Storage-->>Client: Ok
Client-->>User: show ark_lockup_address / bip21
User->>ArkNet: Fund VHTLC to ark_lockup_address
ArkNet-->>Client: confirmation events (polling)
Client->>BoltzAPI: GET /v2/swap/{id} (status polling)
BoltzAPI-->>Client: TransactionServerConfirmed
Client->>BTCNet: Build MuSig2 claim tx (ephemeral + server)
Client->>BoltzAPI: POST /v2/swap/{id}/claim (partial sig)
BoltzAPI-->>Client: server_partial_sig
Client->>BTCNet: Broadcast aggregated tx
Client->>Storage: update_chain(swap_id, status: Claimed)
Client-->>User: Txid
sequenceDiagram
participant User
participant Client
participant BoltzAPI as Boltz API
participant BTCNet as BTC Network
participant ArkNet as ARK Network
participant Storage
User->>Client: create_chain_swap(BtcToArk, amount)
Client->>BoltzAPI: POST /v2/swap/chain (from: BTC,to:ARK)
BoltzAPI-->>Client: ChainSwapResult (swap_id, btc_lockup_address, vhtlc_details)
Client->>Storage: insert_chain(swap_id, ChainSwapData)
Client-->>User: show btc_lockup_address / bip21
User->>BTCNet: Send BTC to btc_lockup_address
BTCNet-->>Client: confirmation events (polling)
Client->>BoltzAPI: GET /v2/swap/{id} (status polling)
BoltzAPI-->>Client: TransactionServerConfirmed
Client->>ArkNet: Construct VHTLC claim tx
Client->>ArkNet: Broadcast VHTLC claim tx
Client->>Storage: update_chain(swap_id, status: Claimed)
Client-->>User: Txid
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
🔍 Arkana PR Review — Chain Swaps (rust-sdk)Scope: 1822 additions across 9 files — adds ARK ↔ BTC chain swap support via Boltz API, including create, claim (both Ark VHTLC and on-chain BTC), refund (both paths), status query, and persistent storage (SQLite + in-memory). ✅ What looks good
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (5)
ark-client-sample/src/main.rs (1)
821-825: Consider logging the original error before fallback.When
claim_chain_swapfails, the error is silently discarded before attempting the BTC claim fallback. This makes debugging harder if both paths fail.🔧 Proposed improvement
match client.claim_chain_swap(swap_id).await { Ok(txid) => { tracing::info!(%txid, swap_id, "Chain swap claimed (ARK VHTLC)"); } - Err(_) => { + Err(ark_err) => { + tracing::debug!("ARK VHTLC claim failed ({ark_err}), trying on-chain BTC claim"); let destination: Address = address🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ark-client-sample/src/main.rs` around lines 821 - 825, When handling the Err branch of client.claim_chain_swap(swap_id).await (in the match that currently drops the error), log the original error before attempting the BTC claim fallback; capture the error from the Err(_) arm (e.g., Err(e)) and emit a tracing::warn or tracing::error including %e and swap_id to preserve context, then proceed with the existing BTC fallback logic so both the original failure and the fallback attempt are visible for debugging.ark-client/src/swap_storage/sqlite.rs (2)
394-416: Missingrows_affectedcheck inupdate_status_chain.Unlike
update_status_submarine(lines 197-199) andupdate_status_reverse(lines 228-230), this method doesn't verify the UPDATE statement actually modified a row. Whileget_chainis called first, there's a TOCTOU race window.Also, the error message at line 398 uses lowercase "chain swap" while existing methods use title case ("Submarine swap", "Reverse swap").
🔧 Proposed fix for consistency
async fn update_status_chain(&self, id: &str, status: SwapStatus) -> Result<(), Error> { let mut swap_data = self .get_chain(id) .await? - .ok_or_else(|| Error::consumer(format!("chain swap not found: {id}")))?; + .ok_or_else(|| Error::consumer(format!("Chain swap not found: {id}")))?; swap_data.status = status; let data_json = serde_json::to_string(&swap_data) .map_err(|e| Error::consumer(format!("Failed to serialize chain swap data: {e}")))?; let now = Self::current_timestamp(); - sqlx::query("UPDATE chain_swaps SET data = ?, updated_at = ? WHERE id = ?") + let result = sqlx::query("UPDATE chain_swaps SET data = ?, updated_at = ? WHERE id = ?") .bind(&data_json) .bind(now) .bind(id) .execute(&self.pool) .await .map_err(|e| Error::consumer(format!("Failed to update chain swap status: {e}")))?; + if result.rows_affected() == 0 { + return Err(Error::consumer(format!("Chain swap not found: {id}"))); + } + Ok(()) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ark-client/src/swap_storage/sqlite.rs` around lines 394 - 416, The update_status_chain function has a TOCTOU hole because it doesn't verify the UPDATE actually modified a row and its error text uses lowercase; update update_status_chain to capture the result of sqlx::query(...).execute(&self.pool).await, check result.rows_affected() and return an Error.consumer when rows_affected() == 0 (matching the behavior in update_status_submarine/update_status_reverse), and change the earlier not-found error text from "chain swap not found" to "Chain swap not found" (also ensure the serialize/update failure messages similarly use "Chain swap" title-case for consistency); keep references to get_chain, current_timestamp, and the chain_swaps UPDATE query when making the changes.
432-434: Inconsistent error message casing.Line 433 uses lowercase "chain swap" while other methods use title case (e.g., "Submarine swap not found", "Reverse swap not found").
🔧 Proposed fix
if result.rows_affected() == 0 { - return Err(Error::consumer(format!("chain swap not found: {id}"))); + return Err(Error::consumer(format!("Chain swap not found: {id}"))); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ark-client/src/swap_storage/sqlite.rs` around lines 432 - 434, The error string returned when no rows are affected uses lowercase "chain swap not found" which is inconsistent with other error messages; update the Error::consumer(format!(...)) call in the rows_affected() check to use title case "Chain swap not found" so it matches messages like "Submarine swap not found" and "Reverse swap not found" and keep the rest of the error handling (the rows_affected() check and return) unchanged.ark-client/src/swap_storage/mod.rs (1)
159-176: Consider adding consistent documentation for chain swap methods.The new chain swap trait methods have minimal doc comments compared to the existing submarine/reverse methods (lines 32-158), which include detailed
# Arguments,# Returns, and# Errorssections. For API consistency, consider adding similar documentation.📝 Example documentation pattern
+ /// Store chain swap data. + /// + /// # Arguments + /// * `id` - Unique identifier for the swap + /// * `data` - The swap data to store + /// + /// # Errors + /// Returns an error if the swap cannot be stored (e.g., database error, duplicate ID). async fn insert_chain(&self, id: String, data: ChainSwapData) -> Result<(), Error>;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ark-client/src/swap_storage/mod.rs` around lines 159 - 176, The chain-swap trait methods (insert_chain, get_chain, update_status_chain, update_chain, list_all_chain, remove_chain) currently lack the detailed doc comments used by the submarine/reverse methods; update each method with consistent Rustdoc including a short description plus `# Arguments` listing parameter meanings (e.g., `id`, `data`, `status`), `# Returns` describing the Result/Option shapes (e.g., Result<(), Error>, Result<Option<ChainSwapData>, Error>, Vec<ChainSwapData>), and `# Errors` describing failure cases (e.g., persistence or not-found errors) to match the existing documentation pattern used earlier in the file.ark-client/src/boltz.rs (1)
2872-2877: Consider reusing the HTTP client across poll iterations.Creating a new
reqwest::Clienton each iteration of the polling loop adds unnecessary overhead. Move the client creation outside the loop for better efficiency.♻️ Suggested refactor
async_stream::stream! { let mut last_status: Option<SwapStatus> = None; let url = format!("{}/v2/swap/{swap_id}", self.inner.boltz_url); + let client = reqwest::Client::new(); loop { - let client = reqwest::Client::new(); let response = client .get(&url) .send() .await;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ark-client/src/boltz.rs` around lines 2872 - 2877, The polling loop currently calls reqwest::Client::new() on every iteration (see the loop and the client variable), causing unnecessary overhead; move the client creation out of the loop by creating a single let client = reqwest::Client::new(); before entering the loop and reuse that client for each client.get(&url).send().await call (or clone it if you need independent handles), so the loop reuses the same reqwest::Client instance.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ark-client-sample/mutinynet/ark.mnemonic`:
- Line 1: Remove the committed mnemonic line in ark.mnemonic from the repo
history and delete the file from the working tree, add a rule to .gitignore to
prevent re-adding mnemonic files, and replace the committed file with a template
(e.g., ark.mnemonic.template) or documentation that instructs developers to
generate a mnemonic at runtime with a provided generation script or environment
variable; if you keep a template ensure it contains no real words and includes a
trailing newline. Also, if this mnemonic was ever used, rotate any derived
keys/credentials immediately. Target the file name ark.mnemonic and the
template/generation mechanism you add to the repo when making these changes.
In `@ark-client-sample/src/main.rs`:
- Around line 877-880: The BTC address parsed into destination currently uses
assume_checked(), which skips network validation; replace that with
require_network() to enforce the expected network. Locate the parsing expression
(addr_str.parse::<Address<NetworkUnchecked>>() -> ... .assume_checked()) and
change the final call to .require_network() (passing the appropriate Network or
NetworkId if needed) so the Address type is validated against the intended
network rather than assumed.
- Around line 826-831: The BTC address parsing uses Address<NetworkUnchecked>
and doesn't validate it against the client's network; update the code that sets
destination (the Address parse chain) to ensure the parsed Address matches the
client's network: either parse into the concrete client network type instead of
NetworkUnchecked or, after parsing and assume_checked(), compare the
address.network() (or equivalent) to the client's network and return an error if
they differ (use the same client network value used elsewhere in this
module/struct). Ensure the error message mirrors other commands (e.g.,
ChainSwap) and uses anyhow! to report a network-mismatch failure.
- Around line 773-778: The BTC address parsing in the ark-to-btc flow currently
uses assume_checked() without verifying it matches the client's network; update
the flow that builds destination: Address by validating the parsed
Address<NetworkUnchecked> against the current network (similar to how
SendOnchain uses require_network(network)) before calling assume_checked(),
e.g., call the same require_network(...) helper or equivalent check on the
parsed value and return an error if mismatched so only network-matching
addresses are assumed checked.
---
Nitpick comments:
In `@ark-client-sample/src/main.rs`:
- Around line 821-825: When handling the Err branch of
client.claim_chain_swap(swap_id).await (in the match that currently drops the
error), log the original error before attempting the BTC claim fallback; capture
the error from the Err(_) arm (e.g., Err(e)) and emit a tracing::warn or
tracing::error including %e and swap_id to preserve context, then proceed with
the existing BTC fallback logic so both the original failure and the fallback
attempt are visible for debugging.
In `@ark-client/src/boltz.rs`:
- Around line 2872-2877: The polling loop currently calls reqwest::Client::new()
on every iteration (see the loop and the client variable), causing unnecessary
overhead; move the client creation out of the loop by creating a single let
client = reqwest::Client::new(); before entering the loop and reuse that client
for each client.get(&url).send().await call (or clone it if you need independent
handles), so the loop reuses the same reqwest::Client instance.
In `@ark-client/src/swap_storage/mod.rs`:
- Around line 159-176: The chain-swap trait methods (insert_chain, get_chain,
update_status_chain, update_chain, list_all_chain, remove_chain) currently lack
the detailed doc comments used by the submarine/reverse methods; update each
method with consistent Rustdoc including a short description plus `# Arguments`
listing parameter meanings (e.g., `id`, `data`, `status`), `# Returns`
describing the Result/Option shapes (e.g., Result<(), Error>,
Result<Option<ChainSwapData>, Error>, Vec<ChainSwapData>), and `# Errors`
describing failure cases (e.g., persistence or not-found errors) to match the
existing documentation pattern used earlier in the file.
In `@ark-client/src/swap_storage/sqlite.rs`:
- Around line 394-416: The update_status_chain function has a TOCTOU hole
because it doesn't verify the UPDATE actually modified a row and its error text
uses lowercase; update update_status_chain to capture the result of
sqlx::query(...).execute(&self.pool).await, check result.rows_affected() and
return an Error.consumer when rows_affected() == 0 (matching the behavior in
update_status_submarine/update_status_reverse), and change the earlier not-found
error text from "chain swap not found" to "Chain swap not found" (also ensure
the serialize/update failure messages similarly use "Chain swap" title-case for
consistency); keep references to get_chain, current_timestamp, and the
chain_swaps UPDATE query when making the changes.
- Around line 432-434: The error string returned when no rows are affected uses
lowercase "chain swap not found" which is inconsistent with other error
messages; update the Error::consumer(format!(...)) call in the rows_affected()
check to use title case "Chain swap not found" so it matches messages like
"Submarine swap not found" and "Reverse swap not found" and keep the rest of the
error handling (the rows_affected() check and return) unchanged.
🪄 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: 5a87f3a5-7f71-496f-99cb-0288d57672ab
📒 Files selected for processing (9)
ark-client-sample/justfileark-client-sample/mutinynet/ark.mnemonicark-client-sample/src/main.rsark-client/migrations/002_chain_swaps.sqlark-client/src/boltz.rsark-client/src/lib.rsark-client/src/swap_storage/memory.rsark-client/src/swap_storage/mod.rsark-client/src/swap_storage/sqlite.rs
bonomat
force-pushed
the
feat/chain-swaps
branch
from
1549374 to
8da00fc
Compare
🔍 Arkana PR Review — Chain Swaps (
|
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ark-client-sample/src/main.rs`:
- Around line 821-839: The current code treats any Err from
client.claim_chain_swap(swap_id) as meaning "wrong chain" and always falls back
to on-chain BTC via claim_chain_swap_btc, which hides real failures; change the
logic in the claim_flow (the match around client.claim_chain_swap and the
analogous refund_flow around client.refund_chain_swap) to only take the BTC path
when you can deterministically detect the swap direction or when the error is
the explicit "wrong side" variant from the client API (e.g., inspect the Err
value for a WrongSwapDirection/WrongSide/SwapNotOnArk variant or query the swap
metadata before deciding), otherwise propagate or log other errors. In short:
replace Err(_) branches with explicit error matching (or a pre-check of swap
direction via client.get_swap or similar) and only call claim_chain_swap_btc or
the BTC refund when the swap is known to be BTC-side; rethrow/return other
errors unchanged.
In `@ark-client/src/boltz.rs`:
- Around line 1824-1827: The current logic sets bip21 from
swap_response.lockup_details.bip21.or(swap_response.claim_details.bip21.clone()),
which can expose the server-funded BTC output for ArkToBtc; change it to only
populate ChainSwapResult::bip21 when the local/user-funded leg is the BTC leg.
Concretely, in the code that constructs bip21 (and the similar block at the
other location), check the swap direction / enum variant (e.g., ArkToBtc vs
BtcToArk or a field indicating which side is user-funded) and only take
lockup_details.bip21 (or claim_details.bip21 when appropriate) when that side is
the user's funding leg; otherwise leave bip21 as None so callers aren't pointed
at the server-funded URI. Use the symbols swap_response, lockup_details,
claim_details, and ChainSwapResult::bip21 to find and update both occurrences.
- Around line 2930-2935: list_pending_vhtlc_spend_txs currently calls
collect_active_vhtlc_infos which only returns submarine/reverse swaps and thus
misses chain-swap spends created by claim_chain_swap()/refund_chain_swap();
update the recovery path to include those by either extending
collect_active_vhtlc_infos to also enumerate active chain-swap records (the
offchain submissions created by claim_chain_swap and refund_chain_swap) or add a
new collector (e.g., collect_active_chain_swap_infos) and merge its results into
list_pending_vhtlc_spend_txs before querying VHTLC addresses so pending
chain-swap spend TXs are discovered.
- Around line 405-418: The match currently swallows terminal statuses
(SwapStatus::SwapExpired, SwapStatus::TransactionLockupFailed, and
SwapStatus::Other(_)) causing waiters in subscribe_to_swap_updates() to block
indefinitely; change those arms to treat them as failures by invoking the same
failure path used for other terminal errors (e.g., return Err/Boltz failure
variant or call the existing notify/fail handler) instead of an empty block so
waiters are unblocked. Update the match in subscribe_to_swap_updates() (and the
other similar matches at the noted ranges) to map SwapStatus::SwapExpired,
SwapStatus::TransactionLockupFailed, and SwapStatus::Other(_) to the failure
handling code path used for TransactionFailed/InvoiceFailedToPay/etc., ensuring
the waiter receives an error/terminal notification.
- Around line 2739-2818: get_fees currently only requests submarine and reverse
endpoints and therefore never populates BoltzFees for chain swaps; add a third
fetch to call the Boltz chain-swap fee endpoint, deserialize into a
new/appropriate response type (e.g., ChainPairsResponse), map its fees into the
BoltzFees.chain field (create a ChainSwapFees struct if missing) and return that
along with submarine and reverse; update get_fees to build the chain URL (e.g.,
format!("{}/v2/swap/chain", self.inner.boltz_url) or the actual chain endpoint
used elsewhere), handle non-success status and JSON deserialize errors similarly
to submarine_response/reverse_response, and reference the new types
ChainPairsResponse and ChainSwapFees when extracting the fee fields.
🪄 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: 814bfaa2-d43c-4a9c-a64f-72684fb92c61
📒 Files selected for processing (3)
ark-client-sample/justfileark-client-sample/src/main.rsark-client/src/boltz.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- ark-client-sample/justfile
| match client.claim_chain_swap(swap_id).await { | ||
| Ok(txid) => { | ||
| tracing::info!(%txid, swap_id, "Chain swap claimed (ARK VHTLC)"); | ||
| } | ||
| Err(_) => { | ||
| let destination: Address = address | ||
| .as_deref() | ||
| .ok_or_else(|| anyhow!("--address is required for ark-to-btc claims"))? | ||
| .parse::<Address<NetworkUnchecked>>() | ||
| .map_err(|e| anyhow!("invalid BTC address: {e}"))? | ||
| .assume_checked(); | ||
|
|
||
| let txid = client | ||
| .claim_chain_swap_btc(swap_id, destination, *fee_rate) | ||
| .await | ||
| .map_err(|e| anyhow!(e))?; | ||
|
|
||
| tracing::info!(%txid, swap_id, "Chain swap claimed (on-chain BTC)"); | ||
| } |
There was a problem hiding this comment.
Don't use a blanket Err as the direction switch.
claim_chain_swap() and refund_chain_swap() can fail for ordinary reasons too. Falling back on every error hides the real failure and can turn a BtcToArk problem into a misleading BTC-address error. Only take the BTC path when the swap direction is known, or when the Ark call failed with the explicit wrong-side condition.
Also applies to: 861-889
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ark-client-sample/src/main.rs` around lines 821 - 839, The current code
treats any Err from client.claim_chain_swap(swap_id) as meaning "wrong chain"
and always falls back to on-chain BTC via claim_chain_swap_btc, which hides real
failures; change the logic in the claim_flow (the match around
client.claim_chain_swap and the analogous refund_flow around
client.refund_chain_swap) to only take the BTC path when you can
deterministically detect the swap direction or when the error is the explicit
"wrong side" variant from the client API (e.g., inspect the Err value for a
WrongSwapDirection/WrongSide/SwapNotOnArk variant or query the swap metadata
before deciding), otherwise propagate or log other errors. In short: replace
Err(_) branches with explicit error matching (or a pre-check of swap direction
via client.get_swap or similar) and only call claim_chain_swap_btc or the BTC
refund when the swap is known to be BTC-side; rethrow/return other errors
unchanged.
| SwapStatus::InvoiceSet | ||
| | SwapStatus::InvoicePending | ||
| | SwapStatus::Created | ||
| | SwapStatus::TransactionMempool | ||
| | SwapStatus::TransactionConfirmed | ||
| | SwapStatus::TransactionServerMempool | ||
| | SwapStatus::TransactionServerConfirmed | ||
| | SwapStatus::TransactionRefunded | ||
| | SwapStatus::TransactionFailed | ||
| | SwapStatus::TransactionClaimed | ||
| | SwapStatus::TransactionLockupFailed | ||
| | SwapStatus::InvoiceFailedToPay | ||
| | SwapStatus::SwapExpired => {} | ||
| | SwapStatus::SwapExpired | ||
| | SwapStatus::Other(_) => {} |
There was a problem hiding this comment.
Treat the new terminal statuses as failures instead of waiting forever.
These arms now swallow SwapExpired and TransactionLockupFailed. Because subscribe_to_swap_updates() only emits on status changes and keeps polling after terminal states, each waiter can block indefinitely once Boltz settles on one of those values. Other(_) has the same failure mode if the server stays on an unrecognized status.
Also applies to: 1265-1278, 1547-1560, 1937-1948
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ark-client/src/boltz.rs` around lines 405 - 418, The match currently swallows
terminal statuses (SwapStatus::SwapExpired, SwapStatus::TransactionLockupFailed,
and SwapStatus::Other(_)) causing waiters in subscribe_to_swap_updates() to
block indefinitely; change those arms to treat them as failures by invoking the
same failure path used for other terminal errors (e.g., return Err/Boltz failure
variant or call the existing notify/fail handler) instead of an empty block so
waiters are unblocked. Update the match in subscribe_to_swap_updates() (and the
other similar matches at the noted ranges) to map SwapStatus::SwapExpired,
SwapStatus::TransactionLockupFailed, and SwapStatus::Other(_) to the failure
handling code path used for TransactionFailed/InvoiceFailedToPay/etc., ensuring
the waiter receives an error/terminal notification.
| let bip21 = swap_response | ||
| .lockup_details | ||
| .bip21 | ||
| .or(swap_response.claim_details.bip21.clone()); |
There was a problem hiding this comment.
Only expose bip21 for the user-funded BTC leg.
lockup_details.bip21.or(claim_details.bip21.clone()) can populate ChainSwapResult::bip21 from the server's BTC output in ArkToBtc. The result type documents this field as the URI the caller should fund, so this can point clients at the wrong side of the swap.
🔧 Suggested fix
- let bip21 = swap_response
- .lockup_details
- .bip21
- .or(swap_response.claim_details.bip21.clone());
+ let bip21 = match &direction {
+ ChainSwapDirection::BtcToArk => swap_response.lockup_details.bip21.clone(),
+ ChainSwapDirection::ArkToBtc => None,
+ };Also applies to: 1872-1878
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ark-client/src/boltz.rs` around lines 1824 - 1827, The current logic sets
bip21 from
swap_response.lockup_details.bip21.or(swap_response.claim_details.bip21.clone()),
which can expose the server-funded BTC output for ArkToBtc; change it to only
populate ChainSwapResult::bip21 when the local/user-funded leg is the BTC leg.
Concretely, in the code that constructs bip21 (and the similar block at the
other location), check the swap direction / enum variant (e.g., ArkToBtc vs
BtcToArk or a field indicating which side is user-funded) and only take
lockup_details.bip21 (or claim_details.bip21 when appropriate) when that side is
the user's funding leg; otherwise leave bip21 as None so callers aren't pointed
at the server-funded URI. Use the symbols swap_response, lockup_details,
claim_details, and ChainSwapResult::bip21 to find and update both occurrences.
| /// Fetch fee information from Boltz for both submarine and reverse swaps. | ||
| /// | ||
| /// # Returns | ||
| /// | ||
| /// - A [`BoltzFees`] struct containing fee information for both swap types. | ||
| pub async fn get_fees(&self) -> Result<BoltzFees, Error> { | ||
| let client = reqwest::Client::builder() | ||
| .timeout(self.inner.timeout) | ||
| .build() | ||
| .map_err(|e| Error::ad_hoc(e.to_string()))?; | ||
|
|
||
| // Fetch submarine swap fees (ARK -> BTC) | ||
| let submarine_url = format!("{}/v2/swap/submarine", &self.inner.boltz_url); | ||
| let submarine_response = client | ||
| .get(&submarine_url) | ||
| .send() | ||
| .await | ||
| .map_err(|e| Error::ad_hoc(e.to_string())) | ||
| .context("failed to fetch submarine swap fees")?; | ||
|
|
||
| if !submarine_response.status().is_success() { | ||
| let error_text = submarine_response | ||
| .text() | ||
| .await | ||
| .map_err(|e| Error::ad_hoc(e.to_string()))?; | ||
| return Err(Error::ad_hoc(format!( | ||
| "failed to fetch submarine swap fees: {error_text}" | ||
| ))); | ||
| } | ||
|
|
||
| let submarine_pairs: SubmarinePairsResponse = submarine_response | ||
| .json() | ||
| .await | ||
| .map_err(|e| Error::ad_hoc(e.to_string())) | ||
| .context("failed to deserialize submarine swap fees response")?; | ||
|
|
||
| let submarine_pair_fees = &submarine_pairs.ark.btc.fees; | ||
| let submarine_fees = SubmarineSwapFees { | ||
| percentage: submarine_pair_fees.percentage, | ||
| miner_fees: submarine_pair_fees.miner_fees, | ||
| }; | ||
|
|
||
| // Fetch reverse swap fees (BTC -> ARK) | ||
| let reverse_url = format!("{}/v2/swap/reverse", self.inner.boltz_url); | ||
| let reverse_response = client | ||
| .get(&reverse_url) | ||
| .send() | ||
| .await | ||
| .map_err(|e| Error::ad_hoc(e.to_string())) | ||
| .context("failed to fetch reverse swap fees")?; | ||
|
|
||
| if !reverse_response.status().is_success() { | ||
| let error_text = reverse_response | ||
| .text() | ||
| .await | ||
| .map_err(|e| Error::ad_hoc(e.to_string()))?; | ||
| return Err(Error::ad_hoc(format!( | ||
| "failed to fetch reverse swap fees: {error_text}" | ||
| ))); | ||
| } | ||
|
|
||
| let reverse_pairs: ReversePairsResponse = reverse_response | ||
| .json() | ||
| .await | ||
| .map_err(|e| Error::ad_hoc(e.to_string())) | ||
| .context("failed to deserialize reverse swap fees response")?; | ||
|
|
||
| let reverse_pair_fees = &reverse_pairs.btc.ark.fees; | ||
| let reverse_fees = ReverseSwapFees { | ||
| percentage: reverse_pair_fees.percentage, | ||
| miner_fees: ReverseMinerFees { | ||
| lockup: reverse_pair_fees.miner_fees.lockup, | ||
| claim: reverse_pair_fees.miner_fees.claim, | ||
| }, | ||
| }; | ||
|
|
||
| Ok(BoltzFees { | ||
| submarine: submarine_fees, | ||
| reverse: reverse_fees, | ||
| }) |
There was a problem hiding this comment.
get_fees() still leaves chain swaps without a quote path.
This implementation only fetches /v2/swap/submarine and /v2/swap/reverse. The new ARK↔BTC flow therefore has no programmatic Boltz fee quote, even though chain-fee support is part of this change set.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ark-client/src/boltz.rs` around lines 2739 - 2818, get_fees currently only
requests submarine and reverse endpoints and therefore never populates BoltzFees
for chain swaps; add a third fetch to call the Boltz chain-swap fee endpoint,
deserialize into a new/appropriate response type (e.g., ChainPairsResponse), map
its fees into the BoltzFees.chain field (create a ChainSwapFees struct if
missing) and return that along with submarine and reverse; update get_fees to
build the chain URL (e.g., format!("{}/v2/swap/chain", self.inner.boltz_url) or
the actual chain endpoint used elsewhere), handle non-success status and JSON
deserialize errors similarly to submarine_response/reverse_response, and
reference the new types ChainPairsResponse and ChainSwapFees when extracting the
fee fields.
| /// List pending (submitted but not finalized) VHTLC spend transactions. | ||
| /// | ||
| /// This checks all non-terminal swaps in storage, queries the server for pending VTXOs | ||
| /// on their VHTLC addresses, and determines the spend type from the PSBT data. | ||
| pub async fn list_pending_vhtlc_spend_txs(&self) -> Result<Vec<PendingVhtlcSpendTx>, Error> { | ||
| let vhtlc_infos = self.collect_active_vhtlc_infos().await?; |
There was a problem hiding this comment.
Pending chain-swap Ark spends are still unrecoverable.
Line 2935 still goes through collect_active_vhtlc_infos(), which only enumerates submarine and reverse swaps. The new claim_chain_swap() / refund_chain_swap() paths also submit Ark offchain transactions before finalization, so a restart between submit and finalize leaves pending chain-swap spends that this recovery command can never see.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ark-client/src/boltz.rs` around lines 2930 - 2935,
list_pending_vhtlc_spend_txs currently calls collect_active_vhtlc_infos which
only returns submarine/reverse swaps and thus misses chain-swap spends created
by claim_chain_swap()/refund_chain_swap(); update the recovery path to include
those by either extending collect_active_vhtlc_infos to also enumerate active
chain-swap records (the offchain submissions created by claim_chain_swap and
refund_chain_swap) or add a new collector (e.g.,
collect_active_chain_swap_infos) and merge its results into
list_pending_vhtlc_spend_txs before querying VHTLC addresses so pending
chain-swap spend TXs are discovered.
we extract a helper function to reconstruct the onchain htlc and add a test.
🔍 Review — Chain Swaps (ARK ↔ BTC via Boltz)Large PR adding full chain swap lifecycle. Solid foundation — both directions work, storage is clean, tests verify address reconstruction. Several items to consider: Security & Protocol
Code Quality
Storage
Missing
Cross-repo impact
VerdictSolid implementation of a complex feature. The |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ark-client/src/boltz.rs`:
- Around line 2828-2834: The polling loop currently constructs a fresh
reqwest::Client each iteration via reqwest::Client::new(), with no timeout,
which can block indefinitely; fix this by building a single Client with a
timeout (e.g., using
reqwest::Client::builder().timeout(Duration::from_secs(...)).build()) outside
the loop and reuse that client for the .get(&url).send().await calls (mirroring
the approach used in get_fees()), removing per-iteration client creation so
requests respect the configured timeout.
🪄 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: ec1da007-1e70-4954-98a4-d0ba83ddfa86
📒 Files selected for processing (1)
ark-client/src/boltz.rs
|
|
||
| loop { | ||
| let client = reqwest::Client::new(); | ||
| let response = client | ||
| .get(&url) | ||
| .send() | ||
| .await; |
There was a problem hiding this comment.
HTTP client lacks timeout, risking indefinite blocking.
The polling loop creates a new reqwest::Client each iteration without a timeout. If the HTTP request hangs (network issue, unresponsive server), the stream will block indefinitely. Compare with get_fees() which builds the client with a timeout.
Consider reusing a single client with a configured timeout:
🛠️ Suggested fix
async_stream::stream! {
let mut last_status: Option<SwapStatus> = None;
let url = format!("{}/v2/swap/{swap_id}", self.inner.boltz_url);
+ let client = reqwest::Client::builder()
+ .timeout(self.inner.timeout)
+ .build()
+ .map_err(|e| Error::ad_hoc(e.to_string()));
+ let client = match client {
+ Ok(c) => c,
+ Err(e) => {
+ yield Err(e);
+ return;
+ }
+ };
loop {
- let client = reqwest::Client::new();
let response = client
.get(&url)
.send()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ark-client/src/boltz.rs` around lines 2828 - 2834, The polling loop currently
constructs a fresh reqwest::Client each iteration via reqwest::Client::new(),
with no timeout, which can block indefinitely; fix this by building a single
Client with a timeout (e.g., using
reqwest::Client::builder().timeout(Duration::from_secs(...)).build()) outside
the loop and reuse that client for the .get(&url).send().await calls (mirroring
the approach used in get_fees()), removing per-iteration client creation so
requests respect the configured timeout.
luckysori
left a comment
luckysori
left a comment
There was a problem hiding this comment.
Very nice.
We can make things nicer once we extract all the Boltz swap into a dedicated crate. For now: the feature works (tested locally).
I will open an issue about the missing collab-refund. Shouldn't be too hard to add.
2 participants
resolves #187
Open todo:
Summary by CodeRabbit