feat: use arkade-regtest for regtest environment#386
Conversation
Fixes Vite browser builds broken by Node-only createRequire in 3.2.2. The 3.2.3 release replaces createRequire with dynamic import() for browser-compatible lazy loading of the optional miniscript dependency.
Replace bespoke docker-compose.yml, server.Dockerfile, wallet.Dockerfile with shared arkade-regtest submodule. Pins arkd to v0.9.0 via .env.regtest. Simplify CI by removing nigiri GH Action and docker build steps.
|
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:
WalkthroughAdds a regtest env file and regtest submodule, replaces compose/Dockerfile-based regtest with regtest scripts, refactors CI into separate jobs, simplifies test setup to poll Boltz readiness, and updates test utilities/endpoints and package scripts/dependency. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant CI as "CI Runner (actions)"
participant Submod as "regtest submodule\n(./regtest/start-env.sh)"
participant Build as "Local Nigiri Build\n(binary)"
participant Docker as "Docker Containers\n(ark, wallet, services)"
participant Tests as "Test Runner\n(pnpm test:integration-docker)"
CI->>Submod: checkout submodule, run ./regtest/start-env.sh
Submod->>Build: seed/build nigiri binary
Submod->>Docker: start containers (ark, wallet, services)
CI->>Tests: run pnpm test:setup-docker / test:integration-docker (ARK_ENV=docker)
Tests->>Docker: poll readiness (ark server, Boltz pairs)
Docker-->>Tests: readiness OK
Tests-->>CI: test results
CI->>Submod: run ./regtest/clean-env.sh (always)
Submod-->>Docker: stop & cleanup containers
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
🔍 Arkana PR Review —
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (5)
.gitmodules (1)
4-4: Use a stable branch (e.g.,master) instead of a feature branch for submodule tracking.The configured feature branch
feat/shared-regtest-env-discoveryis temporary by design. Tracking it in.gitmodulesrisks maintenance issues if the branch is deleted, force-pushed, or rebased. The upstream repository defaults tomaster; prefer this stable branch for reliable CI workflows.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.gitmodules at line 4, Update the .gitmodules configuration to track a stable upstream branch instead of the temporary feature branch: replace the branch value "feat/shared-regtest-env-discovery" with a stable branch like "master" in the .gitmodules entry (the "branch = ..." line) so submodule tracking uses the upstream's default stable branch; commit the updated .gitmodules change..github/workflows/ci.yml (2)
73-83: Build job runs independently — consider if this is intentional.The
buildjob has noneedsclause, so it runs in parallel with lint/unit. This is fine if you want build verification regardless of test results. However, if build artifacts aren't useful when tests fail, addingneeds: [lint, unit]would save CI minutes on failing PRs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml around lines 73 - 83, The build job currently runs independently as "build" and has no needs clause; if you want it to wait for lint and unit jobs and avoid running when they fail, add a needs: [lint, unit] entry to the "build" job definition so the build job depends on those jobs completing successfully.
44-47: Cache key may not capture all relevant changes.The cache key only hashes
regtest/.env.defaultsand.env.regtest, but if the regtest submodule itself changes (e.g., different scripts, docker-compose files), the cache won't invalidate.Consider including the submodule commit hash or a version file in the cache key:
💡 Alternative cache key with submodule revision
- uses: actions/cache@v4 with: path: regtest/_build - key: nigiri-${{ hashFiles('regtest/.env.defaults', '.env.regtest') }} + key: regtest-${{ hashFiles('regtest/.env.defaults', '.env.regtest', 'regtest/**/*.sh', 'regtest/docker-compose*.yml') }}Or use a restore-keys pattern for partial cache hits:
key: regtest-${{ hashFiles('regtest/.env.defaults', '.env.regtest') }}-${{ github.sha }} restore-keys: | regtest-${{ hashFiles('regtest/.env.defaults', '.env.regtest') }}-🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml around lines 44 - 47, Update the actions/cache step that uses actions/cache@v4 (the key currently hashing 'regtest/.env.defaults' and '.env.regtest') so the cache key also reflects changes to the regtest submodule or repo revision; for example, append the regtest submodule commit or use github.sha (or a regtest version file) to the key and add a restore-keys pattern for partial hits to ensure the cache invalidates when regtest content changes.test/setup.mjs (2)
49-62: Thedockerargument passed to this script is unused.The
test:setup-dockerscript passes"docker"as an argument (node test/setup.mjs docker), but this script never referencesprocess.argv. If the parameter is intentionally unused after simplification, consider either:
- Removing the parameter from
test:setup-dockerin package.json, or- Adding a comment explaining that arkade-regtest handles all environment differences internally.
This is a minor cleanup to avoid confusion.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/setup.mjs` around lines 49 - 62, The package.json script test:setup-docker currently calls node test/setup.mjs with a "docker" argument that the setup() script ignores; either remove the extra argument from the test:setup-docker script, or make the intent explicit in setup.mjs by reading process.argv (e.g., check for "docker") or adding a clarifying comment near the top of the file explaining that arkade-regtest handles environment differences and the CLI argument is intentionally unused; update the test:setup-docker entry or the top of the setup.mjs file and keep references to the setup() function and process.argv to locate the change.
51-54: Add esplora readiness check to setup.Integration tests use
EsploraProvider("http://localhost:3000")for on-chain queries. The current setup only verifies ark server (7070) and Boltz (9069) are ready, but doesn't confirm esplora is responsive. If esplora takes longer to initialize than ark/Boltz, tests may fail intermittently on first blockchain query.💡 Proposed addition
async function setup() { try { await waitForArkServer(); await waitForBoltzPairs(); + await waitForEsplora(); console.log("\n━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━");Add a similar polling function:
async function waitForEsplora(maxRetries = 30, retryDelay = 2000) { console.log("Waiting for Esplora..."); for (let i = 0; i < maxRetries; i++) { try { execSync("curl -s http://localhost:3000/blocks/tip/height", { stdio: "pipe", }); console.log(" ✔ Esplora ready"); return true; } catch { if (i < maxRetries - 1) { console.log(` Waiting... (${i + 1}/${maxRetries})`); } await sleep(retryDelay); } } throw new Error("Esplora failed to be ready after maximum retries"); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/setup.mjs` around lines 51 - 54, Add an esplora readiness poll to ensure Esplora is responsive before tests: implement a new helper function waitForEsplora(maxRetries = 30, retryDelay = 2000) that repeatedly curl-requests http://localhost:3000/blocks/tip/height (or equivalent HTTP GET) with retries and sleep, logs progress and throws if exhausted, then call await waitForEsplora() inside setup() alongside waitForArkServer() and waitForBoltzPairs(); reference EsploraProvider("http://localhost:3000") in comments/tests to show why this check is required.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 73-83: The build job currently runs independently as "build" and
has no needs clause; if you want it to wait for lint and unit jobs and avoid
running when they fail, add a needs: [lint, unit] entry to the "build" job
definition so the build job depends on those jobs completing successfully.
- Around line 44-47: Update the actions/cache step that uses actions/cache@v4
(the key currently hashing 'regtest/.env.defaults' and '.env.regtest') so the
cache key also reflects changes to the regtest submodule or repo revision; for
example, append the regtest submodule commit or use github.sha (or a regtest
version file) to the key and add a restore-keys pattern for partial hits to
ensure the cache invalidates when regtest content changes.
In @.gitmodules:
- Line 4: Update the .gitmodules configuration to track a stable upstream branch
instead of the temporary feature branch: replace the branch value
"feat/shared-regtest-env-discovery" with a stable branch like "master" in the
.gitmodules entry (the "branch = ..." line) so submodule tracking uses the
upstream's default stable branch; commit the updated .gitmodules change.
In `@test/setup.mjs`:
- Around line 49-62: The package.json script test:setup-docker currently calls
node test/setup.mjs with a "docker" argument that the setup() script ignores;
either remove the extra argument from the test:setup-docker script, or make the
intent explicit in setup.mjs by reading process.argv (e.g., check for "docker")
or adding a clarifying comment near the top of the file explaining that
arkade-regtest handles environment differences and the CLI argument is
intentionally unused; update the test:setup-docker entry or the top of the
setup.mjs file and keep references to the setup() function and process.argv to
locate the change.
- Around line 51-54: Add an esplora readiness poll to ensure Esplora is
responsive before tests: implement a new helper function
waitForEsplora(maxRetries = 30, retryDelay = 2000) that repeatedly curl-requests
http://localhost:3000/blocks/tip/height (or equivalent HTTP GET) with retries
and sleep, logs progress and throws if exhausted, then call await
waitForEsplora() inside setup() alongside waitForArkServer() and
waitForBoltzPairs(); reference EsploraProvider("http://localhost:3000") in
comments/tests to show why this check is required.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 985d410d-fce9-43ec-9f34-f5d16ac90370
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (9)
.env.regtest.github/workflows/ci.yml.gitmodulesdocker-compose.ymlpackage.jsonregtestserver.Dockerfiletest/setup.mjswallet.Dockerfile
💤 Files with no reviewable changes (3)
- server.Dockerfile
- wallet.Dockerfile
- docker-compose.yml
Git submodules on Windows lose execute permissions. Ensure scripts are executable before running in CI.
🔍 PR Review —
|
- Change arkdExec container from 'arkd' to 'ark' (arkade-regtest name) - Add nigiri build dir to GITHUB_PATH so tests can find the binary
🔍 Review: arkade-regtest submodule migrationSubstantial cleanup — removes 510 lines including Observations:
Cross-repo: Coordinated with arkade-os/boltz-swap#106, arkade-os/dotnet-sdk#56, and ArkLabsHQ/arkade-regtest#1. Clean migration. Minor: consider aligning the |
🔍 Arkana Review — ts-sdk#386Clean migration from bespoke docker-compose to arkade-regtest submodule. Net -510 lines of duplicated infra is the right direction. Highlights
Observations1. Submodule branch should point to master post-merge [submodule "regtest"]
branch = feat/shared-regtest-env-discoveryThis is fine for coordinated development, but must be updated to 2. Dependency bump sneaked in 3. run: chmod +x regtest/*.sh regtest/helpers/*.sh && ./regtest/start-env.shGit should preserve execute bits if the files are committed with them. If they're losing perms, check 4. Cross-Repo |
In arkade-regtest, boltz-fulmine uses 7002 and the dedicated fulmine-delegator uses 7012. Update all test references.
|
Followup review (commit Port fix: delegator Cross-repo note: LGTM ✅ |
- Enable ARKD_ALLOW_CSV_BLOCK_TYPE (needed for VHTLC tests) - Set ARKD_VTXO_TREE_EXPIRY=20 and ARKD_BOARDING_EXIT_DELAY=1024 - Set zero fees (tests manage fees via setFees()/clearFees())
🔍 Arkana Review — Updated CommitsScope: Replace bespoke docker-compose + Dockerfiles with arkade-regtest submodule, restructure CI into parallel jobs. What Changed (vs. previous review)New commits update the submodule pointer to ArchitectureNet deletion of 518 lines (128 added, 518 removed). The
|
|
Followup review (commit |
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 @.env.regtest:
- Around line 11-14: The fee environment variables ARK_OFFCHAIN_INPUT_FEE,
ARK_ONCHAIN_INPUT_FEE, ARK_OFFCHAIN_OUTPUT_FEE, and ARK_ONCHAIN_OUTPUT_FEE are
quoted which is inconsistent with other numeric env values and may be parsed as
strings; remove the surrounding quotes so each is set as an unquoted numeric
literal (e.g., ARK_OFFCHAIN_INPUT_FEE=0.0) to match ARKD's numeric formatting
and other vars like ARKD_VTXO_TREE_EXPIRY and ARKD_BOARDING_EXIT_DELAY.
🪄 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: f2cdcd6a-dc69-4f7b-8647-009d12318f5a
📒 Files selected for processing (2)
.env.regtestregtest
✅ Files skipped from review due to trivial changes (1)
- regtest
| ARK_OFFCHAIN_INPUT_FEE="0.0" | ||
| ARK_ONCHAIN_INPUT_FEE="0.0" | ||
| ARK_OFFCHAIN_OUTPUT_FEE="0.0" | ||
| ARK_ONCHAIN_OUTPUT_FEE="0.0" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check if other .env files in arkade-regtest use quotes for numeric values
# Search for numeric environment variable assignments in the regtest submodule
fd -e env -e env.example . regtest/ --exec cat {} \; | grep -E '(FEE|DELAY|EXPIRY).*=' | head -20Repository: arkade-os/ts-sdk
Length of output: 42
🏁 Script executed:
# Check repository structure and locate env files
ls -la
find . -name ".env*" -type f 2>/dev/null | head -20Repository: arkade-os/ts-sdk
Length of output: 1787
🏁 Script executed:
# Check if regtest is a submodule and explore its contents
git config --file=.gitmodules --name-only --get-regexp path 2>/dev/null || echo "No submodules or .gitmodules not found"
ls -la | grep regtestRepository: arkade-os/ts-sdk
Length of output: 207
🏁 Script executed:
# Read the actual .env.regtest file to get full context
cat .env.regtestRepository: arkade-os/ts-sdk
Length of output: 517
🏁 Script executed:
# Search for how FEE variables are referenced in the codebase
rg "ARK_(?:OFFCHAIN|ONCHAIN)_(?:INPUT|OUTPUT)_FEE" -t json -t ts -t js --max-count=10Repository: arkade-os/ts-sdk
Length of output: 42
🏁 Script executed:
# Check contents of regtest directory
ls -la regtest/Repository: arkade-os/ts-sdk
Length of output: 170
🏁 Script executed:
# Search for setFees and clearFees implementations
rg "setFees|clearFees" --max-count=20Repository: arkade-os/ts-sdk
Length of output: 388
🏁 Script executed:
# Search for .env pattern usage in source files
rg "process\.env|import.*env" src/ --max-count=15Repository: arkade-os/ts-sdk
Length of output: 166
🏁 Script executed:
# Check if there's any documentation about arkd or fee configuration
find . -name "README*" -o -name "*.md" | xargs grep -l "fee\|FEE" 2>/dev/null | head -5Repository: arkade-os/ts-sdk
Length of output: 71
🏁 Script executed:
# Read the setFees/clearFees implementation
sed -n '1,100p' test/e2e/utils.ts | cat -nRepository: arkade-os/ts-sdk
Length of output: 3541
🏁 Script executed:
# Search for the actual implementation of setFees/clearFees function
rg "function setFees|export.*setFees" -A 10Repository: arkade-os/ts-sdk
Length of output: 640
🏁 Script executed:
# Check README for fee-related documentation
grep -i "fee\|ARK_" README.md | head -30Repository: arkade-os/ts-sdk
Length of output: 845
Remove quotes from fee environment variables for consistency with other numeric values.
The fee variables use quoted values ("0.0") while other numeric environment variables in the same file (ARKD_VTXO_TREE_EXPIRY=20, ARKD_BOARDING_EXIT_DELAY=1024) are unquoted. This inconsistency could potentially cause arkd's parser to treat fees as strings rather than numbers, even if the actual value is the same. Standardize by removing the quotes:
ARK_OFFCHAIN_INPUT_FEE=0.0
ARK_ONCHAIN_INPUT_FEE=0.0
ARK_OFFCHAIN_OUTPUT_FEE=0.0
ARK_ONCHAIN_OUTPUT_FEE=0.0
🧰 Tools
🪛 dotenv-linter (4.0.0)
[warning] 11-11: [QuoteCharacter] The value has quote characters (', ")
(QuoteCharacter)
[warning] 12-12: [QuoteCharacter] The value has quote characters (', ")
(QuoteCharacter)
[warning] 13-13: [QuoteCharacter] The value has quote characters (', ")
(QuoteCharacter)
[warning] 13-13: [UnorderedKey] The ARK_OFFCHAIN_OUTPUT_FEE key should go before the ARK_ONCHAIN_INPUT_FEE key
(UnorderedKey)
[warning] 14-14: [QuoteCharacter] The value has quote characters (', ")
(QuoteCharacter)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.env.regtest around lines 11 - 14, The fee environment variables
ARK_OFFCHAIN_INPUT_FEE, ARK_ONCHAIN_INPUT_FEE, ARK_OFFCHAIN_OUTPUT_FEE, and
ARK_ONCHAIN_OUTPUT_FEE are quoted which is inconsistent with other numeric env
values and may be parsed as strings; remove the surrounding quotes so each is
set as an unquoted numeric literal (e.g., ARK_OFFCHAIN_INPUT_FEE=0.0) to match
ARKD's numeric formatting and other vars like ARKD_VTXO_TREE_EXPIRY and
ARKD_BOARDING_EXIT_DELAY.
|
Iterative review (commit Three new commits since last review:
The All four coordinated PRs are aligned on the latest arkade-regtest commit. LGTM ✅ |
🔍 Arkana PR Review — ts-sdk#386 (iterative)feat: use arkade-regtest for regtest environment Changes Since Last ReviewNew commit
|
|
Iterative review — Submodule pointer update to |
🔍 Arkana PR ReviewOverall: Comprehensive migration to shared arkade-regtest. Largest of the three regtest migration PRs but well-structured. CI refactor ✅
|
🔍 Arkana PR ReviewOverall: Clean migration to shared regtest infrastructure. Massive reduction in duplicated infra code (-512 lines). ✅ What looks good
|
🔍 Arkana PR Review —
|
Set BITCOIN_LOW_FEE=false to avoid restarting bitcoind, which breaks nbxplorer's connection and causes arkd to have zero balance.
|
Iterative review — new commit Same fix as wallet#492: CI status: lint ✅, unit ✅, build ✅, integration ⏳ (still running). The integration job is the key one since it exercises the full regtest stack. Note: ts-sdk has Same submodule pin note as wallet: still at |
🔍 Review — arkade-regtest migration (ts-sdk)Net: +128 / −512 — removes custom ✅ What works well
|
🔍 Arkana PR Review — ts-sdk#386Scope: Replace custom docker-compose + Dockerfiles with Changes — Clean Migration ✅Infrastructure removal: Deletes CI restructured into parallel Key Observations1. Delegator port change: 2. 3. 4. Submodule tracks feature branch 5. With comment: "ts-sdk tests manage fees individually via setFees()/clearFees()". Makes sense — tests control fee state explicitly. Potential Issue
VerdictStraightforward migration. Port changes are correct. Test setup simplification is a big win. 👍 |
- ARKD_VTXO_TREE_EXPIRY 20 -> 512 (prevents expiry during setup) - Update regtest submodule to eecb40e (passes zero-fee config to arkd, fixing "Insufficient funds" in asset issuance tests)
|
Follow-up review (new commits: 5842c35..0949f5d) Two submodule bumps tracking arkade-regtest fixes:
The fee passthrough fix is important for ts-sdk — the zero fees in .env.regtest were previously not reaching the arkd container. Tests that call setFees()/clearFees() via the admin API would not be affected, but any test relying on the default fee config would see unexpected 1% input fees. LGTM. |
🔍 Arkana PR Review —
|
ARKD_ALLOW_CSV_BLOCK_TYPE=true is required when SCHEDULER_TYPE=block, otherwise VTXO_TREE_EXPIRY=512 is interpreted as seconds and arkd refuses to start.
|
Iterative review — new commit: Correct fix. When The
LGTM. |
arkd interprets values >= 512 as seconds (BIP 112 threshold). Set to 200 blocks which survives the ~30 block setup while staying in the block-interpretation range. Also enable ALLOW_CSV_BLOCK_TYPE=true.
200 blocks caused timeout in recovery/settleable VTXO tests that need to mine blocks to trigger expiry. The original 20-block value works because ts-sdk tests don't mine enough blocks to expire test VTXOs. The "Insufficient funds" fix is the env var export, not the expiry.
|
🔍 Iterative review — changes since Two new commits:
The revert reasoning is sound: ts-sdk tests actively mine blocks to trigger expiry, so a low value (20) is intentional. The "Insufficient funds" fix came from the env var export in the submodule, not from changing the expiry value. Good root cause analysis. Note: This means ts-sdk uses Cross-repo alignment: All three repos (wallet#492, boltz-swap#106, ts-sdk#386) now share the same submodule commit |
🔍 Arkana PR Review — ts-sdk#386Summary: Replaces custom ✅ Looks Good
|
Add [DIAG] logging to "should issue an asset with existing control asset", "should reissue an asset", and "should get vtxo chain" tests to capture: - VTXO count, values, and details before/after each issue() call - Balance state at each step - dustAmount value - 2-second wait between sequential issue() calls (matching passing tests) - Guard against undefined vtxos[0] in indexer test waitFor Also add CI diagnostic step to dump arkd env, fees, and container state.
🔍 Arkana — Followup Review (new commit)New commit: What changed
AssessmentThis is diagnostic/debugging code — no protocol, security, or functional changes. The 2s waits suggest a race condition where the second issuance fires before the first round's VTXO is indexed. These waits are a reasonable short-term fix; if the issue persists, a proper Note: The |
|
Iterative review (commit Single submodule bump: picks up conditional nbxplorer restart from arkade-regtest. Same fix as wallet#492 and boltz-swap#106. No concerns. |
Keep the 2s waits between sequential issue() calls (needed for VTXO indexing) but strip verbose [DIAG] console.log statements.
|
Iterative review (commit Removes all
Clean cleanup. LGTM ✅ |
|
Follow-up review (new commits since last review) Latest: diagnostic logging removed from e2e tests (kept the 2s waits between sequential Config (
Notable learnings from the commit history (valuable for future reference):
CI — Same pattern as sibling PRs: lint/unit/build parallel, integration gated. Cross-repo — Aligned with boltz-swap#106, wallet#492, dotnet-sdk#56 on same submodule commit. No security concerns. Looks good pending integration CI green. |
1 participant
Summary
Test plan
./regtest/start-env.shstarts full environmentpnpm test:unitpassespnpm test:integration-dockerpassesSummary by CodeRabbit