👀 Use LBTC on wapupay PR review - #130
Open
coelhogonzalo wants to merge 6 commits into
Open
Conversation
…ency A thin cross-device record (fund_order/order_status with no local order) has funding_currency=None, so every denomination branch silently fell back to USDT semantics. If the funding response carried total_amount_usdt next to the L-BTC asset_id, pay_instructions paired a USDT-scale amount (~10^8x) with the L-BTC asset. On reload, from_dict then scrubbed the real total_amount_sats as legacy-USDT residue and kept the wrong base units. - Back-fill funding_currency from a known Liquid policy asset_id (LBTC_ASSET_ID -> LBTC, USDT_LIQUID_ASSET_ID -> USDT) in apply_tentative and in from_dict (before the legacy scrub, so real sats survive reload). - Gate the USDT pay_instructions branch on an explicit USDT rail. - When the rail stays unknown (unknown asset), refuse to name any send amount and point at order_status instead. Finding 1 (HIGH) of the PR #122 review report.
_assert_rail guarded only the funding_currency echo; asset_id — the field lw_send_asset actually spends by — was passed verbatim from WapuPay's response into pay_instructions. An L-BTC order whose funding response carried the USDT asset id told the caller to send the sat figure as USDT base units: the order never settles and funds leave in an unquoted asset. Both rails settle in a Liquid policy asset whose id is a global constant (LBTC_ASSET_ID / USDT_LIQUID_ASSET_ID), so the check is free: any other asset_id for a known rail is an upstream contract violation and raises, annotating the persisted record (funded=True) like the currency flip. Finding 2 (MEDIUM) of the PR #122 review report.
create_order raised via _assert_rail when WapuPay's echo contradicted the requested rail, but the re-issue path (fund_order) and the poll path (order_status) merged the response unchecked: a flipped echo silently re-denominated the stored record and fund_order then emitted instructions for a rail the user never chose. Both paths now run the same _assert_rail check against the rail stored before the merge (via a shared _assert_known_rail helper). Thin records with no stored rail still get the asset-consistency half of the check against the echoed rail. order_status is restructured so only the NETWORK failure degrades to the last-known-local warning — a money-contract violation in the response now raises instead of displaying. Finding 3 (LOW) of the PR #122 review report.
In create_order, the post-funding apply_tentative(funding) ran outside the try that records last_error. Its contract-violation raise (fractional total_amount_sats) left the persisted record CREATED with no last_error while funding existed upstream, and recovery via fund_order hit the same un-annotated raise forever. Both create_order and fund_order now annotate the record via a shared _annotate_rejected_response helper, mirroring _assert_rail(funded=True). The clean STORED record is annotated — the half-merged in-memory order is not saved, so a rejected response never leaves its contract-violating values on disk. Finding 4 (LOW) of the PR #122 review report.
The new strictness check on the L-BTC boundary handled only fractional
floats: a zero or negative integer sailed through into pay_instructions
('Send exactly -25127 sats of L-BTC ...'), and a string-typed value would
round-trip into storage uncoerced. The USDT rail already rejects
non-positive totals inside usdt_to_base_units, so the L-BTC seam was
asymmetrically weaker.
One shared validation now requires a positive int (whole floats coerced,
bool excluded) and raises the same contract-violation ValueError
otherwise. This also covers the string-typed hardening noted in the
review's finding 8.
Finding 5 (LOW) of the PR #122 review report.
The fallback told a USDT payer to fetch total_amount_usdt — a decimal — and 'pay that exact amount with lw_send_asset', whose amount parameter is integer base units: following it literally underpays by ~10^8x (or errors on the non-integer). Name total_funding_amount_base_units instead — the field that is directly payable and that order_status re-derives on load — mirroring how the L-BTC half already names total_amount_sats. Finding 6 (LOW) of the PR #122 review report.
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.
This PR is meant as feedback for the original PR #122
Each commit addresses a separate issue starting with the most relevant first. I ran Kimi3 and Fable to filter out useless "guards" and checks that are sometimes entertained by LLMs and I filtered out other non-issues myself.
Integration test summary with a real Argentinian alias
QA test — WapuPay review fixes (PR #122 follow-up commits)
use-lbtc-on-wapupay-pr-review5d90871,76254e0,0178bb1,57eb6b7,b2d1abf,3560762(fixes for findings 1–6 of
wapupay-report.md)WAPUPAY_BASE_URL=https://be-stage.wapu.app,local branch code directly (
uv run --directory … python), with order storage pointed atan isolated QA directory — the real
~/.aquawas not touched.the unit tests ran separately per commit. No order was ever paid — tentatives expire on
their own; no real funds moved at any point. Two passes: first with the synthetic alias
aqua.test.stage(the sandbox validates any alias), then a full re-run with the realalias
matriz.menudo.saber(§10).Result summary
exchange_rates, prod + stagingspending_limit+quote(with and without alias)create_orderUSDT rail — pay_instructions, asset id, base unitscreate_orderLBTC railfund_orderfrom_dict)order_statusrefresh + flags on the rebuilt record (finding 3 path)FUNDING_ISSUED6+ min after the window closed (see §8)matriz.menudo.saber): quote, USDT order, L-BTC retry, thin-record recovery, reload, statusEvidence
1. Public rates — both hosts reachable through the branch code
2. Keyed read-only calls
3. USDT order — created, funded, correct instructions
create_order(amount_ars="10000", alias="aqua.test.stage", transfer_type="fiat_transfer", funding_method="USDT")→ tentative1e1df359-b959-4e7f-84ce-3e689aaf542d:Checks that this proves live:
asset_idis the USDT policy constant → commit76254e0's asset-vs-rail checkpassed on the happy path (a wrong asset would have raised).
None, no sats, no L-BTC wording on the USDT rail.4. L-BTC order — upstream unavailable on staging (not a code failure)
create_tentative, before persistence.Verified right after: the QA orders directory contained only the USDT order file
(
persisted order files: ['1e1df359-….json']).behavior is covered by the unit tests (§9) and by the PR author's earlier live-stage
verification. Re-run this section when WapuPay re-enables the rail on staging.
5. Thin-record recovery (finding 1) — delete the record, re-fund
The local record was deleted, then
fund_orderran against staging with no local state:Wire observation: the staging funding response does echo
funding_currency(
raw funding_currency: USDT), so the rebuilt record took the rail from the echo, andthe asset-id inference branch added in
5d90871did not need to fire live. The inferencebranch (rail omitted) and the unknown-rail refusal stay covered by unit tests (§9).
The response also carries both
expires_atandfunding_expires_at, confirming thePR #129 rename (
expires_at) is wire-compatible.6. Reload round-trip
The rebuilt record survives
from_dictwith the correct rail and amounts.7.
order_statuson the rebuilt recordThe poll path (restructured in
0178bb1) refreshed and persisted the record with amatching rail echo — no false raise, no warning.
8. Expiry → terminal state (upstream behavior finding)
The order was never paid; its funding window closed at
2026-08-16T11:09:55Z. Aonce-per-minute poll ran for 14 minutes and the terminal flip was not observed:
Finding about the upstream, not the code: WapuPay staging marks expiry
asynchronously — the tentative still reports
FUNDING_ISSUEDmore than 6 minutes afterexpires_at. Callers must not assumeEXPIREDappears at the deadline; treatexpires_atas "do not pay after this", not as "status flips at this". The local flaglogic (
order_is_final/order_is_failedmappingEXPIRED→ final+failed) isunit-covered; re-check the live flip later with
order_status 1e1df359-b959-4e7f-84ce-3e689aaf542dif desired.9. Guard paths — unit-test proof (cannot be triggered against a real API)
Findings 2–5 fire only when WapuPay violates its own contract (flipped rail, wrong asset
id, fractional/zero/negative/string sats). The real staging API answers correctly, so
these are proven by the tests added with the fixes — all passing on this branch:
Full suite on the branch: 1166 passed, 42 skipped.
10. Full re-run with a real alias
The first run used the synthetic alias
aqua.test.stage(the sandbox accepts any alias).The whole live sequence was repeated with the user-provided real alias
matriz.menudo.saber— again never paid:Same results as the synthetic-alias run on every check, now against a real recipient
alias. The L-BTC rail stays blocked upstream (identical 400).
Conclusion
Everything that can be exercised against the live staging API passes on the fixed branch,
in both the synthetic-alias and the real-alias (
matriz.menudo.saber) runs: keyed reads,USDT order creation with correct integer-base-unit instructions, the finding-1
thin-record recovery, reload round-trips, and status polling. The live expiry flip was
not observed — staging marks expiry with a multi-minute lag (§8), an upstream behavior
worth knowing, not a code defect.
The one open live gap is the L-BTC rail itself — currently disabled upstream on staging
(clean 400, no local side effects). The contract-violation guards (findings 2–5) are, by
nature, only provable with injected malformed responses and are green in the unit suite.
Follow-up when WapuPay re-enables LBTC on staging: repeat §3–§8 with
funding_method="LBTC"and verifypay_instructionsquotestotal_amount_satssats withasset_id=6f0279e9…526d, andtotal_funding_amount_base_unitsstaysnull.