test(e2e): integration tests for ElectrumOnchainProvider over regtest - #461
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:
WalkthroughUpdates the Changes
Sequence Diagram(s)sequenceDiagram
participant Wallet
participant Provider as ElectrumOnchainProvider
participant Electrum as ElectrumWS
Wallet->>Electrum: broadcast_transaction(raw_hex)
Electrum-->>Wallet: txid
Wallet->>Provider: send / request txs / watchAddresses
Provider->>Electrum: blockchain.scripthash.get_history (batch)
Electrum-->>Provider: histories (txids, heights)
Provider->>Electrum: blockchain.transaction.get (raw hex) [batched safeRequest]
Electrum-->>Provider: raw tx hexes
Provider->>Electrum: blockchain.block.header (batch heights)
Electrum-->>Provider: block headers
Provider->>Electrum: blockchain.transaction.get_merkle (per-tx)
Electrum-->>Provider: merkle (block_height or tx-not-in-block)
Provider-->>Wallet: emit tx status / coins / watch callbacks (after mapping succeeds)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
ghost
left a comment
ghost
left a comment
There was a problem hiding this comment.
✅ Approve — test-only, no protocol-critical changes
Clean e2e test suite for ElectrumOnchainProvider that mirrors the existing esplora coverage. Reviewed the full 344-line diff against the production ElectrumOnchainProvider (electrum.ts:445-843), WsElectrumChainSource (electrum.ts:146-427), OnchainProvider interface, OnchainWallet, and the existing test/e2e/onchain.test.ts.
What's good
- Full interface coverage. Every
OnchainProvidermethod is exercised against a real electrs backend. ThewatchAddressestest is especially valuable — it catches subscription lifecycle bugs that unit mocks miss. - Correct value handling.
Number(out.value) === satsat line 120 is correct —ExplorerTransaction.vout.valueis typedstring, and the electrum provider'sverboseToExplorerreturnsString(...)from exact-sats parsing. TheNumber()cast is safe for these test amounts. - Good test isolation. Fresh
SingleKey.fromRandomBytes()per test, separate WS connections perdescribeblock,finally-guardedstop()on the watch subscription. - Concurrency correctness. The
fetchHistoriesbatch test (line 302-320) correctly validates the parallel round-trip path. - Proper cleanup. Both
afterAllblocks close the provider with.catch(() => {})to swallow already-closed errors.
Minor notes (non-blocking)
decodeP2trScripthelper (line 337-339): Utility function duplicates the privateencodeAddresslogic inElectrumOnchainProvider. Not worth exporting just for a test, but noting it.- Submodule pin (regtest →
96d02a59): Points at theelectrum-ws-bridgenigiri branch per ArkLabsHQ/arkade-regtest#12. PR description clearly states this will be rebased to master once vulpemventures/nigiri#258 lands. This PR should not merge until that submodule pointer is stable on master. - Two WS connections: The two
describeblocks each open their ownElectrumWS. Acceptable for isolation but worth noting the test suite uses 2 concurrent connections during the run.
Cross-repo impact
- None. Test-only change, no public API modifications. Checked
src/index.tsexports — no changes. No downstream SDK breakage possible.
Verdict
Ship it. Good parity testing between Esplora and Electrum providers. Just track the submodule rebase before merge.
🤖 Reviewed by Arkana
ghost
left a comment
ghost
left a comment
There was a problem hiding this comment.
✅ Approve — test-only, no protocol-critical changes
Clean e2e test suite for ElectrumOnchainProvider that mirrors the existing esplora coverage. Reviewed the full 344-line diff against the production ElectrumOnchainProvider (electrum.ts:445-843), WsElectrumChainSource (electrum.ts:146-427), OnchainProvider interface, OnchainWallet, and the existing test/e2e/onchain.test.ts.
What's good
- Full interface coverage. Every
OnchainProvidermethod is exercised against a real electrs backend. ThewatchAddressestest is especially valuable — it catches subscription lifecycle bugs that unit mocks miss. - Correct value handling.
Number(out.value) === satsat line 120 is correct —ExplorerTransaction.vout.valueis typedstring, and the electrum provider'sverboseToExplorerreturnsString(...)from exact-sats parsing. TheNumber()cast is safe for these test amounts. - Good test isolation. Fresh
SingleKey.fromRandomBytes()per test, separate WS connections perdescribeblock,finally-guardedstop()on the watch subscription. - Concurrency correctness. The
fetchHistoriesbatch test (lines 302-320) correctly validates the parallel round-trip path. - Proper cleanup. Both
afterAllblocks close the provider with.catch(() => {})to swallow already-closed errors.
Minor notes (non-blocking)
decodeP2trScripthelper (line 337-339): Utility function duplicates the privateencodeAddresslogic inElectrumOnchainProvider. Not worth exporting just for a test, but noting it.- Submodule pin (regtest →
96d02a59): Points at theelectrum-ws-bridgenigiri branch per ArkLabsHQ/arkade-regtest#12. PR description clearly states this will be rebased to master once vulpemventures/nigiri#258 lands. This PR should not merge until that submodule pointer is stable on master. - Two WS connections: The two
describeblocks each open their ownElectrumWS. Acceptable for isolation but worth noting the test suite uses 2 concurrent connections during the run.
Cross-repo impact
- None. Test-only change, no public API modifications. Checked
src/index.tsexports — no changes. No downstream SDK breakage possible.
Verdict
Ship it. Good parity testing between Esplora and Electrum providers. Just track the submodule rebase before merge.
🤖 Reviewed by Arkana
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
test/e2e/electrum.test.ts (1)
17-29: Replace fixed delays withwaitForpolling to reduce e2e flakiness.This test file uses fixed 5-10 second delays, but the codebase already standardizes on
waitForwith polling (defined intest/e2e/utils.ts, used extensively insettlement.test.ts,ark.test.ts, etc.). Replace the fixed delays withwaitForloops that poll the actual conditions (coin availability, balance, etc.) with a short interval, which reduces false failures under CI load and aligns with the established test pattern.Applies to lines: 105, 153, 161, 190, 202, 236, 265, 321, 330
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/electrum.test.ts` around lines 17 - 29, The tests use fixed delays (FAUCET_PROPAGATION_MS, direct setTimeout via delay, and hardcoded waits) which should be replaced with the project's waitFor polling helper from test/e2e/utils.ts; update the tests to remove uses of FAUCET_PROPAGATION_MS and delay and instead call waitFor polling the real conditions (e.g., coin availability, wallet balance, mempool/tx visibility) with a short interval until the expected state is observed, and update any helper usages (faucet function can keep returning satoshis) so callers use waitFor to await propagation rather than sleeping.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/e2e/electrum.test.ts`:
- Around line 126-132: The test currently asserts exact equality for an
unconfirmed result from provider.getTxStatus (variable status), which is
brittle; change the assertion to a partial match so only required fields are
checked (e.g., assert that status.confirmed is false using toMatchObject or
expect.objectContaining) while leaving other possible fields untouched; update
the branch that handles !status.confirmed in electrum.test.ts to use a partial
matcher against status instead of strict equality.
- Around line 241-242: The test dereferences coins[0] without ensuring coins is
non-empty; before accessing coins[0] (after calling
provider.getCoins(wallet.address)), add a defensive assertion that coins has at
least one element (e.g., assert coins.length > 0 or
expect(coins).not.toHaveLength(0)) so the subsequent
expect(seen).toContain(coins[0].txid) cannot throw on undefined; update the test
around the provider.getCoins call in electrum.test.ts to perform this check
first.
- Around line 2-25: The faucet() helper currently uses execSync with
shell-interpolated arguments and no timeout, which can hang CI; replace it with
child_process.execFileSync using the command "nigiri" and an args array [
"faucet", address, btc.toString() ] (or format btc as needed), add a timeout
option (e.g. 30_000 ms) and stdio options as appropriate, and catch/propagate
errors so failures surface instead of hanging; update any other e2e helpers
(e.g. in vhtlc.test.ts, onchain.test.ts) that call execSync to the same
execFileSync pattern and argument array usage to eliminate shell interpolation
and ensure bounded execution.
---
Nitpick comments:
In `@test/e2e/electrum.test.ts`:
- Around line 17-29: The tests use fixed delays (FAUCET_PROPAGATION_MS, direct
setTimeout via delay, and hardcoded waits) which should be replaced with the
project's waitFor polling helper from test/e2e/utils.ts; update the tests to
remove uses of FAUCET_PROPAGATION_MS and delay and instead call waitFor polling
the real conditions (e.g., coin availability, wallet balance, mempool/tx
visibility) with a short interval until the expected state is observed, and
update any helper usages (faucet function can keep returning satoshis) so
callers use waitFor to await propagation rather than sleeping.
🪄 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: 85c2ed04-d2db-4178-a127-1edde04baaea
📒 Files selected for processing (2)
regtesttest/e2e/electrum.test.ts
| import { execSync } from "child_process"; | ||
| import { ElectrumWS } from "ws-electrumx-client"; | ||
| import { Address, OutScript } from "@scure/btc-signer"; | ||
| import { | ||
| ElectrumOnchainProvider, | ||
| OnchainWallet, | ||
| SingleKey, | ||
| WsElectrumChainSource, | ||
| } from "../../src"; | ||
| import { networks } from "../../src/networks"; | ||
|
|
||
| // nigiri's electrum-ws bridge (from vulpemventures/nigiri#258) exposes the | ||
| // existing electrs TCP endpoint as a WebSocket on this port. | ||
| const ELECTRUM_WS_URL = "ws://localhost:50003"; | ||
|
|
||
| // Faucets propagate via electrs's mempool subscription within ~1 polling | ||
| // interval; 5 seconds is the conservative upper bound used elsewhere in | ||
| // the e2e suite. | ||
| const FAUCET_PROPAGATION_MS = 5_000; | ||
|
|
||
| function faucet(address: string, btc = 0.001): number { | ||
| execSync(`nigiri faucet ${address} ${btc}`); | ||
| return Math.round(btc * 100_000_000); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify sync child_process usage patterns in e2e tests.
rg -n --type=ts -C3 '\b(execSync|execFileSync)\s*\(' test/e2eRepository: arkade-os/ts-sdk
Length of output: 3725
Avoid shell interpolation and unbounded sync process execution in faucet() and similar e2e helpers.
The faucet() function at line 22–25 uses execSync with shell interpolation and no timeout. This pattern appears across multiple e2e tests (vhtlc.test.ts, onchain.test.ts) and creates CI hang risk. Use execFileSync with argument arrays and a timeout:
Safer helper patch
-import { execSync } from "child_process";
+import { execFileSync } from "child_process";
...
function faucet(address: string, btc = 0.001): number {
- execSync(`nigiri faucet ${address} ${btc}`);
+ execFileSync("nigiri", ["faucet", address, String(btc)], {
+ timeout: 15_000,
+ stdio: "pipe",
+ });
return Math.round(btc * 100_000_000);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import { execSync } from "child_process"; | |
| import { ElectrumWS } from "ws-electrumx-client"; | |
| import { Address, OutScript } from "@scure/btc-signer"; | |
| import { | |
| ElectrumOnchainProvider, | |
| OnchainWallet, | |
| SingleKey, | |
| WsElectrumChainSource, | |
| } from "../../src"; | |
| import { networks } from "../../src/networks"; | |
| // nigiri's electrum-ws bridge (from vulpemventures/nigiri#258) exposes the | |
| // existing electrs TCP endpoint as a WebSocket on this port. | |
| const ELECTRUM_WS_URL = "ws://localhost:50003"; | |
| // Faucets propagate via electrs's mempool subscription within ~1 polling | |
| // interval; 5 seconds is the conservative upper bound used elsewhere in | |
| // the e2e suite. | |
| const FAUCET_PROPAGATION_MS = 5_000; | |
| function faucet(address: string, btc = 0.001): number { | |
| execSync(`nigiri faucet ${address} ${btc}`); | |
| return Math.round(btc * 100_000_000); | |
| } | |
| import { execFileSync } from "child_process"; | |
| import { ElectrumWS } from "ws-electrumx-client"; | |
| import { Address, OutScript } from "@scure/btc-signer"; | |
| import { | |
| ElectrumOnchainProvider, | |
| OnchainWallet, | |
| SingleKey, | |
| WsElectrumChainSource, | |
| } from "../../src"; | |
| import { networks } from "../../src/networks"; | |
| // nigiri's electrum-ws bridge (from vulpemventures/nigiri#258) exposes the | |
| // existing electrs TCP endpoint as a WebSocket on this port. | |
| const ELECTRUM_WS_URL = "ws://localhost:50003"; | |
| // Faucets propagate via electrs's mempool subscription within ~1 polling | |
| // interval; 5 seconds is the conservative upper bound used elsewhere in | |
| // the e2e suite. | |
| const FAUCET_PROPAGATION_MS = 5_000; | |
| function faucet(address: string, btc = 0.001): number { | |
| execFileSync("nigiri", ["faucet", address, String(btc)], { | |
| timeout: 15_000, | |
| stdio: "pipe", | |
| }); | |
| return Math.round(btc * 100_000_000); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/e2e/electrum.test.ts` around lines 2 - 25, The faucet() helper currently
uses execSync with shell-interpolated arguments and no timeout, which can hang
CI; replace it with child_process.execFileSync using the command "nigiri" and an
args array [ "faucet", address, btc.toString() ] (or format btc as needed), add
a timeout option (e.g. 30_000 ms) and stdio options as appropriate, and
catch/propagate errors so failures surface instead of hanging; update any other
e2e helpers (e.g. in vhtlc.test.ts, onchain.test.ts) that call execSync to the
same execFileSync pattern and argument array usage to eliminate shell
interpolation and ensure bounded execution.
| const status = await provider.getTxStatus(coins[0].txid); | ||
| if (status.confirmed) { | ||
| expect(status.blockHeight).toBeGreaterThan(0); | ||
| expect(status.blockTime).toBeGreaterThan(0); | ||
| } else { | ||
| expect(status).toEqual({ confirmed: false }); | ||
| } |
There was a problem hiding this comment.
Use partial matching for unconfirmed status shape.
Line 131 uses strict equality on the whole object, which is brittle if getTxStatus later adds non-breaking fields. Assert only required fields.
✅ Minimal assertion hardening
- } else {
- expect(status).toEqual({ confirmed: false });
- }
+ } else {
+ expect(status).toMatchObject({ confirmed: false });
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const status = await provider.getTxStatus(coins[0].txid); | |
| if (status.confirmed) { | |
| expect(status.blockHeight).toBeGreaterThan(0); | |
| expect(status.blockTime).toBeGreaterThan(0); | |
| } else { | |
| expect(status).toEqual({ confirmed: false }); | |
| } | |
| const status = await provider.getTxStatus(coins[0].txid); | |
| if (status.confirmed) { | |
| expect(status.blockHeight).toBeGreaterThan(0); | |
| expect(status.blockTime).toBeGreaterThan(0); | |
| } else { | |
| expect(status).toMatchObject({ confirmed: false }); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/e2e/electrum.test.ts` around lines 126 - 132, The test currently asserts
exact equality for an unconfirmed result from provider.getTxStatus (variable
status), which is brittle; change the assertion to a partial match so only
required fields are checked (e.g., assert that status.confirmed is false using
toMatchObject or expect.objectContaining) while leaving other possible fields
untouched; update the branch that handles !status.confirmed in electrum.test.ts
to use a partial matcher against status instead of strict equality.
ghost
left a comment
ghost
left a comment
There was a problem hiding this comment.
🤖 Reviewed by Arkana (incremental — new commit e6769af)
Focused on the second commit: fix(electrum): drop verbose-tx dependency for electrs compatibility. This is a protocol-relevant change — it alters how transaction confirmation status, block heights, block times, and output amounts are derived for every Electrum-backed call path.
Overall: well-motivated, well-executed. Eliminating the Fulcrum-only verbose=true dependency makes the provider work with electrs (mempool.space, blockstream.info, nigiri), and switching to raw-tx parsing removes the Math.round(value * 1e8) float footgun entirely. The unit test coverage is thorough and specifically asserts no verbose-tx calls leak through.
⚠️ Issue — fetchTxMerkle blanket catch masks transient failures
src/providers/electrum.ts — fetchTxMerkle (new method, ~line 214–235 in diff)
The catch block swallows all errors and returns null (= "unconfirmed"):
} catch {
return null;
}electrs raises a specific error for mempool txs, but this also catches WebSocket disconnects, timeouts, and server errors. Downstream in unroll.ts:253, getTxStatus returning {confirmed: false} for a confirmed tx throws "tx ${txid} is not confirmed" and blocks the VTXO exit path until a retry succeeds.
This errs on the safe side (false unconfirmed, never false confirmed), so it won't lose money — but it can stall exit processing on flaky connections. Consider narrowing the catch to error messages matching the electrs pattern (e.g. /not yet in a block|is not yet in the blockchain/i) and re-throwing unexpected errors so callers can distinguish "mempool" from "server down".
Severity: medium — no money risk, but could cause user-visible stalls in exit flows.
ℹ️ Observation — buildExplorerTx silently drops vout on parse failure
src/providers/electrum.ts — buildExplorerTx (~line 700–730 in diff)
If rawHex is undefined (fetchTransactions returned less than expected) or unparseable, vout becomes []. wallet.ts:546 calls getTransactions to find boarding transactions by matching output addresses/amounts — an empty vout means the boarding tx is invisible. This matches the old parseExactSats fallback behavior (it returned null, then the verbose-tx vout was used), but now there's no verbose fallback.
In practice fetchTransactions will throw before returning a missing entry (the retry loop in fetchTransactions covers this), so this is defensive-only. Just noting it for awareness.
✅ What looks good
-
Money handling correctness:
(output.amount ?? 0n).toString()from raw tx bytes is exact. The oldMath.round(v.value * 1e8)float path is fully eliminated. This is the right direction for a protocol-level SDK. -
Block header de-duplication:
historyToExplorerTxscollapses N history entries into K unique height lookups. Good optimization for addresses with many txs at the same height. -
watchAddresses consistency: The subscribe path now shares
historyToExplorerTxswith the poll path, so subscribe-driven and poll-driven callers see identicalExplorerTransactionshapes. Previously they could diverge (verbose vs non-verbose). -
getTxStatus height derivation: The old
tipHeight - confirmations + 1was racy — a new block between the verbose-tx call andsubscribeHeaders()would yield a wrong height.get_merkle.block_heightis stable. -
Unit test quality: Tests assert no verbose-tx calls leak, block-header batching is de-duped, and height=0 is treated as mempool. The method-level assertions (
expect(methods).not.toContain(...)) are a good pattern for enforcing protocol constraints. -
Dead code left intentionally:
fetchVerboseTransaction(s)remain public onWsElectrumChainSourcefor Fulcrum-targeting callers. Cross-repo search confirms no external consumers — this is safe.
Previous review items (commit 1 — e2e tests)
My earlier feedback on test/e2e/electrum.test.ts still applies (defensive coins.length assertion before indexing, execFileSync over execSync for shell-safety, narrowing getTxStatus assertions). Those are test quality issues, not blockers.
Verdict: No blocking issues. The blanket catch in fetchTxMerkle is worth tightening before this goes to production, but it fails safe. Ship it. 👍
ghost
left a comment
ghost
left a comment
There was a problem hiding this comment.
🤖 Reviewed by Arkana (incremental — new commit 010c387)
Commit: chore(regtest): point submodule back at arkade-regtest master
Submodule-only change. regtest pointer moved from f13b9a26 → 8c27e31b, which is the current HEAD of ArkLabsHQ/arkade-regtest master. This reverts the temporary NIGIRI_BRANCH=electrum-ws-bridge pin now that vulpemventures/nigiri#258 has merged upstream.
Verified:
- ✅ No source code or test changes in this commit
- ✅ Submodule commit
8c27e31bexists on arkade-regtest master (dated 2026-04-24) - ✅ Commit message accurately describes the change
No new issues. My previous feedback on the blanket catch in fetchTxMerkle still stands as a nice-to-have improvement but is not blocking.
Approving. All three commits reviewed across iterations — good to merge once CI is green and a human signs off.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/providers/electrum.ts`:
- Around line 215-234: In fetchTxMerkle, don't blanket-catch all errors from
this.ws.request (GetTransactionMerkleMethod); instead, only swallow the specific
Electrum “not yet in a block” response and rethrow everything else. Update the
catch to inspect the thrown error from this.ws.request (in fetchTxMerkle) and if
its message/code matches the known "transaction not in a block" pattern returned
by electrum/electrs, return null, otherwise rethrow the original error so
backend outages/auth failures/malformed responses surface to callers.
- Around line 863-876: The code marks new txids as seen (using known.add and
knownTxids.set) before converting and delivering them, so failures in
historyToExplorerTxs or eventCallback permanently suppress the txids; change the
order to first call await this.historyToExplorerTxs(newEntries) and then call
eventCallback(explorerTxs), and only after both succeed add each entry.tx_hash
to the known Set and update knownTxids.set(scripthash, known); alternatively, if
you prefer to keep the current order, wrap historyToExplorerTxs and
eventCallback in try/catch and on any failure remove the previously added txids
from known (via known.delete(entry.tx_hash)) before rethrowing/logging so
supervision remains accurate.
- Around line 725-757: The code in buildExplorerTx is currently swallowing parse
errors and returning an empty vout, which hides real deposits; change it so
decode/parse failures are propagated instead of suppressed: inside
buildExplorerTx (the rawHex handling that calls hex.decode and
Transaction.fromRaw and fills vout), remove the empty catch block and rethrow
the original error (or throw a new error that includes the entry.txid and the
original error) so callers can detect and retry/fail-fast; keep the current
fallback-only behavior when rawHex is undefined, but do not convert parse errors
into an empty vout.
🪄 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: 38057734-9d68-4065-849a-9bc171e3eee5
📒 Files selected for processing (3)
regtestsrc/providers/electrum.tstest/electrum.test.ts
✅ Files skipped from review due to trivial changes (1)
- regtest
Addresses three CodeRabbit findings on the verbose-removal refactor and adopts the project's e2e polling helper. (1) fetchTxMerkle: scope catch to "not in block" errors only The previous blanket catch silently mapped every failure — including auth errors, network outages, and malformed responses — to "mempool / unconfirmed", freezing any caller polling getTxStatus. Now matches electrs/Fulcrum's specific "not yet in a block" wording and rethrows everything else. (2) watchAddresses: dedupe AFTER successful delivery, not before Previously a failed historyToExplorerTxs call or a throwing user callback would still mark the txid as seen, permanently suppressing re-delivery on the next notification. Moves the known-set update to after the eventCallback resolves. (3) buildExplorerTx: propagate parse errors instead of empty vout Returning an empty vout on a parse failure silently hides outputs — including incoming deposits. For protocol-level money handling, fail loud with the offending txid in the error message and let the caller decide whether to retry or skip. (4) e2e tests: replace fixed delays with waitFor polling Aligns test/e2e/electrum.test.ts with the established pattern from settlement.test.ts / ark.test.ts (waitFor in test/e2e/utils.ts) so under CI load the tests finish as soon as electrs sees the tx rather than waiting out a fixed 5-10s budget. Reduces flakiness. Tests: - 3 new unit tests cover the three production fixes: - getTxStatus propagates non-"not in block" errors - watchAddresses re-delivers when first delivery throws - getTransactions propagates parse errors with the offending txid - Total electrum unit tests: 45 (was 42); full suite: 1020 passing - Lint clean
|
Pushed CodeRabbit findings — all addressed1. 2. 3. Nitpick — also addressed4. e2e tests use fixed delays instead of arkanaai feedbackAll three minor notes were either already addressed or non-blocking:
Local verification
|
ghost
left a comment
ghost
left a comment
There was a problem hiding this comment.
🤖 Reviewed by Arkana (incremental — new commit 88ec262)
All three issues flagged in my previous review are properly addressed:
1. fetchTxMerkle — selective error handling ✅
isTxNotInBlockError() (line 957-967) now catches only the known electrs/Fulcrum "not in block" error strings. Auth failures, network outages, and malformed responses propagate to callers — no more silent "treat as mempool" for real errors. The string matching is conservative and well-documented.
2. buildExplorerTx — parse errors propagate ✅
Line 748-751: malformed raw hex now throws with the offending txid in the message instead of silently producing an empty vout. Correct call — hiding outputs is far worse than failing the batch in a money-handling context.
3. watchAddresses dedupe — mark-after-deliver ✅
Lines 883-886: known.add() and knownTxids.set() now execute after historyToExplorerTxs + eventCallback succeed. If either throws, txids stay unseen and the next status notification will re-attempt delivery. The handleStatusChange chain (line 889-898) correctly swallows the rejection via .catch(() => undefined) to keep the serialization chain alive.
Tests
All three fixes have corresponding unit tests:
propagates non-'not in block' errors from get_merkle— verifies auth errors surfacepropagates parse errors with the offending txid— verifies gibberish hex fails loudre-delivers a tx on the next notification when delivery fails the first time— regression test for the dedupe ordering bug
E2e tests migrated from fixed delay() to waitFor() polling — better CI reliability.
LGTM. Ship it.
ghost
left a comment
ghost
left a comment
There was a problem hiding this comment.
🤖 Reviewed by Arkana (incremental — new commit 88ec262)
All three issues flagged in my previous review are properly addressed:
1. fetchTxMerkle — selective error handling ✅
isTxNotInBlockError() (line 957-967) now catches only the known electrs/Fulcrum "not in block" error strings. Auth failures, network outages, and malformed responses propagate to callers — no more silent "treat as mempool" for real errors. The string matching is conservative and well-documented.
2. buildExplorerTx — parse errors propagate ✅
Line 748-751: malformed raw hex now throws with the offending txid in the message instead of silently producing an empty vout. Correct call — hiding outputs is far worse than failing the batch in a money-handling context.
3. watchAddresses dedupe — mark-after-deliver ✅
Lines 883-886: known.add() and knownTxids.set() now execute after historyToExplorerTxs + eventCallback succeed. If either throws, txids stay unseen and the next status notification will re-attempt delivery. The handleStatusChange chain (line 889-898) correctly swallows the rejection via .catch(() => undefined) to keep the serialization chain alive.
Tests
All three fixes have corresponding unit tests:
propagates non-'not in block' errors from get_merkle— verifies auth errors surfacepropagates parse errors with the offending txid— verifies gibberish hex fails loudre-delivers a tx on the next notification when delivery fails the first time— regression test for the dedupe ordering bug
E2e tests migrated from fixed delay() to waitFor() polling — better CI reliability.
LGTM. Ship it.
|
Pushed What brokeBoth e2e failures traced to the same electrs race: a tx can appear in The previous (verbose-tx) path tolerated this by falling back to Two narrow fixesProduction: If the batch This is the opposite shape from the parse-error finding addressed in the previous commit (which kept E2E: wrap
New regression testAdded Local verification
|
ghost
left a comment
ghost
left a comment
There was a problem hiding this comment.
🤖 Reviewed by Arkana (incremental — new commit cb1ebe89)
Scope: Tolerates electrs block-header index lag in historyToExplorerTxs.
Changes reviewed
-
src/providers/electrum.ts— batch→per-height header fallback (lines 714–738)- Batch
fetchBlockHeadersis tried first (fast path). On rejection, falls back toPromise.allSettledover individualfetchBlockHeader(h)calls. Heights whose headers aren't yet indexable silently degrade toblock_time = 0via the existingblockTimeByHeight.get(entry.height) ?? 0inbuildExplorerTx(line 794). - ✅ Correct: the fallback only triggers when the batch rejects wholesale (one poisoned height), so the common case stays a single round-trip.
- ✅
block_time = 0is the right degraded semantic — matches what the old verbose-tx path did (vtx.blocktime || vtx.time || 0). Confirmation status and amounts remain authoritative; only the timestamp degrades. - ✅ Parse errors in
buildExplorerTxstill throw (previous commit's fix preserved). No silent data loss.
- Batch
-
test/electrum.test.ts— new unit test (line 531)- Mocks the batch rejection → individual fallback → partial success path. Asserts
block_time = 0for the lagging height while the good height gets its real timestamp. Adequate coverage.
- Mocks the batch rejection → individual fallback → partial success path. Asserts
-
test/e2e/electrum.test.ts—waitForaroundgetTxStatus- Swallows only
missingheight/not in blockpatterns; rethrows everything else. Correct — won't mask real failures. fetchHistoriestimeout bumped to 40s and assertion relaxed to>= 1coin. Reasonable under CI load.
- Swallows only
Verdict
No issues. The fallback is narrowly scoped (only header-fetch failures, not tx or amount data), the degradation is safe (block_time is informational, not protocol-critical), and both unit and e2e coverage are adequate.
The full PR across all 5 commits is solid. The verbose-tx removal is a meaningful improvement — eliminates Fulcrum dependency for the core provider path, kills the Math.round(value * 1e8) float footgun, and opens up electrs-based server support. All previous feedback has been addressed.
LGTM — ready for human sign-off.
ghost
left a comment
ghost
left a comment
There was a problem hiding this comment.
🤖 Reviewed by Arkana (incremental — new commit cb1ebe89)
Scope: Tolerates electrs block-header index lag in historyToExplorerTxs.
Changes reviewed
-
src/providers/electrum.ts— batch→per-height header fallback (lines 714–738)- Batch
fetchBlockHeadersis tried first (fast path). On rejection, falls back toPromise.allSettledover individualfetchBlockHeader(h)calls. Heights whose headers aren't yet indexable silently degrade toblock_time = 0via the existingblockTimeByHeight.get(entry.height) ?? 0inbuildExplorerTx(line 794). - ✅ Correct: the fallback only triggers when the batch rejects wholesale (one poisoned height), so the common case stays a single round-trip.
- ✅
block_time = 0is the right degraded semantic — matches what the old verbose-tx path did (vtx.blocktime || vtx.time || 0). Confirmation status and amounts remain authoritative; only the timestamp degrades. - ✅ Parse errors in
buildExplorerTxstill throw (previous commit's fix preserved). No silent data loss.
- Batch
-
test/electrum.test.ts— new unit test (line 531)- Mocks the batch rejection → individual fallback → partial success path. Asserts
block_time = 0for the lagging height while the good height gets its real timestamp. Adequate coverage.
- Mocks the batch rejection → individual fallback → partial success path. Asserts
-
test/e2e/electrum.test.ts—waitForaroundgetTxStatus- Swallows only
missingheight/not in blockpatterns; rethrows everything else. Correct — won't mask real failures. fetchHistoriestimeout bumped to 40s and assertion relaxed to>= 1coin. Reasonable under CI load.
- Swallows only
Verdict
No issues. The fallback is narrowly scoped (only header-fetch failures, not tx or amount data), the degradation is safe (block_time is informational, not protocol-critical), and both unit and e2e coverage are adequate.
The full PR across all 5 commits is solid. The verbose-tx removal is a meaningful improvement — eliminates Fulcrum dependency for the core provider path, kills the Math.round(value * 1e8) float footgun, and opens up electrs-based server support. All previous feedback has been addressed.
LGTM — ready for human sign-off.
|
Pushed The previous fix made
E2e test simplifies back to a direct call — no more waitFor wrapper needed. Two new unit tests
Local verification
|
ghost
left a comment
ghost
left a comment
There was a problem hiding this comment.
🤖 Reviewed by Arkana (incremental — new commits 1050558, b92abc1)
Two new commits since last review (cb1ebe89). Both address the electrs index-lag race in getTxStatus and stabilize the e2e test harness.
Changes reviewed
src/providers/electrum.ts — getTxStatus now wraps the fetchBlockHeader call in try/catch, tolerating missingheight errors (returns blockTime: 0 instead of throwing). New isMissingHeightError helper correctly discriminates index-lag from real failures (network, auth).
✅ Correct. Matches the degradation semantic already established in historyToExplorerTxs (bare catch → Promise.allSettled → missing heights get block_time: 0). The new code is actually stricter — it only swallows missingheight, propagating everything else. No protocol-safety concern: blockTime is metadata, not used in VTXO validation or signing paths.
test/e2e/electrum.test.ts — faucet() now mines a block immediately after funding. Eliminates the retry-loop workaround in the getTxStatus test.
✅ Good fix. Mining after faucet is what other e2e suites do (settlement.test.ts etc.). Removes test flakiness at the source.
test/electrum.test.ts — Two new unit tests: (1) missingheight tolerance returns {confirmed: true, blockHeight: 200, blockTime: 0}, (2) non-missingheight errors propagate.
✅ Good coverage. Both the happy-degraded and fail-loud paths are tested.
Minor observation (not blocking)
historyToExplorerTxs (line ~716) uses a bare catch {} that swallows all fetchBlockHeaders errors before falling back to Promise.allSettled. Now that isMissingHeightError exists, a future cleanup could tighten that catch to only tolerate the expected race. Not in scope for this PR.
Verdict: LGTM. No protocol-critical changes, no security concerns, good test coverage for the new path.
ghost
left a comment
ghost
left a comment
There was a problem hiding this comment.
🤖 Reviewed by Arkana (incremental — new commits 1050558, b92abc1)
Two new commits since last review (cb1ebe89). Both address the electrs index-lag race in getTxStatus and stabilize the e2e test harness.
Changes reviewed
src/providers/electrum.ts — getTxStatus now wraps the fetchBlockHeader call in try/catch, tolerating missingheight errors (returns blockTime: 0 instead of throwing). New isMissingHeightError helper correctly discriminates index-lag from real failures (network, auth).
✅ Correct. Matches the degradation semantic already established in historyToExplorerTxs (bare catch → Promise.allSettled → missing heights get block_time: 0). The new code is actually stricter — it only swallows missingheight, propagating everything else. No protocol-safety concern: blockTime is metadata, not used in VTXO validation or signing paths.
test/e2e/electrum.test.ts — faucet() now mines a block immediately after funding. Eliminates the retry-loop workaround in the getTxStatus test.
✅ Good fix. Mining after faucet is what other e2e suites do (settlement.test.ts etc.). Removes test flakiness at the source.
test/electrum.test.ts — Two new unit tests: (1) missingheight tolerance returns {confirmed: true, blockHeight: 200, blockTime: 0}, (2) non-missingheight errors propagate.
✅ Good coverage. Both the happy-degraded and fail-loud paths are tested.
Minor observation (not blocking)
historyToExplorerTxs (line ~716) uses a bare catch {} that swallows all fetchBlockHeaders errors before falling back to Promise.allSettled. Now that isMissingHeightError exists, a future cleanup could tighten that catch to only tolerate the expected race. Not in scope for this PR.
Verdict: LGTM. No protocol-critical changes, no security concerns, good test coverage for the new path.
|
Pushed The library ( This explains why the previous fixes all failed: my application-level try/catch around Switched Trade-offN parallel
For typical history sizes (<20 unique heights) the cost is negligible. Test changes
|
ghost
left a comment
ghost
left a comment
There was a problem hiding this comment.
🤖 Reviewed by Arkana (incremental — new commit f209bcc8)
Scope: Eliminates unhandled-rejection leak from library batchRequest for block headers.
Changes reviewed
-
src/providers/electrum.ts:694–738— Replaces the batch-then-fallback header fetch with a directPromise.allSettledover per-heightfetchBlockHeadercalls. Correct fix: the library'sbatchRequestusesPromise.allinternally, so a single rejected element orphans the remaining in-flight promises, whose later rejections go unhandled.allSettledgives every promise a handler. Degradation behavior (missing height →block_time = 0) is preserved. -
test/electrum.test.ts— Mocks updated frombatchRequesttorequestfor header calls. New test ("never uses the library batchRequest for headers") explicitly guards against regression. Existing race-tolerance test renamed and simplified to match the new single-path logic.
Verified
- No other call site in the provider uses
fetchBlockHeaders(batch) — only the now-removed path did. getTxStatusalready uses singlefetchBlockHeader+ try/catch, unaffected.fetchBlockHeadersremains public onWsElectrumChainSourcefor external callers targeting Fulcrum — no API break.- Trade-off (N requests vs 1 batch) is acceptable: heights are de-duplicated and the set is typically small.
No protocol-critical concerns — change only affects block_time metadata, not coin amounts, VTXO state, or signing paths.
LGTM ✅
|
Pushed Previous attempt only patched What changedAdded Replaced all six call sites:
The library's Trade-offSequential per-request framing instead of one batched JSON-RPC frame. Wall-clock identical (parallel sends), wire overhead slightly higher on the request side. For typical sizes (<20 elements) the overhead is negligible. Tests
|
ghost
left a comment
ghost
left a comment
There was a problem hiding this comment.
🤖 Reviewed by Arkana (incremental — new commit 54ef4d47)
Scope: Promotes safeBatchRequest as a codebase-wide replacement for the library's leaky ws.batchRequest, applied to all six remaining call sites.
Changes reviewed
-
src/providers/electrum.ts:178–195—safeBatchRequest<T>: Correct implementation.Promise.allSettledensures every promise has a handler (no orphan rejections), then surfaces the first failure. Empty-input short-circuit present. The cast(r as PromiseFulfilledResult<T>).valueis safe because we already threw on any rejected result. -
Six call-site migrations (
fetchTransactions,fetchVerboseTransactions,fetchHistories,fetchBlockHeaders,getTxOutspends×2): All correctly switch fromws.batchRequest<T[]>(...requests)tothis.safeBatchRequest<T>(requests)/this.chain.safeBatchRequest<T>(requests). Generic type parameters adjusted to element type (e.g.stringnotstring[]). No semantic change. -
historyToExplorerTxsfallback (lines ~753–776): Try batch first; on failure, fallback to per-heightfetchBlockHeaderunderPromise.allSettled. One minor inefficiency: fulfilled results from the failed batch attempt are discarded and all heights re-fetched individually. Acceptable trade-off — the set is typically <20 heights, and the fallback only triggers during electrs index lag (rare). -
test/electrum.test.ts:mockBatchhelper correctly queues N sequentialmockResolvedValueOnceresponses to matchsafeBatchRequest's per-elementws.requestcalls. 25 mock sites updated. Regression test now assertsbatchRequestis never called (was "called exactly once"). Header-failure test updated to account for the two-tier attempt (batch then fallback).
Verified
grepconfirms zero remainingws.batchRequestcalls in production code (only appears in JSDoc comments).safeBatchRequestis public onWsElectrumChainSource, accessible fromElectrumOnchainProviderviathis.chain— no access-level issues.- No protocol-critical concerns — change is purely about request lifecycle management and error handling for metadata (block_time). No VTXO, signing, forfeit, or exit path code is touched.
LGTM ✅
ghost
left a comment
ghost
left a comment
There was a problem hiding this comment.
🤖 Reviewed by Arkana (incremental — new commit 54ef4d47)
Scope: Promotes safeBatchRequest as a codebase-wide replacement for the library's leaky ws.batchRequest, applied to all six remaining call sites.
Changes reviewed
-
src/providers/electrum.ts:178–195—safeBatchRequest<T>: Correct implementation.Promise.allSettledensures every promise has a handler (no orphan rejections), then surfaces the first failure. Empty-input short-circuit present. The cast(r as PromiseFulfilledResult<T>).valueis safe because we already threw on any rejected result. -
Six call-site migrations (
fetchTransactions,fetchVerboseTransactions,fetchHistories,fetchBlockHeaders,getTxOutspends×2): All correctly switch fromws.batchRequest<T[]>(...requests)tothis.safeBatchRequest<T>(requests)/this.chain.safeBatchRequest<T>(requests). Generic type parameters adjusted to element type (e.g.stringnotstring[]). No semantic change. -
historyToExplorerTxsfallback (lines ~753–776): Try batch first; on failure, fallback to per-heightfetchBlockHeaderunderPromise.allSettled. One minor inefficiency: fulfilled results from the failed batch attempt are discarded and all heights re-fetched individually. Acceptable trade-off — the set is typically <20 heights, and the fallback only triggers during electrs index lag (rare). -
test/electrum.test.ts:mockBatchhelper correctly queues N sequentialmockResolvedValueOnceresponses to matchsafeBatchRequest's per-elementws.requestcalls. 25 mock sites updated. Regression test now assertsbatchRequestis never called (was "called exactly once"). Header-failure test updated to account for the two-tier attempt (batch then fallback).
Verified
grepconfirms zero remainingws.batchRequestcalls in production code (only appears in JSDoc comments).safeBatchRequestis public onWsElectrumChainSource, accessible fromElectrumOnchainProviderviathis.chain— no access-level issues.- No protocol-critical concerns — change is purely about request lifecycle management and error handling for metadata (block_time). No VTXO, signing, forfeit, or exit path code is touched.
LGTM ✅
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/providers/electrum.ts (1)
897-955:⚠️ Potential issue | 🟠 MajorAwait async
eventCallbacks before marking txids as seen.The code comment (lines 948-951) explicitly states the dedupe set should be updated only after delivery succeeds, but line 953 calls
eventCallback(explorerTxs)without awaiting. Since the callback parameter type is(txs: ExplorerTransaction[]) => void, TypeScript callers can pass anasyncfunction; if it returns a rejected promise, lines 954-955 still mark the txid as delivered before the rejection is discovered. This violates the stated invariant and reintroduces lost-redelivery bugs for async consumers.Suggested fix
- eventCallback: (txs: ExplorerTransaction[]) => void + eventCallback: ( + txs: ExplorerTransaction[] + ) => void | Promise<void> @@ - eventCallback(explorerTxs); + await Promise.resolve(eventCallback(explorerTxs)); for (const entry of newEntries) known.add(entry.tx_hash); knownTxids.set(scripthash, known);Also mirror the widened callback type in the
OnchainProviderinterface declaration and add a regression test where the callback returns a rejectedPromise.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/providers/electrum.ts` around lines 897 - 955, The processStatusChange in watchAddresses calls eventCallback(explorerTxs) without awaiting, so async callbacks can reject yet txids are marked as seen; change to await the callback and only update knownTxids after it resolves. Specifically, update watchAddresses → processStatusChange to: await eventCallback(explorerTxs) (wrap in try/catch as appropriate so you don't mark txids on rejection), keep the existing behavior of swallowing/failing the inFlight chain per existing comment but do NOT update known.add(...) unless the await succeeded, and ensure inFlight continues to chain even on callback errors. Also widen the callback type on the OnchainProvider interface (and any usages) from (txs: ExplorerTransaction[]) => void to accept Promise-returning handlers ((txs: ExplorerTransaction[]) => void | Promise<void> or Promise<void>) and add a regression test where the subscriber callback is async and rejects to confirm txids are not marked and the notification is retried.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/providers/electrum.ts`:
- Around line 897-955: The processStatusChange in watchAddresses calls
eventCallback(explorerTxs) without awaiting, so async callbacks can reject yet
txids are marked as seen; change to await the callback and only update
knownTxids after it resolves. Specifically, update watchAddresses →
processStatusChange to: await eventCallback(explorerTxs) (wrap in try/catch as
appropriate so you don't mark txids on rejection), keep the existing behavior of
swallowing/failing the inFlight chain per existing comment but do NOT update
known.add(...) unless the await succeeded, and ensure inFlight continues to
chain even on callback errors. Also widen the callback type on the
OnchainProvider interface (and any usages) from (txs: ExplorerTransaction[]) =>
void to accept Promise-returning handlers ((txs: ExplorerTransaction[]) => void
| Promise<void> or Promise<void>) and add a regression test where the subscriber
callback is async and rejects to confirm txids are not marked and the
notification is retried.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 40c59bfd-fd77-4551-8131-7bab14426c8e
📒 Files selected for processing (2)
src/providers/electrum.tstest/electrum.test.ts
ghost
left a comment
ghost
left a comment
There was a problem hiding this comment.
🤖 Reviewed by Arkana (incremental — new commits 230511a, e66fd6d, dfe7d57)
Three new commits since last review. All look good.
230511a — Default URLs to Ark Labs–operated mempool/electrum
ESPLORA_URLdefaults switched frommempool.space/mutinynet.comtomempool.arkade.shetc.- New
ELECTRUM_WS_URLandELECTRUM_TCP_HOSTmaps exported fromsrc/providers/electrum.tsand re-exported fromsrc/index.ts. - Cross-repo impact: checked all repos — no downstream consumer imports or hardcodes the old URLs.
bancotest code uses hardcodedlocalhost:3000. Server-side repos useARKD_ESPLORA_URLenv var (unrelated). No breakage. - Types are correct (
Record<NetworkName, string>/Record<NetworkName, string | null>). - Comment about testnet defaulting to Blockstream (Ark doesn't host it) is accurate and helpful.
✅ Clean.
e66fd6d — Treat missingheight from get_merkle as unconfirmed
fetchTxMerklecatch block now acceptsisMissingHeightError(err)in addition toisTxNotInBlockError(err), returningnull(unconfirmed/mempool).- This is correct —
get_merklecan hit the same electrs index-lag race asblock.header. Mapping it to "not yet confirmed" lets callers poll again. Genuine errors (auth, network) still throw. - New unit test at
test/electrum.test.ts:439covers this path specifically.
✅ Clean.
dfe7d57 — Drive fetchHistories test via the helper itself
- Test-only change.
waitForloop now pollschain.fetchHistories()directly instead ofPromise.all([getCoins(a), getCoins(b)]). - Eliminates CI flake from concurrent WS pressure. Sound approach.
✅ Clean.
CI: build ✅ lint ✅ unit ✅ integration ⏳ (pending — needs regtest infra)
No protocol-critical changes in this batch (no VTXO, signing, forfeit, or exit path changes). The URL defaults and error-tolerance improvements are safe. Approving.
ghost
left a comment
ghost
left a comment
There was a problem hiding this comment.
🤖 Reviewed by Arkana (incremental — new commit 9b7cddd1)
One new commit since last review. Test-only change.
9b7cddd1 — retry fetchHistories on transient WS request timeout
test/e2e/electrum.test.ts:289-303:waitForloop now wrapsfetchHistoriesin try/catch, swallowingrequest timeoutandmissingheighterrors (returnsfalse→ retry) while re-throwing genuine errors.- Regex
/request timeout|missingheight/iis appropriately scoped — won't mask real failures (network auth, parse errors, etc.). - This is the right fix for CI flake: transient electrs index lag under load shouldn't fail the test on first attempt.
✅ Clean. No production code touched. No protocol-critical changes.
|
All green on What it tookAfter many false-starts, the actual root causes turned out to be two narrow infrastructure races, not the library batch leak I'd been chasing:
Production wins this PR delivered along the way
Final test count
|
ghost
left a comment
ghost
left a comment
There was a problem hiding this comment.
🤖 Reviewed by Arkana (incremental — new commit 63ff2023)
One new commit since last review. Docs-only — no logic changes.
63ff2023 — docs: document onchain providers + Electrum support
- README.md: New "Onchain Providers" section with provider comparison table, default URL examples, Esplora/Electrum usage snippets, 1P1C package broadcast docs, and server compatibility notes. Clean, accurate, matches the implementation.
- src/providers/electrum.ts: Expanded JSDoc on
ElectrumOnchainProvider— documents Fulcrum/electrs compatibility, non-verbose tx.get rationale, bigint-based amounts, and 1P1C broadcast semantics. Updated@exampleblocks to useELECTRUM_WS_URLand@arkade-os/sdkimports.
No runtime code touched. LGTM. ✅
|
@pietro909 can you give this a test? We now have a different default esplora instance urls. Ideally we switch to electrum instead. |
Adds test/e2e/electrum.test.ts which exercises every method on OnchainProvider against nigiri's bitcoin Electrum endpoint, exposed as a WebSocket on port 50003 by the electrum-ws bridge from vulpemventures/nigiri#258. Coverage parallels the esplora e2e tests (test/e2e/onchain.test.ts) so any divergence between providers shows up side-by-side: - getChainTip — header-subscription cache reuse across calls - getFeeRate — accepts both undefined (regtest -1) and a positive sat/vB - getCoins / getTransactions / getTxStatus — fund a fresh address via nigiri faucet and verify each method reports it - broadcastTransaction — single-tx Alice-to-Bob send roundtrip - getTxOutspends — spent-output detection on a partially-consumed tx - watchAddresses — funded-tx delivery via the scripthash subscription - WsElectrumChainSource.fetchHistories — batch round-trip The 1P1C broadcast_package path is not exercised here because nigiri's bridge wraps electrs (which lacks broadcast_package); the unit suite already covers it. Once nigiri#258 is replaced with a fulcrum-based stack, the missing cases can be added in a follow-up. Bumps the regtest submodule to ArkLabsHQ/arkade-regtest#12 which points NIGIRI_BRANCH at electrum-ws-bridge. After arkade-regtest#12 merges, re-bump the submodule pointer to its master.
The provider was calling blockchain.transaction.get with verbose=true,
which Fulcrum supports but electrs (used by Blockstream, mempool.space,
and the nigiri regtest) explicitly does not — electrs returns "verbose
transactions are currently unsupported" and the call fails.
Refactor the three call sites that needed verbose data to use only
methods every Electrum server supports:
- getTransactions / watchAddresses: already had heights from
scripthash.get_history; we now batch-fetch raw tx hex via
blockchain.transaction.get (non-verbose) and parse it for vouts and
exact sat amounts, then batch-fetch unique block headers via
blockchain.block.header to derive block_time.
- getTxStatus: switches from verbose-tx confirmations to
blockchain.transaction.get_merkle for the block height (a standard
SPV-protocol method supported by every Electrum server). Block_time
comes from parsing the corresponding block header.
This also tightens money handling — output values now come from parsing
the raw tx bytes (exact bigints), never the floating-point `value`
field. Math.round(value * 1e8) was a known footgun for protocol-level
amounts. The transition to electrum verbose-tx was previously the only
codepath that risked it; with verbose gone, it's fully eliminated.
The fetchVerboseTransaction(s) methods on WsElectrumChainSource remain
public for callers who explicitly target Fulcrum, but the provider no
longer depends on them.
42 electrum unit tests pass (replaced 5 verbose-mocked tests with their
non-verbose equivalents and added new tests asserting that:
- the provider never calls blockchain.transaction.get with verbose;
- block-header lookups are deduplicated across history entries that
share the same height;
- height=0 history entries are treated as mempool/unconfirmed.
Addresses three CodeRabbit findings on the verbose-removal refactor and adopts the project's e2e polling helper. (1) fetchTxMerkle: scope catch to "not in block" errors only The previous blanket catch silently mapped every failure — including auth errors, network outages, and malformed responses — to "mempool / unconfirmed", freezing any caller polling getTxStatus. Now matches electrs/Fulcrum's specific "not yet in a block" wording and rethrows everything else. (2) watchAddresses: dedupe AFTER successful delivery, not before Previously a failed historyToExplorerTxs call or a throwing user callback would still mark the txid as seen, permanently suppressing re-delivery on the next notification. Moves the known-set update to after the eventCallback resolves. (3) buildExplorerTx: propagate parse errors instead of empty vout Returning an empty vout on a parse failure silently hides outputs — including incoming deposits. For protocol-level money handling, fail loud with the offending txid in the error message and let the caller decide whether to retry or skip. (4) e2e tests: replace fixed delays with waitFor polling Aligns test/e2e/electrum.test.ts with the established pattern from settlement.test.ts / ark.test.ts (waitFor in test/e2e/utils.ts) so under CI load the tests finish as soon as electrs sees the tx rather than waiting out a fixed 5-10s budget. Reduces flakiness. Tests: - 3 new unit tests cover the three production fixes: - getTxStatus propagates non-"not in block" errors - watchAddresses re-delivers when first delivery throws - getTransactions propagates parse errors with the offending txid - Total electrum unit tests: 45 (was 42); full suite: 1020 passing - Lint clean
Two e2e failures on the previous commit traced to the same race in electrs: a tx can show as confirmed via listunspent (height>0) before its block header is queryable, so block.header(N) rejects with "missingheight". Two narrow fixes: (1) historyToExplorerTxs: best-effort header fetch If the batch block-header request fails wholesale (one bad height poisoning the batch), retry per-height with Promise.allSettled and collect what's available. Heights whose headers still won't resolve land in the result map with no entry, and buildExplorerTx falls back to block_time = 0 — same semantic the old verbose-tx path had via `vtx.blocktime || vtx.time || 0`. Txid + confirmation status are still authoritative, only block_time degrades. This preserves the "fail loud on real corruption" stance from the previous review (parse errors still throw with the offending txid); header indexing lag is a transient infra race, not corruption. (2) e2e: wrap getTxStatus in waitFor getTxStatus calls fetchBlockHeader directly and intentionally throws on failure (single-tx caller can retry; that's the right semantic). The e2e test now retries via waitFor, swallowing only the specific "missingheight" / "not in block" wording, until electrs settles. Also bumps the fetchHistories test timeout to 40s and relaxes its waitFor to >= 1 coin per address (the assertion only needs that). Tests: - 46 electrum unit tests (was 45, +1 for the new header-resilience path) - Full unit suite still 1020 passing, lint clean
The previous commit made historyToExplorerTxs resilient to electrs's "missingheight" index-lag race but left getTxStatus throwing on the same race. The e2e wrapper retried via waitFor, but in CI the race window outlasted waitFor's 25s budget, so the test still timed out. Apply the same policy to getTxStatus: confirmation status and height are authoritative; only block_time degrades to 0 when the header isn't yet indexable. Mirrors the old verbose-tx path's `blocktime || time || 0` fallback. Genuine errors still propagate via isMissingHeightError narrowing. E2e test no longer needs the waitFor wrapper, so it goes back to a direct call with shape assertions. Tests: - Two new unit tests for getTxStatus: - returns confirmed=true with blockTime=0 on missingheight - propagates non-missingheight errors from block.header - Total electrum unit tests: 48 (was 46)
Two e2e failures in the previous attempt traced to electrs racing on the bridge: listunspent/get_merkle would report a tx as confirmed at height N before block.header(N) was indexable, surfacing as "missingheight" or as request-timeouts on subsequent calls. The production-side fix (tolerating missingheight in historyToExplorerTxs and getTxStatus) didn't help here because the library can't always forward those errors as Promise rejections — the race manifests as unhandled errors thrown out of the WebSocket onMessage handler, where no caller-side try/catch can intercept. Mine a block immediately after each `nigiri faucet` so electrs has a stable confirmed state to index. Other e2e suites already do this (settlement.test.ts mines after every state change for the same reason). The production tolerance code stays — it's still correct for real-world use against laggy electrs servers — but the test is no longer dependent on it.
Root cause for the integration failures: ws-electrumx-client's batchRequest uses Promise.all internally — when one element rejects, the other in-flight requests stay pending. When their responses arrive later (often as more error responses), the library rejects them too, and nobody is awaiting them. Vitest catches the unhandled rejections and crashes the test run. The header-fetch path was the most exposed: a single "missingheight" from electrs's index lag would leak (N-1) more rejections. Switch historyToExplorerTxs to per-height ws.request calls under Promise.allSettled, so every promise has an explicit handler and one height's failure doesn't poison the others. Same final shape as before — heights with valid headers populate blockTimeByHeight, missing ones fall back to block_time = 0 in buildExplorerTx, mirroring the old verbose-tx code's `vtx.blocktime || vtx.time || 0`. Performance: N parallel ws.request calls vs 1 batch. allSettled fires all at once, so wall-clock cost is one round-trip-time, slightly more WebSocket frame overhead for the request side. Tests: - Updated 3 unit tests that mocked the now-removed header batch path - Added a new test asserting batchRequest is called exactly once (for raw tx hex) and never for headers — guards against the regression - 49 electrum unit tests pass (was 48)
Previous fix only addressed historyToExplorerTxs's header batch. The same orphan-rejection leak in ws-electrumx-client's batchRequest affected every other call site too: fetchTransactions, fetchVerboseTransactions, fetchHistories, fetchBlockHeaders, and the two batches inside ElectrumOnchainProvider.getTxOutspends. electrs returning "missingheight" (or any error) for one element of any batch would leak the other in-flight requests' rejections. Add WsElectrumChainSource.safeBatchRequest — a drop-in replacement that issues each request through ws.request (each gets its own request-promise lifecycle) and aggregates via Promise.allSettled (every promise has an explicit handler), then surfaces the first rejection if any. Same wall-clock cost as the library batch (parallel sends), no orphan rejections. Replace all six call sites with safeBatchRequest. The library's batchRequest is now banned in production code — it's still imported implicitly via the typings but never called. historyToExplorerTxs reverts to fetchBlockHeaders for the fast path and falls back to per-height fetchBlockHeader under Promise.allSettled when one or more heights fail (electrs index lag race) — preserving the old verbose-tx code's `blocktime || time || 0` tolerance. Tests: - New mockBatch(mock, responses) helper queues N sequential ws.request responses (mirroring how safeBatchRequest consumes them) - Updated 25 batch mock sites to use mockBatch / sequential mocks - New regression test asserts wsMock.batchRequest is never called by the provider's hot path — guards against future re-introduction - 49 electrum unit tests pass; full suite 1024 passing; lint clean
Adds reachable defaults for the SDK's onchain providers so consumers don't have to wire up every URL manually: ESPLORA_URL.bitcoin -> https://mempool.arkade.sh/api ESPLORA_URL.signet -> https://mempool.signet.arkade.sh/api ESPLORA_URL.mutinynet -> https://mempool.mutinynet.arkade.sh/api ELECTRUM_WS_URL.bitcoin -> wss://electrum.arkade.sh ELECTRUM_WS_URL.signet -> wss://electrum.signet.arkade.sh ELECTRUM_WS_URL.mutinynet -> wss://electrum.mutinynet.arkade.sh ELECTRUM_WS_URL.testnet -> wss://electrum.blockstream.info:60004 ELECTRUM_WS_URL.regtest -> ws://localhost:50003 All three Ark Labs Fulcrum 2.1.0 endpoints verified reachable; they support blockchain.transaction.broadcast_package which the provider uses for atomic 1P1C TRUC relay (see ElectrumOnchainProvider). Also adds an informational ELECTRUM_TCP_HOST map for Node-side consumers who'd build their own TCP transport — the SDK provider is WebSocket-only because it has to run in browsers, but the hostnames expose ports 50001 (TCP), 50002 (TCP+TLS), 50003 (WS). Testnet keeps mempool.space (Ark doesn't host); regtest unchanged.
The remaining e2e flake traces back here, not the batchRequest leak.
electrs can return "missingheight" from blockchain.transaction.get_merkle
in the same index-lag window where block.header(N) does — the tx is
reportedly confirmed via listunspent but the block isn't fully indexed.
Previously fetchTxMerkle only narrowed the catch to "not in a block"
wording (isTxNotInBlockError), so missingheight propagated up through
getTxStatus and surfaced as an unhandled error in the test runner.
Now also accepts isMissingHeightError as a sign of the same race —
mempool/unknown — so getTxStatus returns {confirmed: false} until
the next poll, which is the right behavior for a transient electrs
race. Genuine errors (auth/network/malformed) still propagate.
50 electrum unit tests pass (was 49); regression test asserts
get_merkle missingheight maps to confirmed=false.
Previous waitFor polled `Promise.all([getCoins(a), getCoins(b)])` while the test target was `chain.fetchHistories([aScript, bScript])`. Under CI load that occasionally tripped the library's 10s WS request timeout when two parallel listunspent calls landed during faucet propagation. Switch the wait loop to call fetchHistories directly — same code path the assertion uses, no extra concurrent listunspent calls competing for the connection. Eliminates the only remaining e2e flake.
Under CI load electrs occasionally wedges for the library's full 10s REQUEST_TIMEOUT on a single get_history call, causing the previous attempt's first poll iteration to throw and waitFor to propagate the error before any retry. Treat library-level request-timeout (and missingheight) as "not ready yet" inside the wait body; only genuine errors propagate. Eliminates the last remaining e2e flake without skipping the test.
Adds an 'Onchain Providers' section to README.md (placed before 'Receiving Bitcoin' since onchain queries underpin that flow): - Comparison table: EsploraProvider (HTTP) vs ElectrumOnchainProvider (WebSocket) — when to use each. - Default URL constants (ESPLORA_URL, ELECTRUM_WS_URL, ELECTRUM_TCP_HOST) with the network-by-network bitcoin/signet/ mutinynet endpoints we ship. - Working examples for both providers. - TRUC 1P1C atomic package broadcast notes — Fulcrum requirement, no fallback semantics, verified arkade.sh deployments. - Electrum server compatibility matrix: which methods both Fulcrum and electrs support, which are Fulcrum-only (broadcast_package), what the provider deliberately avoids (verbose transaction.get). Also expands the class-level JSDoc on ElectrumOnchainProvider so it's useful in the typedoc-generated reference docs (default URL example, both Fulcrum and electrs constraints, exact-sat parsing note).
63ff202 to
90a5c56
Compare
1 participant
Summary
Adds end-to-end integration tests for `ElectrumOnchainProvider` against the nigiri regtest stack. Exercises every method on the `OnchainProvider` interface against a real Electrum backend so any divergence from `EsploraProvider` (which already has e2e coverage) surfaces side-by-side.
Mirror of `test/e2e/onchain.test.ts` plus electrum-specific coverage:
Plumbing
To talk to nigiri's bitcoin Electrum endpoint from a JS client (which can't open raw TCP), this PR uses the `electrum-ws` websocat bridge from vulpemventures/nigiri#258, now merged to nigiri master. Submodule points at arkade-regtest's plain `master`.
Drive-by fix: dropped verbose-tx dependency for electrs compatibility
The first round of CI on this branch failed because the provider was calling `blockchain.transaction.get` with `verbose=true`. That endpoint is Fulcrum-only — electrs explicitly does not support it (returns `"verbose transactions are currently unsupported"`). The nigiri stack uses electrs.
Refactored the three call sites that needed verbose data (`getTransactions`, `getTxStatus`, `watchAddresses`) to use only methods every Electrum server supports:
The `fetchVerboseTransaction(s)` methods on `WsElectrumChainSource` remain public for callers who explicitly target Fulcrum, but the provider no longer depends on them. Side effect: the SDK now works against electrs-based Electrum servers (Blockstream, mempool.space, self-hosted electrs) where it previously couldn't.
Out of scope
The 1P1C `broadcast_package` path isn't exercised in regtest because nigiri uses electrs (no `broadcast_package` support). Unit-test coverage of that path lives in `test/electrum.test.ts`. Once a Fulcrum-based regtest stack is available, the missing cases can be added in a follow-up.
Test plan
Summary by CodeRabbit