Repository navigation
Conversation
arkana-ai-bot
left a comment
There was a problem hiding this comment.
Arkana review — PR #79 (Add optional Taxi profile)
Overall: clean opt-in pattern, consistent with the existing evm-e2e idiom. The lifecycle wiring (taxi excluded from the wave, brought up explicitly in setupTaxi(), ports bound to loopback, volume persisted across stop/start, clean removes it) all look correct. The chain.mjs loadwallet-before-createwallet fix is correct: on restart an existing wallet file needs loading, not re-creating. No protocol-critical paths (VTXO, signing, forfeit, round lifecycle, unilateral exit) are touched.
One change-required defect, two nits.
[CHANGE REQUIRED] lib/setup/taxi.mjs:28 — reservedSats not null-guarded before BigInt() conversion
// line 26
if (funding.usableSats === null || !funding.arkAddress) fail('Taxi funding inventory is unavailable');
// line 28
const missing = float - BigInt(funding.usableSats) - BigInt(funding.reservedSats);funding.reservedSats is not checked before BigInt() is called on it. If the Taxi funding endpoint ever returns null or omits the field (feasible on a first-boot before any inventory exists), BigInt(null) / BigInt(undefined) throws a TypeError — bypassing the graceful fail() path and producing a cryptic crash rather than an actionable message. The null-guard on line 26 should cover both fields:
if (funding.usableSats === null || funding.reservedSats === null || !funding.arkAddress)
fail('Taxi funding inventory is unavailable');Nit — lib/setup/taxi.mjs:45-47 — info.operatorKey / info.dust not null-guarded
const coinsUrl = `http://localhost:${env('ARKD_PORT', '7070')}/v1/indexer/vtxos?scripts=5120${info.operatorKey}&spendableOnly=true`;
await waitForOrFail('Taxi inventory split', async () => (await fetchJson(coinsUrl)).json?.vtxos?.length >= 2, { attempts: 60, intervalMs: 2000 });info comes from a fetchJson call against ${publicUrl}/v1/info. If info.operatorKey or info.dust is absent (API schema drift, partial response), the guard float >= BigInt(info.dust) * 2n throws and the VTXO indexer URL is malformed. A defensive if (!info?.operatorKey || !info?.dust) + fail() before these two lines would give a better error.
Nit — scripts/taxi-smoke.mjs:17 — strict string equality for sats
assert.equal(funding.usableSats, env('TAXI_FLOAT_SATS', '500000'));In node:assert/strict mode, assert.equal is an alias for assert.strictEqual. If the Taxi API changes usableSats from a string to a number (common JSON evolution), the assertion fails with a confusing type mismatch rather than a value mismatch. assert.equal(String(funding.usableSats), ...) or assert.equal(Number(funding.usableSats), Number(...)) would make the intent explicit and survive a type change.
Minor observation — .env.defaults ships TAXI_OPERATOR_PRIVKEY
TAXI_OPERATOR_PRIVKEY=0000...0003 (scalar 3) lands in .env.defaults, which is loaded for every stack regardless of profile. The comment says "Public regtest-only key. Never fund this identity on another network." — that's the right note. The profile is opt-in so the key is never used unless taxi is explicitly activated, and having it pre-set avoids a foot-gun where the profile starts with no key. No action required; just confirming this was intentional.
CI — pinned commit
ref: ffdeff7c1b171e660e1c119cd6994c440a456338 in the taxi-smoke job is correct pinning practice for an external checkout.
arkana-ai-bot
left a comment
There was a problem hiding this comment.
Arkana follow-up review — commit a6a1b73
Status: 1 commit since last pass ("fix: use Taxi runtime freshness defaults"); 0 prior issues fixed, 1 change-required + 2 nits still open.
Incremental diff — docker/compose.ark.yml
The only change is the removal of TAXI_RECONCILE_INTERVAL_MS: '1000'. This is correct: a hard-coded 1 s polling interval was a regtest convenience that could cause noisy logs and excessive load during the smoke run. Dropping it defers to whatever the Taxi binary's compile-time default is, which is the right call for a compose config that shouldn't paper over runtime defaults. The waitForOrFail('Taxi inventory split', …, { attempts: 60, intervalMs: 2000 }) in lib/setup/taxi.mjs gives a 2-minute window for the split to complete, which should be ample unless the Taxi default reconcile interval is longer than that — no concern there given the 30-minute CI job timeout. No new findings from this diff.
Prior-issue tracker
[CHANGE REQUIRED — still open] lib/setup/taxi.mjs:28 — BigInt(funding.reservedSats) with no null guard
lib/setup/taxi.mjs is unchanged from the last pass. The null check on line 26 covers usableSats and arkAddress but not reservedSats; if the Taxi funding endpoint returns null or omits the field, BigInt(null) / BigInt(undefined) throws a TypeError rather than routing through fail(). This is the blocker from the previous review and still needs to be addressed:
// current (line 26)
if (funding.usableSats === null || !funding.arkAddress) fail('Taxi funding inventory is unavailable');
// fix
if (funding.usableSats === null || funding.reservedSats === null || !funding.arkAddress)
fail('Taxi funding inventory is unavailable');[Nit — still open] lib/setup/taxi.mjs:49-51 — info.operatorKey / info.dust not null-guarded
Unchanged. A defensive if (!info?.operatorKey || !info?.dust) fail(…) before those two lines would give a cleaner error on API schema drift.
[Nit — still open] scripts/taxi-smoke.mjs:17 — strict string equality for usableSats
Unchanged. assert.equal(funding.usableSats, env('TAXI_FLOAT_SATS', '500000')) will break with a confusing type-mismatch if the API starts returning a number. assert.equal(String(funding.usableSats), …) makes the intent explicit.
|
Current-source clarification at
The follow-up review body describes Update: exact-head CI now passed both smoke jobs. Taxi bootstrap funded one 500000-sat coin, completed its inventory split, and reached |
arkana-ai-bot
left a comment
There was a problem hiding this comment.
Arkana follow-up review — commit c364ea8
Status: 2 commits since last pass (, ); 2 of 3 prior issues fixed (including the change-required blocker); 1 nit still open. No longer blocking.
Prior-issue tracker
[CHANGE REQUIRED → Fixed in 9b02b32] lib/setup/taxi.mjs — BigInt(funding.reservedSats) with no null guard
Fixed. The guard is now !/^\d+$/.test(funding.reservedSats ?? '') which is strictly stronger than the null-only check I asked for — it rejects null, undefined, and non-numeric strings before any BigInt() conversion. ✓
[Nit → Fixed in 9b02b32] lib/setup/taxi.mjs — info.operatorKey / info.dust not null-guarded
Fixed. Both are validated at the top of the initial block with regexes (/^[0-9a-f]{64}$/ and /^[1-9]\d*$/) before coinsUrl is constructed or dust is used in arithmetic. ✓
[Nit → Still open] scripts/taxi-smoke.mjs:17 — strict string equality for usableSats
File is unchanged in both new commits. @Kukks notes the Taxi API specifies decimal strings; the assertion is correct for the current schema. The nit stands: assert.equal(String(funding.usableSats), env('TAXI_FLOAT_SATS', '500000')) would survive a future type change without a confusing failure. Low priority; not blocking.
Incremental diff findings
lib/setup/taxi.mjs — new wait-for-float loop (line ~40, 9b02b32)
The loop now polls the VTXO indexer directly rather than the admin /funding endpoint. The reduce sums BigInt(coin.amount) without validating each element's amount field. That said, waitFor (line 16 of wait.mjs) explicitly swallows thrown exceptions and treats them as "not ready yet", so a malformed VTXO response would simply retry silently rather than crash. Acceptable for regtest tooling.
lib/setup/taxi.mjs — rescan on every poll iteration in the paused-restart path (c364ea8)
The paused-restart path now issues a POST to /admin/api/rescan on each waitForOrFail tick (default: 30 attempts × 2 s = 60 s window). For regtest this is harmless — the endpoint is idempotent and the volume is low. The approach is sound: rescan, then check walletSynced, is the right ordering to confirm the wallet has caught up before comparing retained funding.
lib/setup/taxi.mjs — Content-Type: application/json on both rescan calls (c364ea8)
Correct fix for the observed HTTP 400; both rescan requests now send {} with the right header. ✓
No protocol-critical paths (VTXO, signing, forfeit, round lifecycle, unilateral exit) are touched by either commit. CI passed at exact head per the run linked in the PR thread.
arkana-ai-bot
left a comment
There was a problem hiding this comment.
Arkana follow-up review — commit c364ea8
Status: 2 commits since last pass (9b02b32, c364ea8); 2 of 3 prior issues fixed (including the change-required blocker); 1 nit still open. No longer blocking.
Prior-issue tracker
[CHANGE REQUIRED → Fixed in 9b02b32] lib/setup/taxi.mjs — BigInt(funding.reservedSats) with no null guard
Fixed. The guard is now !/^\d+$/.test(funding.reservedSats ?? '') which is strictly stronger than the null-only check I requested — it rejects null, undefined, and non-numeric strings before any BigInt() conversion. ✓
[Nit → Fixed in 9b02b32] lib/setup/taxi.mjs — info.operatorKey / info.dust not null-guarded
Fixed. Both are validated at the top of the initial block with regexes (/^[0-9a-f]{64}$/ and /^[1-9]\d*$/) before coinsUrl is constructed or dust is used in arithmetic. ✓
[Nit → Still open] scripts/taxi-smoke.mjs:17 — strict string equality for usableSats
File is unchanged in both new commits. @Kukks notes the Taxi API specifies decimal strings; the assertion is correct for the current schema. The nit stands: assert.equal(String(funding.usableSats), env('TAXI_FLOAT_SATS', '500000')) would survive a future type change without a confusing failure message. Low priority; not blocking.
Incremental diff findings
lib/setup/taxi.mjs — new wait-for-float loop (~line 40, 9b02b32)
The loop now polls the VTXO indexer directly rather than the admin /funding endpoint. The reduce sums BigInt(coin.amount) without validating each element's amount field first. That said, waitFor in wait.mjs explicitly swallows thrown exceptions and treats them as "not ready yet", so a malformed VTXO response would retry silently rather than crash. Acceptable for regtest tooling.
lib/setup/taxi.mjs — rescan on every poll iteration in the paused-restart path (c364ea8)
The paused-restart path now issues a POST to /admin/api/rescan on each waitForOrFail tick (default: 30 attempts × 2 s = 60 s window). For regtest this is harmless — the endpoint is idempotent and the volume is low. The ordering (rescan, then check walletSynced) is correct: it ensures the wallet has caught up before comparing retained funding.
lib/setup/taxi.mjs — Content-Type: application/json on both rescan calls (c364ea8)
Correct fix for the observed HTTP 400; both rescan requests now send {} with the right header. ✓
No protocol-critical paths (VTXO, signing, forfeit, round lifecycle, unilateral exit) are touched by either commit. CI passed at exact head per the run linked in the PR thread.
arkana-ai-bot
left a comment
There was a problem hiding this comment.
Arkana follow-up review — commit 78d0b7a
Status: 1 commit since last pass ("fix: reconcile regtest Taxi lockups promptly"); 0 prior issues fixed; 1 nit still open (unchanged). No blocking issues.
Prior-issue tracker
[Nit → Still open] scripts/taxi-smoke.mjs:17 — strict string equality for usableSats
scripts/taxi-smoke.mjs is unchanged. assert.equal(funding.usableSats, env('TAXI_FLOAT_SATS', '500000')) is correct for the current decimal-string schema; the nit about wrapping in String() to survive a future type change stands at low priority. Not blocking.
Incremental diff findings — docker/compose.ark.yml
The sole change reintroduces TAXI_RECONCILE_INTERVAL_MS, this time as ${TAXI_RECONCILE_INTERVAL_MS:-10000} instead of the hard-coded '1000' that was removed in a6a1b73.
Assessment: correct. The history here is intentional and coherent:
a6a1b73removed1000to stop papering over the binary's compile-time default.- This commit establishes that the binary's production default is apparently too long for the CI smoke window; 10 seconds is the right regtest-facing floor.
- Using
${TAXI_RECONCILE_INTERVAL_MS:-10000}rather than a bare literal means operators can override it via env without touching the compose file — strictly better than the previous hard-coded form.
The 10-second interval pairs correctly with waitForOrFail('Taxi inventory split', …, { attempts: 60, intervalMs: 2000 }) in lib/setup/taxi.mjs, which grants a 120-second window. An inventory split should clear well within one to two reconcile cycles at this rate.
TAXI_RECONCILE_INTERVAL_MS is absent from .env.defaults (intentional — only relevant when the taxi profile is active) and from .env.taxi (the compose shell default of 10 000 ms is the intended regtest value). No inconsistency.
No protocol-critical paths (VTXO, signing, forfeit, round lifecycle, unilateral exit) are touched.
- compose: drop TAXI_DB_PATH and TAXI_HTTP_PORT (set by the Taxi image's Dockerfile ENV) and the expiry-headroom/recovery seconds (config.ts defaults at the CI-pinned Taxi ffdeff7). The renewal threshold and reconcile interval differ from the defaults and stay. - .env.taxi: drop TAXI_IMAGE, already set identically in .env.defaults. - regtest.mjs: drop the TAXI_IMAGE guard. It could only fire on an explicitly empty value, which compose's fallback renders as the same tag. - setupTaxi: drop the locktime margins from the partial policy PATCH. They equal DEFAULT_POLICY and the PATCH only runs on an unedited policy. - ALL_PROFILES: derive from PROFILE_DEPS plus solver-init, and drop the assertion that only guarded drift between the two lists. - taxi-profile test: keep the loopback binding and expiry-vs-renewal checks; drop assertions that restate YAML literals or the removed env, and the REGTEST_PROJECT/REGTEST_CONTAINER_PREFIX env they no longer use.
arkana-ai-bot
left a comment
There was a problem hiding this comment.
Arkana follow-up review — commit 301bd3d
Status: 1 commit since last pass ("refactor(taxi): drop settings that repeat Taxi defaults"); all prior issues fixed or unchanged from the last pass; 1 nit still open. Not blocking.
Prior-issue tracker
[CHANGE REQUIRED → Fixed in 9b02b32, confirmed] lib/setup/taxi.mjs — BigInt(funding.reservedSats) null guard
lib/setup/taxi.mjs is untouched in this commit; the fix from 9b02b32 is still present. ✓
[Nit → Fixed in 9b02b32, confirmed] lib/setup/taxi.mjs — info.operatorKey/info.dust null-guarded
Same — unchanged and still fixed. ✓
[Nit → Still open] scripts/taxi-smoke.mjs:17 — strict string equality for usableSats
assert.equal(funding.usableSats, env('TAXI_FLOAT_SATS', '500000')) is unchanged. As noted last pass, the Taxi API currently specifies decimal strings so the test is correct today; assert.equal(String(funding.usableSats), ...) would survive a future type change without a confusing mismatch message. Low priority; not blocking.
Incremental diff — 301bd3d findings
lib/compose.mjs — ALL_PROFILES derived from PROFILE_DEPS
[...Object.keys(PROFILE_DEPS), 'solver-init'] produces the same 14 profiles as the old hard-coded list; solver-init is correctly appended at the end. The import chain compose.mjs → profiles.mjs → log.mjs is acyclic (log.mjs has no imports). ✓ This is a genuine maintenance win: new profiles added to PROFILE_DEPS are automatically included in stop/down coverage, removing the previous sync-drift risk.
docker/compose.ark.yml — dropped TAXI_DB_PATH, TAXI_HTTP_PORT, TAXI_MIN_EXPIRY_HEADROOM_SECONDS, TAXI_RECOVERY_BROADCAST_SECONDS, TAXI_RECOVERY_CRITICAL_SECONDS
All five are confirmed Taxi config.ts defaults at the CI-pinned commit (ffdeff7). The host-facing port binding 127.0.0.1:${TAXI_HTTP_PORT:-8080}:8080 uses the regtest env var as the published port; the container still exposes 8080 from the image's Dockerfile ENV — two separate things that happen to share a name and default. Correct. ✓
lib/setup/taxi.mjs — dropped locktimeMarginBlocks: 144 and locktimeMarginSeconds: 86400 from policy PATCH
Commit claims these equal DEFAULT_POLICY. The PATCH runs only on an unedited policy (initial === true), so Taxi will apply its compile-time defaults — behaviour is identical. ✓
regtest.mjs — dropped TAXI_IMAGE guard
The guard if (active.has('taxi') && !env('TAXI_IMAGE')) fail(...) could only fire when TAXI_IMAGE was explicitly set to an empty string, since compose's ${TAXI_IMAGE:-arkade-taxi:regtest} fallback silently fills in the default tag otherwise. Dropping it is correct; the emulator guard directly below still provides a meaningful boot-time check. ✓
test/taxi-profile.test.mjs — removed volume and env-literal assertions
The container-name assertion removal is fine (it depended on REGTEST_PROJECT, which was also dropped). The env-literal assertions (TAXI_ESPLORA_URL, TAXI_PUBLIC_ARKD_URL, TAXI_PUBLIC_EMULATOR_URL) are YAML constants and don't need test coverage. The TAXI_MIN_EXPIRY_HEADROOM_SECONDS > TAXI_RECOVERY_BROADCAST_SECONDS headroom check is gone because neither env var is set anymore; the safety property still holds at the image's compile-time defaults — acceptable. Minor nit: the taxi.volumes.some(source === 'taxi_datadir') assertion was a useful runtime sanity-check that the persistence mount is wired up; its removal is a small coverage regression. The volume is still declared in the YAML (taxi_datadir volume + /data mount is still there after this commit), so correctness is unaffected — it just won't be caught by this test if someone later removes the mount.
No protocol-critical paths (VTXO, signing, forfeit, round lifecycle, unilateral exit) are touched. The refactor is clean.
|
@arkana-ai-bot on the two nits at
|
Arkana — response to @Kukks on
|
arkana-ai-bot
left a comment
There was a problem hiding this comment.
Arkana follow-up review — commit baab2f4
Status: 3 commits + 1 merge since last pass (7f0e64a emulator bump, 9ac38fe delegatee container, e639c8d/baab2f4 merges); 0 prior issues newly fixed; 1 prior nit still open. No new blockers; requesting-changes flag cleared.
Prior-issue tracker
[Nit — still open] scripts/taxi-smoke.mjs:17 — assert.equal(funding.usableSats, env('TAXI_FLOAT_SATS', '500000'))
File is unchanged in this diff. @Kukks confirmed the Taxi API specifies decimal strings, so the assertion is correct today. The fragility remains: a future type change in the response would produce a confusing type-mismatch failure rather than a value mismatch. Not blocking; carry-forward only.
Incremental diff findings
.env.defaults + docker/compose.ark.yml:194 — Emulator image bump
v0.0.8-rc.0 → v0.0.8 in .env.defaults; compose inline default bumped from the stale v0.0.6 to v0.0.8 in the same pass. The commit message documents the behavioral change: OP_CHECKTIME now pushes a boolean on a future timestamp instead of failing. Both files are now in sync. ✓
docker/compose.ark.yml:249-262 — Delegatee port not loopback-bound
ports:
- '7280:7080' # binds 0.0.0.0 — all interfacesEvery other externally-accessible service in this file that has restart: unless-stopped and no auth in prod (Taxi, covclaimd, etc.) uses an explicit loopback prefix:
ports:
- '127.0.0.1:${TAXI_HTTP_PORT:-8080}:8080'The delegatee service exposes port 7280 on all interfaces and has DELEGATEE_ADMIN_AUTH: disabled. On a shared CI runner or a developer machine that is reachable from a LAN, the admin-capable endpoint is accessible without authentication from any host that can reach port 7280. Nit for a regtest fixture, but it is inconsistent with the established pattern.
Suggested fix:
ports:
- '127.0.0.1:${DELEGATEE_PORT:-7280}:7080'docker/compose.ark.yml:261 — DELEGATEE_DELEGATE_KEY missing regtest-only disclaimer
TAXI_OPERATOR_PRIVKEY in .env.defaults carries an explicit comment: "Public regtest-only key. Never fund this identity on another network." The analogous key for the delegatee is embedded directly in the compose YAML without any such notice:
DELEGATEE_DELEGATE_KEY: 5c2b7d0e8f1a3c4b6d9e0f2a1b3c5d7e9f0a2b4c6d8e0f1a3b5c7d9e1f0a2b4cThis is not a security concern in practice (regtest tooling, opt-in profile), but the missing disclaimer is a gap relative to how other keying material is handled elsewhere in this repo. A one-line comment above the key — identical to the Taxi one — would close that gap.
regtest.mjs:303 and :440 — Delegatee port hardcoded, not env-overridable
lines.push(' delegatee http://localhost:7280'); // banner
await waitForOrFail('delegatee /v1/info', () => httpOk('http://localhost:7280/v1/info'), { attempts: 60 });Every other service reads its host port from an env variable (e.g., env('TAXI_HTTP_PORT', '8080')), which is what allows simultaneous stacks to be isolated by setting distinct port numbers alongside REGTEST_CONTAINER_PREFIX. Hardcoding 7280 here means two stacks with the delegatee profile active on the same host will collide on that port regardless of any prefix/override. The taxi-profile test already demonstrates the isolation pattern (TAXI_HTTP_PORT: '28080').
Suggested fix: introduce DELEGATEE_PORT in .env.defaults and read it in both places.
No depends_on: postgres in the delegatee service
The delegatee connects to the shared postgres (DELEGATEE_DATABASE_URL), but the service definition has no depends_on: postgres (with condition service_healthy). The postgres healthcheck is in place and the delegatee has restart: unless-stopped, so in practice it will retry until postgres is ready. The waitForOrFail('delegatee /v1/info', …, { attempts: 60 }) in regtest.mjs also buffers this. Acceptable for regtest; flagging for awareness only.
No protocol-critical paths (VTXO, signing, forfeit, round lifecycle, unilateral exit) are touched in this diff. The delegatee is introduced only as a compose service entry and a profile dependency; its implementation is opaque to this repo.
Adds an opt-in
taxiprofile to arkade-regtest using its existing Compose lifecycle and project/port settings. It runs the real Taxi against arkd, emulator and Esplora, derives the funding address at runtime, and preserves operator policy and funds across stop/start. Taxi is built from pinnedffdeff7, with internal connections and host-facing advertised provider URLs. Bootstrap validates funding parameters, waits for the actual indexed deposit, enables the initial policy, and rescans with Taxi's JSON mutation contract. The Taxi profile defaults to an overridable ten-second reconciliation interval, so finalized lockups become visible inside the consumer confirmation window. This also keeps the safety freshness window long enough for normal bootstrap and reconciliation. Paused restarts rescan and wait for wallet synchronization without replacing the saved policy. Validation: nine local CLI tests pass (seven on the baseline). Prior-head Linux CI passed both standard smoke and Taxi smoke: one 500000-sat funding payment, actual inventory split,/ready, then stop/start preserving the address, balance and edited paused policy. One Taxi-only retry addressed a hosted-runner acquisition cancellation before execution. This proves profile bootstrap and persistence; payment and swap scenarios remain consumer E2E coverage. At301bd3dthe profile also drops Compose settings that only repeated Taxi image/config defaults (database path, HTTP port, expiry and recovery windows), the redundant.env.taxiimage line andTAXI_IMAGEguard, and policy margins equal to Taxi defaults, and derivesALL_PROFILESfromPROFILE_DEPS. Nine local CLI tests pass before and after. Diff: 230 added / 19 removed text lines. No new runtime dependency or separate orchestration framework. All three funded Wallet Mobile Chrome journeys passed on this profile at78d0b7a, before the settings cleanup: sub-dust send/claim and live receipts; three queued free asset claims on reopen; carrier purchase, sponsored delivery and paused-service refusal. Node checks pass all nine tests; rendered interval defaults and explicit overrides were verified. Each consumer owns its layer's payment assertions.Prior-head CI at
301bd3d. Both standard smoke and Taxi smoke pass, including actual funding, inventory splitting, readiness and paused stop/start persistence. Payment and swap assertions remain in their owning consumer/client E2E suites.Current update
baab2f4merges currentmasterat9ac38fe, retaining the optional Taxi profile, its smoke coverage and paused-policy persistence alongside upstream's delegatee profile and emulator v0.0.8 default. The profile list remains derived fromPROFILE_DEPS, and the startup banner includes both Taxi and delegatee.Local validation: all nine CLI tests pass before and after the merge,
node --check regtest.mjsandgit diff --checkpass, and executable assertions confirm Taxi is opt-in and resolves its ark/base/emulator dependencies. Ponytail review found no further actionable duplication. Slopo indexed 123 source units and recognized 14 changed units with no duplicate matches, using an identical-byte .mjs-to-.js review copy because its installed parser does not support .mjs. Exact-head CI atbaab2f4passes both standard smoke and Taxi smoke, including funding, inventory splitting, readiness and paused restart persistence. Human review and consumer payment assertions remain separate gates.