fix: Merkle batch history, real WASM CI gate, full deploy coverage, correct rollback - #1031
Merged
martinzhames merged 4 commits intoSep 1, 2026
Conversation
…ifiable Closes dupdab#1027. `submit_merkle_root` wrote each new batch to the single `DataKey::CurrentBatch` key, overwriting the previous one, and `verify_settlement` only ever read that key. A proof generated for a payment reconciled in an earlier cycle became permanently unverifiable the moment the next batch landed, even though it was validly included at the time — leaving auditors and dispute flows with no on-chain way to check a settlement older than one cycle. Every batch is now also stored under `DataKey::Batch(id)`, with `submit_merkle_root` returning the assigned ID and the submission event carrying it so an indexer can ask for the root again later. Batches go to persistent storage rather than instance storage: instance storage is bounded and shares a single TTL, so an unbounded history there would eventually stop the contract working. The `CurrentBatch` pointer stays where it was. `verify_settlement_proof(batch_id, payment_id, proof)` verifies against the root the proof was actually generated for, and returns `true` when the proof is valid — the opposite of `verify_settlement`, whose inverted return the issue flags as a footgun. An unknown batch ID panics rather than returning false: a missing root is not the same as an unsettled payment, and collapsing the two would let a caller read one as the other. `verify_settlement` keeps both its current-batch-only behaviour and its inverted return so existing integrators are unaffected; its doc comment now states both plainly and points at the replacement. Both verifiers share one `compute_root` helper so they cannot drift apart. Also adds `get_batch`, `get_latest_stored_batch` and `batch_count` for addressing the history, and tests covering ID assignment, retention across cycles, an old proof verifying after a newer batch lands, both polarities of the new verifier, the unknown-batch panic, and `get_current_batch` still tracking the newest submission.
Closes dupdab#1028. The final two steps were leftover boilerplate from a different project: `cargo wasm-build` is not a real subcommand — no such alias exists in `dabdub_contracts/.cargo/config.toml` or any Cargo.toml — and neither `contracts/paylink` nor `contracts/cheese_pay` is among this workspace's 14 contracts. Nothing in CI built or validated WASM for any real contract. More consequentially, `ci.yml` never ran `make check-wasm-size`, so the documented 64 KB budget was enforced only by `deploy.yml` on the `push: main` and tag paths. A pull request growing a contract past the Soroban deploy limit was not caught until after it merged. The build now runs `make check-wasm-size`, which builds every workspace contract, runs `wasm-opt -Oz`, and fails on any contract over the limit — the same gate `deploy.yml` applies, moved to where it can block a merge. binaryen is installed via apt, matching deploy.yml. The uploaded artifact now points at the real output directory, `dabdub_contracts/target/wasm32v1-none/release/*.wasm`. Two fixes beyond the stale steps, both needed for the above to run at all: the cargo workspace lives in `dabdub_contracts/` and there is no manifest at the repository root, so `cargo test` and `cargo tarpaulin` were also running from the wrong directory — they now carry `working-directory`, and the Codecov `files:` path is corrected to match. The Codecov `name:` and the `LLVM_PROFILE_FILE` prefix, both still naming the other project, are renamed to dabdub.
Closes dupdab#1029. Both deploy jobs built and size-checked WASM for the whole workspace but only called `deploy_contract` for `admin_timelock`, `merchant_registry` and `payment_escrow`. The other 11 — including `settlement_ledger`, `multisig_admin` and `fee_calculator`, no less security-relevant than the three that were automated — needed a manual, undocumented deploy with no record of which contract ID came from which commit or WASM hash. All 14 are now deployed, with outputs and the step-summary table covering every one. Ten of them take `__constructor` arguments, which the workflow never passed — even for `admin_timelock` and `merchant_registry`, both of which take an admin. Arguments now come from per-network Actions variables, forwarded through `deploy_contract` after `--`. The step checks every required variable up front and fails naming the missing one, rather than letting an opaque CLI error surface partway through a partial deploy. `payment_escrow` takes an `Option<Address>` registry; it defaults to the `merchant_registry` deployed in the same run, so the two are wired together in one pass, and `REGISTRY_ADDRESS` overrides that to point at an existing registry. Deploy order puts constructor-free contracts first, then admin-only, then multi-argument, so the registry exists before the escrow needs it. `docs/deployment_configuration.md` lists every required variable, which contract consumes it, and the JSON shapes for the two container-typed arguments (`emergency_signers`, `fee_tiers`). Note for whoever merges: deploy.yml will fail on its next run until these variables are configured in repository settings. That is deliberate — the alternative is deploying ten contracts with unset admin addresses.
Closes dupdab#1030. The rollback reminder told operators to run `stellar contract deploy` with the previous WASM. That installs the WASM and creates a brand-new contract with a new ID and empty storage — it does not touch the live contract. An operator following it verbatim during an incident would stand up an orphaned, unpopulated copy while the contract holding every open escrow, the admin address and the registry pointer kept running the bad code, with every downstream service still pointed at it. Not a rollback at all. The reminder now describes what actually works for `payment_escrow`: `stellar contract install` to get the previous WASM hash, then `stellar contract invoke ... -- upgrade --caller <admin> --new_wasm_hash <hash>` against the live contract ID, which calls `update_current_contract_wasm` and preserves all storage. It notes that `upgrade` is admin-only and needs multisig signatures where the admin is a multisig, and that `get_version` counts upgrades rather than naming a code version — it increments on a downgrade too, so the on-chain WASM hash is what to verify against. It also states the limitation the old text hid: `payment_escrow` is the only contract here that implements `upgrade`. The other 13 have no `update_current_contract_wasm`, so rolling one back means deploying a new contract and repointing every reference to the old ID — the registry pointer via `set_registry`, the NestJS backend's configured addresses, anything else holding it — with no storage carried over. That is a migration, and the text says so. EMERGENCY_RUNBOOK.md gains the same procedure so the two documents agree, each cross-referencing the other and docs/escrow_upgrade_procedure.md.
|
@echoripplestudio Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
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
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.
Bundles the four issues assigned to me in the Stellar Wave Program. Each lands as its own commit so they can be reviewed — or reverted — independently.
Closes #1027
Closes #1028
Closes #1029
Closes #1030
#1027 — Reconciliation kept only the latest Merkle batch
submit_merkle_rootwrote each batch to the singleDataKey::CurrentBatchkey, overwriting the previous one, andverify_settlementonly ever read that key. A proof for a payment reconciled in an earlier cycle became permanently unverifiable the moment the next batch landed — leaving auditors and dispute flows with no on-chain way to check a settlement older than one cycle.Every batch is now also stored under
DataKey::Batch(id).submit_merkle_rootreturns the assigned ID and the submission event carries it, so an indexer can ask for that root again later.Batches go to persistent storage rather than instance storage: instance storage is bounded and shares one TTL, so an unbounded history there would eventually stop the contract working. The
CurrentBatchpointer stays where it was.verify_settlement_proof(batch_id, payment_id, proof)verifies against the root the proof was actually generated for, and returnstruewhen the proof is valid — the opposite ofverify_settlement, whose inverted return the issue flags as a footgun. An unknown batch ID panics rather than returningfalse: a missing root is not the same as an unsettled payment, and collapsing the two would let a caller read one as the other.Also adds
get_batch,get_latest_stored_batchandbatch_count.#1028 — CI built an unrelated project's contracts
cargo wasm-buildis not a real subcommand — no such alias exists indabdub_contracts/.cargo/config.tomlor anyCargo.toml— and neithercontracts/paylinknorcontracts/cheese_payis among this workspace's 14 contracts. Nothing in CI built or validated WASM for anything real.More consequentially,
ci.ymlnever ranmake check-wasm-size, so the documented 64 KB budget was enforced only bydeploy.ymlonpush: mainand tags. A PR growing a contract past the Soroban deploy limit was not caught until after it merged. CI now runsmake check-wasm-size— the same gate, moved to where it can block a merge — and uploads the real artifacts fromdabdub_contracts/target/wasm32v1-none/release/*.wasm. binaryen is installed via apt, matchingdeploy.yml.#1029 — Only 3 of 14 contracts were deployed
Both jobs built and size-checked the whole workspace but called
deploy_contractfor three contracts. The other 11 — includingsettlement_ledger,multisig_adminandfee_calculator, no less security-relevant than the automated three — needed a manual, undocumented deploy with no record of which ID came from which commit.All 14 are now deployed, with outputs and the summary table covering every one.
Ten take
__constructorarguments that the workflow never passed — includingadmin_timelockandmerchant_registry, two of the three it already deployed. Arguments now come from per-network Actions variables, forwarded after--. The step validates every required variable up front and fails naming the missing one, rather than letting an opaque CLI error surface partway through a partial deploy.payment_escrow'sOption<Address>registry defaults to themerchant_registrydeployed in the same run, so the two are wired together in one pass;REGISTRY_ADDRESSoverrides it. Deploy order is constructor-free → admin-only → multi-argument, so the registry exists before the escrow needs it.docs/deployment_configuration.mdlists every variable, which contract consumes it, and the JSON shapes for the two container-typed arguments (emergency_signers,fee_tiers).#1030 — Rollback guidance deployed a new contract instead of upgrading
The reminder told operators to run
stellar contract deploywith the previous WASM. That creates a brand-new contract with a new ID and empty storage. An operator following it during an incident would stand up an orphaned copy while the contract holding every open escrow kept running the bad code, with every downstream service still pointed at it.It now describes what actually works for
payment_escrow:stellar contract installfor the hash, thenstellar contract invoke ... -- upgrade --caller <admin> --new_wasm_hash <hash>against the live ID, which callsupdate_current_contract_wasmand preserves storage. It notes thatupgradeis admin-only and needs multisig signatures where applicable, and thatget_versioncounts upgrades rather than naming a code version — it increments on a downgrade too, so verify against the on-chain WASM hash.It also states the limitation the old text hid:
payment_escrowis the only contract here that implementsupgrade. The other 13 have noupdate_current_contract_wasm, so rolling one back means deploying a new contract and repointing every reference to the old ID — the registry pointer viaset_registry, the NestJS backend's addresses, anything else holding it — with no storage carried over. That is a migration, and the text says so.EMERGENCY_RUNBOOK.mdgains the same procedure so the two agree, each cross-referencing the other anddocs/escrow_upgrade_procedure.md.Testing
Per the constraints I was working under, I did not run
cargo test,cargo build, or install anything — none of this has been executed.The reconciliation change ships nine new tests covering ID assignment, retention across cycles, an old proof still verifying after a newer batch lands (the regression itself), both polarities of the new verifier, the unknown-batch panic,
get_latest_stored_batch, andget_current_batchstill tracking the newest submission. One existing test is updated for the newbatch_idfield on the submission event.Two things worth a reviewer's attention, both flagged above: the deploy variables must be configured before
deploy.ymlruns again, and#1027adds a field to a public event struct plus changessubmit_merkle_root's return type from()tou32— source-compatible for callers that ignore it, but a client-binding regeneration for anyone consuming the contract's interface.