Repository navigation
feat(core): type a wallet balance shortfall as Error::InsufficientFunds (#152) - #224
Nic-dorman wants to merge 3 commits into
Conversation
f6fc3ab to
795606a
Compare
…ds (#152) A wallet without enough payment tokens failed with the generic Error::Payment(String), so consumers (the mobile FFI/SDK, the CLI) could only tell "top up your wallet" from any other payment failure by matching message text. Since #152 was filed, file and data uploads moved to evmlib's journaled payment API, which returns InsufficientTokensForQuotes already rendered as a String, and ant-protocol re-exports neither evmlib::wallet::Error nor its variant (and ant-core may not depend on evmlib directly). So the variant cannot be matched by type on any path; evmlib's report text is the contract. Error::from_payment_failure parses its have/need amounts wherever the report sits in the message and is used at every wallet-backed payment site: the journaled native path, wave-batch and Merkle batch payment, the non-native upload adapter, and single-chunk payment (whose caller no longer re-wraps the typed error). The e2e_native_payment Anvil test now asserts InsufficientFunds { have: 0, need: 100 } for an unfunded wallet, pinning evmlib's wording through the real path: with the old mapping it fails with the Payment string. classify_error treats the new variant as an application error. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
795606a to
388c381
Compare
MaidSafe-Hermes
left a comment
There was a problem hiding this comment.
REQUEST_CHANGES — #224 at head 388c3814e08dad8e713affbedb9579015ba98db9.
Coverage: 2/2 independent seats reviewed this head — sol-1 (gpt-6.1-sol, xhigh) returned UNCERTAIN; glm-1 (z-ai/glm-5.3) returned CHANGES_REQUIRED. The material objection below is source-confirmed; the seats did not agree.
Finding — ant-core/src/data/client/upload.rs, upload_unique_records_inner (the match combining preparation and storage results): when a later payment preparation returns Error::InsufficientFunds while storage of already-paid batches also fails, the PartialUpload arm keeps the payment error only as formatted reason text. The other both-fail arm wraps it as Error::Payment. In either case the caller loses the typed InsufficientFunds { have, need } cause and must parse display text, defeating the API's stated top-up use case for multi-batch partial uploads. This path is reachable because the function runs store_upload_merkle after preparation fails, to handle already-paid batches; the PR itself documents the PartialUpload case.
Please preserve a structured insufficient-funds cause when returning PartialUpload (while retaining the partial spend and outer partial-upload status), and add a regression test for insufficient funds on a later batch combined with a storage failure. Cover the ordinary both-fail branch as well if it remains reachable.
Other gates: all reported GitHub checks are green. Locally, cargo test --locked -p ant-core --lib data::error::tests -- --nocapture passed (23 tests). The focused e2e_native_payment test could not start locally because the Anvil executable is absent (SpawnFailed: No such file or directory); CI reports the E2E jobs passing. The public data::Error enum addition is source-breaking because it is not #[non_exhaustive]; the PR discloses this and marks it breaking.
`PartialUpload` gains `cause: Option<Box<Error>>`, the typed error that stopped the upload before every chunk was tried (for example `InsufficientFunds` on a later batch). Three places that flattened it to `reason` text now keep it: - a failed payment preparation combined with a partial storage of the already-paid batches (`payment_and_storage`); - the same with any other storage error, which became `Error::Payment` text; - the upload-level wrapper that turns an error after spend into a `PartialUpload` (`after_spend`), with `upload_records` carrying the field through. Storage-failure constructors set `cause: None`. `reason` and the CLI output are unchanged. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Addressed in d36eb64. Finding: the typed shortfall is lost when a partial upload is returned. Three places used to flatten the error to text. They now keep it typed:
The combining logic now lives in two small functions,
All other Compatibility: adding the field breaks code that builds
There is no new E2E test. It would need a wallet that is funded for the first Merkle batch and then runs short, combined with a storage failure in the same upload. The devnet tests can't set that up reliably, so the combining logic is unit-tested directly instead. Verified (Linux, dev2, stable,
CI on d36eb64: all jobs pass except Security Audit. The 6 new tests pass on ubuntu and macOS, and the lib passes 877/877 on both. Security Audit: this job fails only on RUSTSEC-2026-0387 (crypto-bigint 0.5.5, reached through alloy/k256 and webrtc/dtls). The same failure is on |
MaidSafe-Hermes
left a comment
There was a problem hiding this comment.
APPROVE — focused re-review of #224 at head d36eb644183381a412e4da19d1e757e00762f8f6, limited to the fix-up after prior review 388c3814.
The prior blocker is resolved. payment_and_storage now preserves the payment error as PartialUpload::cause in both combined-failure arms, while retaining stored/failed chunk counts, spend, and reason text. after_spend carries the cause through the upload-level partial wrapper, and the retry/result-expansion path plus Error::duplicate() preserve it. The new tests cover both combined failures, prior spend, passthrough, and cause preservation. I found no remaining material blocker in the reviewed delta.
Coverage: 2/2 focused independent seats reviewed this delta — sol-1 (gpt-6.1-sol, xhigh) and glm-1 (z-ai/glm-5.3) both returned APPROVE. This was a focused fix re-review, not a fresh review of the entire PR.
Verification: current-head cargo fmt --all -- --check passed; cargo test --locked -p ant-core --lib data::client::upload::tests -- --nocapture passed (6 tests); GitHub CI Unit Tests, Clippy, Format, builds, E2E, Merkle E2E, WASM/browser and PR checks pass. The local full ant-core lib command exceeded the local tool's 420-second limit, so I do not claim a local full-suite pass.
Separate CI note: Security Audit fails on RUSTSEC-2026-0387 (crypto-bigint 0.5.5). The Cargo.lock object is identical at the base, prior head and current head; the same audit command also fails at base on that advisory. The repository ruleset requires only linear-link and pr-template, both passing. Treat this as an inherited audit issue, not a blocker introduced by this PR. The new public PartialUpload::cause field is source-breaking for downstream constructors/exhaustive patterns; the changelog documents this impact.
Approval is not a merge authorization.
#238 rotated the old [Unreleased] into the 0.3.7 to 0.3.12 sections, so this PR's entry moves to the new, empty [Unreleased], under the same heading it had before. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Fixes #152. Upstream half of V2-601 (mobile SDK).
A wallet-backed payment with too few payment tokens failed with the generic
data::Error::Payment(String). Consumers such as ant-ffi, the mobile SDK and the CLI could only tell "top up your wallet" apart from any other payment failure by matching message text.Why it isn't the fix #152 describes
#152 proposed matching
evmlib::wallet::Error::InsufficientTokensForQuotes(have, need)by type before the generic stringify. That's no longer possible on any path:native_payment.rs→Wallet::prepare_payment). It returnsResult<_, String>and already calls.to_string()on that variant (evmlib-0.10.0/src/wallet/journal.rs:214).evmlib::wallet::Errornor its variant, onlyPayForQuotesErrorandWallet. ant-core's Cargo.toml forbids a direct evmlib dependency, so even the paths that still hold the typed error can't name it.That leaves evmlib's report text as the only contract, so the fix pins it with a test.
Change
data::Error::InsufficientFunds { have: Amount, need: Amount }(atto). Display:insufficient funds: wallet has {have} atto, payment needs {need} atto.Error::from_payment_failure(String)parses evmlib'sInsufficient tokens to pay for quotes. Have: … atto, need: … attowherever it appears in the message (callers prefix context). Anything else staysPaymentwith the message verbatim, and truncated or malformed reports are not guessed at.native_payment.rs, used by file and data uploads)pay_for_quotes(batch.rs×2)pay_for_merkle_tree(merkle.rs)upload.rs×2)payment.rsnow passes the typed error through instead of re-wrapping it asPayment("On-chain payment failed: …")classify_errortreats it asApplicationError, so the adaptive limiter isn't pushed down. The exhaustiveness guard and the covers-all-variants table are extended.PartialUpload, so the spend and the stored chunks are still reported. The error is kept typed in the newPartialUpload::causefield (Option<Box<Error>>). That also holds when storing the already-paid Merkle batches fails as well.reasonkeeps the text, so the CLI output is unchanged.Payment, as noted out of scope in Payment errors collapse insufficient-funds into generic Error::Payment(String) — add a typed InsufficientFunds variant #152.Linear issue
Closes V2-1507
Risk tier
Client-side error typing only. The payment flow, amounts, wire messages and node interaction are unchanged.
Compatibility
ant_core::data::Error, which is not#[non_exhaustive].matchmust add an arm.PartialUploadgains acausefield, so code that builds it, or destructures it without.., must add it. The CLI, ant-ffi and antd'sFromimpl all match with.., so they are unaffected. antd's testcore_partial_upload_is_not_retryable_and_says_re_preparebuilds one, so it needscause: Noneat antd's next ant-core bump.ant-sdk/ffi/rust/ant-ffi/src/lib.rsFrom<ant_core::data::Error>) has another =>catch-all, so it still compiles. But until it adds an arm, a shortfall maps toClientError::InternalErrorwhere it used to map toPaymentError. That arm (→ a newClientError::InsufficientFunds) is V2-601's follow-up.payment error: Insufficient tokens to pay for quotes. Have: …toinsufficient funds: wallet has … atto, payment needs … atto.Semver impact
Test evidence
Review fix
d36eb64(2026-10-27): the shortfall stays typed inPartialUpload::cause. Run on Linux (dev2, stable,--locked), on388c3814plus this commit:cargo fmt --all -- --checkandcargo clippy --all-targets --all-features -- -D warnings: pass.data::client::upload::tests(6), thedata::errortests and the settlement-refusal test: 30/30. The wholeant-corelib: 877 passed, 0 failed.e2e_native_payment(Anvil): 4/4, includinginsufficient_funds_then_funding_unblocks_native_payment.ant file downloadof the canary file (3cba93c8…, 5,277,908 B): pass, and the byte count matches.--no-merkle:File upload failed: insufficient funds: wallet has 0 atto, payment needs 220115689453125000 atto--merkle:File upload failed: insufficient funds: wallet has 0 atto, payment needs 288523605468750000 attoRebased 2026-10-23 onto
mainat50bc653(0.3.12 promoted), 66 commits newer. Two changes came with it:CHANGELOG.mdconflicted with the new[Unreleased]entries; both sides were kept.mainhas since added an exhaustiveError::duplicate()(shared in-flight reads, perf(read): share in-flight chunk fetches across concurrent reads #218), so it gains anInsufficientFunds { have, need }arm.Every direct wallet payment call site on the new
mainstill maps errors throughError::from_payment_failure. Re-run on Linux (dev2, stable):cargo fmt --all -- --check,cargo clippy --all-targets --all-features -- -D warningsand the browser-wasm clippy: pass.cargo test -p ant-core --lib data: pass.e2e_native_payment: 4 passed, includinginsufficient_funds_then_funding_unblocks_native_payment.Original evidence follows.
Real-chain guard:
e2e_native_payment::insufficient_funds_then_funding_unblocks_native_payment(Anvil) now assertsErr(InsufficientFunds { have: 0, need: 100 })for an unfunded wallet through the real journaled path. With the oldmap_err(Error::Payment)restored, it fails withexpected InsufficientFunds, got Err(Payment("Insufficient tokens to pay for quotes. Have: 0 atto, need: 100 atto")). That confirms both evmlib's live wording and that the test catches a regression.cargo test -p ant-core --features native,test-utils --test e2e_native_payment: 4/4 pass.Unit tests:
have/needaboveu64::MAXto prove the U256 round-trip.Paymentverbatim.classify_error_covers_all_variants.cargo fmt --all -- --check✓ ·cargo clippy --all-targets --all-features -- -D warnings✓ ·cargo test -p ant-core --lib: 804 passed ·cargo test -p ant-cli: 30 passed. All run on Windows 11.Prod compat smoke on mainnet (2026-10-23), rebased head
388c3814againstmainccd4106d. Run on Linux (dev2, stable 1.99.1) with--lockedrelease builds.The wallet is a freshly generated key, and
ant wallet balancereports0. It holds neither tokens nor gas, so nothing can be paid.Uploading a random 3 MiB file (3 chunks) collects real mainnet quotes and then fails at payment:
main--no-merkleFile upload failed: payment error: Insufficient tokens to pay for quotes. Have: 0 atto, need: 228374654296875000 attoFile upload failed: insufficient funds: wallet has 0 atto, payment needs 228374654296875000 atto--merkleFile upload failed: payment error: Insufficient tokens to pay for quotes. Have: 0 atto, need: 185711296875000000 attoFile upload failed: insufficient funds: wallet has 0 atto, payment needs 294520417968750000 attoBoth builds exit 1 before sending any transaction. The
needamounts differ between runs because quotes vary.This confirms that evmlib's wording on the live network still matches what
from_payment_failureparses, on both payment paths.A mainnet
ant file downloadof a public canary file (3cba93c8…, 5,277,908 B) with this build also succeeded, and the byte count matches.New dependency
none
ADR
n/a
Mitigation / rollback
Revert the commit. Consumers that already match
InsufficientFundsgo back to seeingPayment(String).🤖 Generated with Claude Code