Release: develop -> main - #166
Merged
Merged
Conversation
* feat(api): add GET /api/history endpoint (#153) Adds paginated per-address transaction history reading from the account_history table populated by the migration-0008 trigger. * /api/history?address=<hex>&limit=<n>&offset=<n>, behind 'always' status (no feature gate). * Reuses the same hex decode + 32-byte length rules /api/balance applies; rejects missing address, invalid hex, limit outside [1, 200], negative offset with 400. * Newest-first ORDER BY changed_at DESC; offset beyond total returns empty items with the unfiltered total so the caller can detect end-of-list. * LEFT JOIN observed_inscriptions + pending_inscriptions on triggering_commit_txid so block_height + status surface once a future caller threads zkcoins.account_commit_txid through the upsert. Today both joined columns are NULL and txid/block_height remain null on the wire. * counterparty + memo intentionally null in v1: the current schema does not store the recipient address per-mutation and has no memo column. Out-of-scope for this PR: app UI changes, OpenAPI export, WebSocket push, the Zod schema in zk-coins/app. * README Features table entry added. * fix(api/history): round-2 review fixes — SQL-side filter, pending default, 422, one DB call Six fixes from the two independent reviews of #153: 1. Push `source IN ('mint','send','receive')` into both the page query and the filtered total so pagination is correct. The post-fetch `filter_map` stays as a defense-in-depth safety net but no longer actually drops rows. Closes a bug where `total` over-counted hidden rows and pages came back smaller than the requested `limit`. 2. Status default flips from `confirmed` to `pending`. A DB-committed `account_history` row only proves a server-side state change, not an on-chain confirmation. The new mapping: * pending_inscriptions.status='complete' -> 'confirmed' * pending_inscriptions.status='failed' -> 'failed' * pending_inscriptions.status IN ('constructed', 'commit_broadcast','reveal_broadcast') -> 'pending' * no pending row + observed_inscriptions.block_height IS NOT NULL -> 'confirmed' * no pending row + no observed row -> 'pending' The match goes through a new `PendingInscriptionStatus` enum so the `match` is exhaustive — a future schema state addition fails to compile (no `_ => "pending"` catch-all). 3. Collapse `count_account_history` + `list_account_history` into one round-trip via a CTE: one filtered-count CTE cross-joined to the LIMIT/OFFSET page CTE. The handler now has a single DB error branch, closing the dead-arm coverage gap the two-call layout left behind. The empty-page case still returns the real total (sentinel row) so the caller can drive pagination without a second query. 4. Switch all input-validation status codes from 400 to 422 to match `/api/balance` and the rest of the read surface. Framework-level 400 (axum Query rejection on non-integer limit) stays. 5. A non-null `prev_data` blob that fails to bincode-decode no longer silently collapses to `prev_balance = 0` (which would fabricate the full new balance as the delta). The row is dropped with a warn log. 6. TODOs for the deferred work reference the follow-up issues: * #159 — thread `zkcoins.account_commit_txid` GUC * #160 — capture counterparty_address per row (zk-coins/app#145 covers the typed-client wiring on the other repo.) * fix(api/history): hoist blob.len() out of tracing::warn! for coverage The tracing macro lazily evaluates its arguments based on the active log level, so under the default-off test subscriber blob.len() is never executed. Coverage Gate flagged it as the only uncovered line in the node + shared scope (99.97% lines, 1 missed). Pre-compute blob_len in a let binding so the line runs regardless of tracing config.
…, not DFX (#158) Three spots in README.md leaked the DFX-as-operator framing: 1. Trust Model table row was "Yes — DFX runs the hosted node". The hosted node at api.zkcoins.app is operated by zkcoins.app (one of hopefully many such service providers). DFX is the underlying hosting / financial-services layer — invisible to wallet integrators and consumers of this README. 2. Configuration table description for ESPLORA_URL said "PRD: ... (DFX Mainnet stack)". Replaced with "On the api.zkcoins.app stack: PRD ..., DEV ...". Same meaning, correct attribution. 3. ESPLORA_WS_URL description analog — "on the DFX mempool/backend stack" → "self-hosted mempool/backend sidecar" (the relevant detail is "self-hosted vs external", not "whose hosting"). Memory: reference_zkcoins_org_structure documents the three-layer separation (zkCoins protocol / zkcoins.app service provider / DFX infra) so this distinction does not get muddled again. No code change. Pre-push sanity: fmt clean, clippy clean.
…rover_warm (#154) * perf(bootstrap): warmup prover in a background task; gate /health/ready on prover_warm PR #147 paid the ~7 s Plonky2 cold-prove tax synchronously between load_from_pg and TcpListener::bind, which pushed API offline time per deploy from ~14 s (circuit build alone) to ~21 s (circuit build + cold prove). The user constraint is explicit: API must be reachable as soon as possible. PR #147 was closed for failing that constraint. This shape moves the warmup off the bootstrap-critical path: 1. TcpListener::bind returns at ~0.1 s. axum::serve starts draining connections — /health (liveness) is 200, /api/* paths return correct answers (a /api/mint or /api/send during the warmup window pays the ~7 s cold tax, but it serves correctly). 2. tokio::task::spawn_blocking launches AccountNode::warmup_prover on the blocking pool so the CPU-bound prove does not starve the tokio worker that owns axum::serve. 3. After ~21 s the warmup task flips the new prover_warm Arc<AtomicBool> to true. /health/ready transitions from 503 with {"status":"starting","prover":"warming","failures":["prover"]} to 200 with {"status":"ready","prover":"ready"}. A load balancer / Kuma monitor keyed on /health/ready holds traffic on the previous-generation pod through the warmup window; /health (liveness) is unaffected so container restart loops are not triggered. ZKCOINS_SKIP_BOOTSTRAP_WARMUP=1 skips the background task entirely (smoke tests in runtime_tests.rs). Three architecture decisions, codified in MIGRATION_RESEARCH.md §7.25: - spawn_blocking over tokio::spawn — Plonky2 prove is CPU-bound and would starve the tokio worker dispatching HTTP requests. - Arc<AtomicBool> over Arc<RwLock<bool>> — flag is write-once + read-many, AtomicBool::store is a single instruction. - std::process::exit(1) over panic!() — a panic inside spawn_blocking only surfaces when the JoinHandle is awaited (it deliberately is not), so a bare panic would leave the node serving 503 forever. exit(1) crash-loops the container at the same severity as PR #147's synchronous expect(). CONTRIBUTING.md gains a Bootstrap timing section + a row for the new env var. AccountNode::warmup_prover + the warmup_prover_completes_successfully test were adapted from PR #147 with the return type switched to anyhow::Result for the runtime call site. A new router test asserts /health/ready returns 503 with the warming-tag payload when prover_warm is false. * docs(runtime): correct warmup-task scanner-ordering comment Reviewer caught: the prior comment claimed the scanner spawns AFTER start_rest_node returns. That is factually wrong — main.rs runs start_rest_node + run_scanner_ws concurrently via tokio::spawn. The correctness conclusion still holds because the scanner locks `state`, not `account_node`. Rewrite the comment to name the right invariant and the right contender (a user request that lands during the ~7 s warmup window). * test(coverage): drop unused .map_err closure in warmup_prover The previous shape `.map(|_| ()).map_err(|e| anyhow!("...{e}"))` left two never-called closures in the happy-path test, costing the 100% function + 100% line coverage gate. `?` propagation matches what `prove_initial` already returns (`anyhow::Result<Proof>`) and is covered by the same single test.
* feat(api): add GET /api/history endpoint (#153) (#162) * feat(api): add GET /api/history endpoint (#153) Adds paginated per-address transaction history reading from the account_history table populated by the migration-0008 trigger. * /api/history?address=<hex>&limit=<n>&offset=<n>, behind 'always' status (no feature gate). * Reuses the same hex decode + 32-byte length rules /api/balance applies; rejects missing address, invalid hex, limit outside [1, 200], negative offset with 400. * Newest-first ORDER BY changed_at DESC; offset beyond total returns empty items with the unfiltered total so the caller can detect end-of-list. * LEFT JOIN observed_inscriptions + pending_inscriptions on triggering_commit_txid so block_height + status surface once a future caller threads zkcoins.account_commit_txid through the upsert. Today both joined columns are NULL and txid/block_height remain null on the wire. * counterparty + memo intentionally null in v1: the current schema does not store the recipient address per-mutation and has no memo column. Out-of-scope for this PR: app UI changes, OpenAPI export, WebSocket push, the Zod schema in zk-coins/app. * README Features table entry added. * fix(api/history): round-2 review fixes — SQL-side filter, pending default, 422, one DB call Six fixes from the two independent reviews of #153: 1. Push `source IN ('mint','send','receive')` into both the page query and the filtered total so pagination is correct. The post-fetch `filter_map` stays as a defense-in-depth safety net but no longer actually drops rows. Closes a bug where `total` over-counted hidden rows and pages came back smaller than the requested `limit`. 2. Status default flips from `confirmed` to `pending`. A DB-committed `account_history` row only proves a server-side state change, not an on-chain confirmation. The new mapping: * pending_inscriptions.status='complete' -> 'confirmed' * pending_inscriptions.status='failed' -> 'failed' * pending_inscriptions.status IN ('constructed', 'commit_broadcast','reveal_broadcast') -> 'pending' * no pending row + observed_inscriptions.block_height IS NOT NULL -> 'confirmed' * no pending row + no observed row -> 'pending' The match goes through a new `PendingInscriptionStatus` enum so the `match` is exhaustive — a future schema state addition fails to compile (no `_ => "pending"` catch-all). 3. Collapse `count_account_history` + `list_account_history` into one round-trip via a CTE: one filtered-count CTE cross-joined to the LIMIT/OFFSET page CTE. The handler now has a single DB error branch, closing the dead-arm coverage gap the two-call layout left behind. The empty-page case still returns the real total (sentinel row) so the caller can drive pagination without a second query. 4. Switch all input-validation status codes from 400 to 422 to match `/api/balance` and the rest of the read surface. Framework-level 400 (axum Query rejection on non-integer limit) stays. 5. A non-null `prev_data` blob that fails to bincode-decode no longer silently collapses to `prev_balance = 0` (which would fabricate the full new balance as the delta). The row is dropped with a warn log. 6. TODOs for the deferred work reference the follow-up issues: * #159 — thread `zkcoins.account_commit_txid` GUC * #160 — capture counterparty_address per row (zk-coins/app#145 covers the typed-client wiring on the other repo.) * fix(api/history): hoist blob.len() out of tracing::warn! for coverage The tracing macro lazily evaluates its arguments based on the active log level, so under the default-off test subscriber blob.len() is never executed. Coverage Gate flagged it as the only uncovered line in the node + shared scope (99.97% lines, 1 missed). Pre-compute blob_len in a let binding so the line runs regardless of tracing config. * docs: clarify operator language — api.zkcoins.app runs at zkcoins.app, not DFX (#158) Three spots in README.md leaked the DFX-as-operator framing: 1. Trust Model table row was "Yes — DFX runs the hosted node". The hosted node at api.zkcoins.app is operated by zkcoins.app (one of hopefully many such service providers). DFX is the underlying hosting / financial-services layer — invisible to wallet integrators and consumers of this README. 2. Configuration table description for ESPLORA_URL said "PRD: ... (DFX Mainnet stack)". Replaced with "On the api.zkcoins.app stack: PRD ..., DEV ...". Same meaning, correct attribution. 3. ESPLORA_WS_URL description analog — "on the DFX mempool/backend stack" → "self-hosted mempool/backend sidecar" (the relevant detail is "self-hosted vs external", not "whose hosting"). Memory: reference_zkcoins_org_structure documents the three-layer separation (zkCoins protocol / zkcoins.app service provider / DFX infra) so this distinction does not get muddled again. No code change. Pre-push sanity: fmt clean, clippy clean. * perf(bootstrap): warmup prover in background; gate /health/ready on prover_warm (#154) * perf(bootstrap): warmup prover in a background task; gate /health/ready on prover_warm PR #147 paid the ~7 s Plonky2 cold-prove tax synchronously between load_from_pg and TcpListener::bind, which pushed API offline time per deploy from ~14 s (circuit build alone) to ~21 s (circuit build + cold prove). The user constraint is explicit: API must be reachable as soon as possible. PR #147 was closed for failing that constraint. This shape moves the warmup off the bootstrap-critical path: 1. TcpListener::bind returns at ~0.1 s. axum::serve starts draining connections — /health (liveness) is 200, /api/* paths return correct answers (a /api/mint or /api/send during the warmup window pays the ~7 s cold tax, but it serves correctly). 2. tokio::task::spawn_blocking launches AccountNode::warmup_prover on the blocking pool so the CPU-bound prove does not starve the tokio worker that owns axum::serve. 3. After ~21 s the warmup task flips the new prover_warm Arc<AtomicBool> to true. /health/ready transitions from 503 with {"status":"starting","prover":"warming","failures":["prover"]} to 200 with {"status":"ready","prover":"ready"}. A load balancer / Kuma monitor keyed on /health/ready holds traffic on the previous-generation pod through the warmup window; /health (liveness) is unaffected so container restart loops are not triggered. ZKCOINS_SKIP_BOOTSTRAP_WARMUP=1 skips the background task entirely (smoke tests in runtime_tests.rs). Three architecture decisions, codified in MIGRATION_RESEARCH.md §7.25: - spawn_blocking over tokio::spawn — Plonky2 prove is CPU-bound and would starve the tokio worker dispatching HTTP requests. - Arc<AtomicBool> over Arc<RwLock<bool>> — flag is write-once + read-many, AtomicBool::store is a single instruction. - std::process::exit(1) over panic!() — a panic inside spawn_blocking only surfaces when the JoinHandle is awaited (it deliberately is not), so a bare panic would leave the node serving 503 forever. exit(1) crash-loops the container at the same severity as PR #147's synchronous expect(). CONTRIBUTING.md gains a Bootstrap timing section + a row for the new env var. AccountNode::warmup_prover + the warmup_prover_completes_successfully test were adapted from PR #147 with the return type switched to anyhow::Result for the runtime call site. A new router test asserts /health/ready returns 503 with the warming-tag payload when prover_warm is false. * docs(runtime): correct warmup-task scanner-ordering comment Reviewer caught: the prior comment claimed the scanner spawns AFTER start_rest_node returns. That is factually wrong — main.rs runs start_rest_node + run_scanner_ws concurrently via tokio::spawn. The correctness conclusion still holds because the scanner locks `state`, not `account_node`. Rewrite the comment to name the right invariant and the right contender (a user request that lands during the ~7 s warmup window). * test(coverage): drop unused .map_err closure in warmup_prover The previous shape `.map(|_| ()).map_err(|e| anyhow!("...{e}"))` left two never-called closures in the happy-path test, costing the 100% function + 100% line coverage gate. `?` propagation matches what `prove_initial` already returns (`anyhow::Result<Proof>`) and is covered by the same single test. --------- Co-authored-by: TaprootFreak <142087526+TaprootFreak@users.noreply.github.com>
github-actions
Bot
added
the
ci:full
…set gates (#167) Two structural changes to the heavy CI lane, each preserving 100% coverage gate strictness and the existing test set. 1. Merge `node-tests` and `coverage` into a single `test-and-coverage` job. The previous topology ran the same `-p node -p shared --all-features` test set TWICE on the same self-hosted m3-ultra pool (once plain, once under `cargo llvm-cov nextest`). The instrumented run already produces the test execution AND the coverage data, so the standalone `node-tests` job was pure duplication. After the merge: * Heavy gate still runs `cargo llvm-cov nextest … --fail-under-lines 100 --fail-under-functions 100` — same strictness, same ignore regex, same `-E 'not binary(api_remote)'` exclusion, same `RUSTFLAGS=--cfg coverage_nightly`. * Test set is `-p node -p shared --all-features` (matching the former `node-tests` scope); the coverage scope stays `-p node` via an additional `shared/src/.*\.rs$` entry in the ignore regex. * One m3-ultra agent slot is occupied per PR instead of two, directly reducing the Colima / Postgres-container pressure on dfx01 that produced sporadic `PoolTimedOut` flakes when multiple PRs ran the heavy lane in parallel. 2. Add two new subset-gate jobs for faster developer iteration: * `db-tests` — gated by the new `ci:db` label. Runs the Postgres-backed test surface (db / state / job_store / audit / username / r2_probe / publisher / runtime / commitment / the jobs-API router subset / the build_network_config crate-root tests / the persist+load account_node tests) under plain `cargo nextest run` without llvm-cov instrumentation. ~15 min. * `prover-tests` — gated by the new `ci:prover` label. Runs the full account_node send / mint / receive surface (Plonky2 happy paths and pure-Rust error paths) so anything touching account-node state transitions is covered. ~25 min. Both subsets carry an `&& !contains(... labels.*.name, 'ci:full')` guard so a PR labeled with both runs only the heavy gate (the superset). Both run on the same `[self-hosted, m3-ultra]` pool with the same env block, sccache config, DOCKER_HOST step, and Telegram-alert step as the heavy job. They are NOT a pre-merge gate; `ci:full` remains the authoritative check. Job topology now: lint-and-build → db-tests / prover-tests / test-and-coverage (parallel, all `needs: lint-and-build`) → notify-failure (needs: lint-and-build + test-and-coverage) All filter expressions consistently `^`-anchored to the module root to disambiguate from `<other>::tests::<same-prefix>` collisions.
…rfaces as 50k delta (#168) `balance_from_account_blob` previously read only `Account.balance`, which is the *settled* balance after sends. The mint and receive paths push the credited coin into `coin_queue` without touching the `balance` field — `Account::get_balance()` is the only call that sums both. Reading just `balance` here made every first-mint history row collapse to `new_balance = 0, prev_balance = 0, amount = 0`, so the wire item reported `amount = 0` for a 50_000-sat credit. The existing unit test masked this because its fixture set `a.balance = 5_000` directly, a shape no production caller produces on the mint or receive path. That test now documents that it pins the settled-balance variant (a valid post-send shape), and a new sibling test `history_row_to_item_balance_from_coin_queue_only` in `account_node_tests` walks the real mint flow (`execute_send_coins` + `receive_coin`) to pin the previously- uncovered queue-only case end to end — including a direct assertion on `balance_from_account_blob` itself. E2E (api_remote::history_after_mint_records_mint_row) flagged this against dev-api on PR #166 (Release develop->main).
Mirror of zk-coins/app#151 — the GITHUB_TOKEN that opens these PRs hits GitHub's anti-recursion policy and silently skips ci.yaml, leaving every staging-bound and develop-bound auto-PR without a pre-merge CI gate. Creating as DRAFT lets the operator's explicit `gh pr ready` toggle fire the `ready_for_review` event that IS allowed to trigger downstream workflows, so the full Lint & Build plus (with `ci:full` already applied at creation) Node + Shared Tests + Coverage Gate run against the actual PR HEAD before merge. Both workflows in this repo carry the same one-line addition: - auto-release-pr-staging.yaml (staging → develop) - auto-release-pr.yaml (develop → main, keeps ci:full label) Operator UX: one extra click. `gh pr ready <num>` (or the UI button) promotes the PR + runs CI in a single step.
…election (#172) lnurlp_handler picks http:// for hosts containing 'localhost' and https:// otherwise. The https arm is already pinned by lnurlp_known_address_returns_pay_request; the http arm (router.rs:2647) was uncovered, which broke the 100%-line coverage gate at 3265/3266 lines = 99.97%. New test lnurlp_localhost_host_returns_http_callback issues the same .well-known/lnurlp/<prefix> request the existing test does, but with Host: localhost:8080, and asserts the callback URL starts with http://localhost:8080/. Closes the 1-line gap without changing production code. Surfaced as part of the dfxai runner pool smoke test on PR #169 — see DFXServer/server commit 4347a4a for the new dfxai CI host.
Promote: staging -> develop
2 tasks
…174) Post-deploy smoke test was checking `/api/info` for HTTP 200, but after #154 the node binds the HTTP listener BEFORE the Plonky2 prover warmup completes — `/api/info` returns 200 within seconds while `/health/ready` stays at `{"ready":false,"prover":"warming"}` for the 10-30 s warmup window. Downstream jobs (API E2E preflight against `/health/ready` + `/health/publisher`) raced the warmup: the E2E job picked the runner up ~4 s after the deploy job reported success, hit `/health/ready` once, got back the warming snapshot, and failed with `::error::/health/ready not ready` — observed empirically on Release PR #166's run https://github.com/zk-coins/node/actions/runs/26793933906/job/78986599030. Switch the smoke loop to `/health/ready` + a `jq '.ready == true'` assertion, keeping the 30-attempt × 10-s budget (~5 min) so a genuine bootstrap stall still surfaces with the same timeout behaviour. The deploy job now only reports success once the node is actually ready for traffic, which removes the race the E2E preflight was tripping over. `/api/info` is no longer a deploy-success signal. The E2E preflight retains its explicit `/health/ready` + publisher-wallet gate as a sanity check (still a single shot — it relies on the smoke test having already enforced readiness). The deploy job runs on `ubuntu-24.04-arm` where `jq` is part of the default GitHub-hosted image; no install step needed here.
…174) (#175) Post-deploy smoke test was checking `/api/info` for HTTP 200, but after #154 the node binds the HTTP listener BEFORE the Plonky2 prover warmup completes — `/api/info` returns 200 within seconds while `/health/ready` stays at `{"ready":false,"prover":"warming"}` for the 10-30 s warmup window. Downstream jobs (API E2E preflight against `/health/ready` + `/health/publisher`) raced the warmup: the E2E job picked the runner up ~4 s after the deploy job reported success, hit `/health/ready` once, got back the warming snapshot, and failed with `::error::/health/ready not ready` — observed empirically on Release PR #166's run https://github.com/zk-coins/node/actions/runs/26793933906/job/78986599030. Switch the smoke loop to `/health/ready` + a `jq '.ready == true'` assertion, keeping the 30-attempt × 10-s budget (~5 min) so a genuine bootstrap stall still surfaces with the same timeout behaviour. The deploy job now only reports success once the node is actually ready for traffic, which removes the race the E2E preflight was tripping over. `/api/info` is no longer a deploy-success signal. The E2E preflight retains its explicit `/health/ready` + publisher-wallet gate as a sanity check (still a single shot — it relies on the smoke test having already enforced readiness). The deploy job runs on `ubuntu-24.04-arm` where `jq` is part of the default GitHub-hosted image; no install step needed here. Co-authored-by: TaprootFreak <142087526+TaprootFreak@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Automatic Release PR
Commits: 4 new commit(s)