Repository navigation
chore(x402): reject bad recovery ids, exact chain labels, drop commit_reservation - #118
Conversation
Tiny Sweeper reviewReviewed head: Verdict: Ready for maintainer review · Priority: none Diagnostics and supporting detailsFull summaryPull request purpose (title): chore(x402): reject bad recovery ids, exact chain labels, drop commit_reservation. The PR makes four x402 clean-ups across 12 files (+103/−74): `eip712_signature` refuses a secp256k1 recovery id outside 0/1 instead of only overflow-checking the `+27` offset; the paid-response chain label matches exact CAIP-2 constants before falling back to namespace labels; the dead `Reservation::commit`/`PaymentLedger::commit_reservation` path is removed in favor of `commit_durable`/`try_commit_reservation`, with tests and docs repointed and the README updated; and a paging test file is added. Review snapshot
Completeness: Complete What changedTwelve files change (+103/−74). The recovery-id range check lands in `crates/tinywallet-x402/src/crypto/evm_payment.rs`; `crates/tinywallet-x402/src/tools/request.rs` swaps prefix matching for exact CAIP-2 constants; `crates/tinywallet-x402/src/ledger/reservation.rs` and `crates/tinywallet-x402/src/ledger/store.rs` delete the lossy commit path; doc references in `crates/tinywallet-x402/src/ledger/mod.rs` and `crates/tinywallet-x402/src/protocol/client.rs` move to `commit_durable`; `crates/tinywallet-x402/README.md` no longer advertises the removed `commit_reservation`; tests are updated in `crates/tinywallet-x402/src/crypto/mod_tests.rs`, `crates/tinywallet-x402/src/ledger/mod_tests.rs`, `crates/tinywallet-x402/src/ledger/mod_holds_tests.rs`, `crates/tinywallet-x402/src/tools/mod_tests.rs`, and the new `crates/tinywallet-x402/src/ledger/mod_paging_tests.rs`. Features
Tests
FindingsNo active actionable findings. Before mergeNone. How this fits togetherflowchart LR
n0["...ed_when_a_commit_makes_the_snapshot_stale<br/>changed"]:::changed
n1["reserve_during_commit<br/>changed"]:::changed
n2["...al_reservation_committed_is_recorded_once<br/>changed"]:::changed
n3["...on_records_the_payment_and_frees_the_hold<br/>changed"]:::changed
n4["PaymentLedger<br/>changed"]:::changed
n5["...he_session_total_with_or_without_a_thread"]:::impacted
n6["...read_and_the_record_read_cannot_be_missed"]:::impacted
n7["memory"]:::impacted
n8["lock"]:::impacted
n9["reserve_at"]:::impacted
n10["record_payment"]:::impacted
n0 -->|calls| n7
n0 -->|tests| n7
n0 -->|calls| n9
n0 -->|tests| n9
n0 -->|calls| n10
n0 -->|tests| n10
n1 -->|calls| n7
n1 -->|calls| n8
n1 -->|calls| n9
n2 -->|calls| n7
n2 -->|tests| n7
n3 -->|calls| n7
n3 -->|tests| n7
n5 -->|uses| n4
n5 -->|calls| n8
n5 -->|tests| n8
n6 -->|calls| n7
n6 -->|tests| n7
n6 -->|calls| n9
n6 -->|tests| n9
n6 -->|calls| n10
n6 -->|tests| n10
n8 -->|uses| n4
classDef changed fill:#0d4429,stroke:#238636,color:#e6edf3
classDef impacted fill:#161b22,stroke:#6e7681,color:#c9d1d9
classDef flagged fill:#5a1e02,stroke:#d93f0b,color:#ffffff
classDef blocking fill:#67060c,stroke:#f85149,color:#ffffff
Agent review detailscritique
security
tests
commits
description
Evidence and run details
|
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configuration
Comment |
…_reservation - eip712_signature refuses a recovery id other than 0 or 1 instead of adding 27 to any byte up to 228. - The paid-response label matches CAIP-2 ids exactly: eip155:84532 is Base Sepolia, not Base, and eip155:10/137/... are EVM, not Ethereum. - Remove Reservation::commit and PaymentLedger::commit_reservation. They had no production callers and released the hold after a failed append; every path uses commit_durable / try_commit_reservation. - Pin ledger reload past one 512-record read window.
ledger/mod_tests.rs is past the 750-line cap; put the new paging test in a sibling mod_paging_tests.rs instead of growing it.
e137eb2 to
7940830
Compare
Summary
Four small x402 clean-ups, each with its test:
eip712_signatureturned the wallet seam'sr ‖ s ‖ recovery_idintor ‖ s ‖ vby adding 27 with only an overflow check, so any id from 2 to 228 became avthat no verifier accepts, and the payment failed later at the server instead of here. A secp256k1 recovery id is 0 or 1; anything else is now refused asrecovery id out of range.eip155:84532(Base Sepolia) started witheip155:8453and was shown as "Base", and everyeip155:1…chain (Optimismeip155:10, Polygoneip155:137, …) was shown as "Ethereum". It now matches the CAIP-2 constants exactly: Base, Base Sepolia, Ethereum, then "EVM" / "Solana" by namespace.Reservation::commitandPaymentLedger::commit_reservationhad no production callers; both payment paths usecommit_durable/try_commit_reservation, which keep the hold when the append fails. The removed path released the hold even when the record could not be written. Its tests are ported to the durable path, and the docs that pointed at it now point atcommit_durable.Related issue
None.
API or behavior changes
eip155:84532is labelled "Base Sepolia"; other non-Base, non-mainnet EVM chains are labelled "EVM".Validation
cargo fmt --all -- --check(touched files, rustfmt edition 2024)cargo clippy --all-targets --all-features -- -D warnings: left to CIcargo build --all-targets --all-features: left to CIcargo test --all-features: only the targeted tests below ran locally; CI runs the suiteTests
cargo test -p tinywallet-x402 --features tools --lib -- <names>crypto::test::the_recovery_id_is_offset_and_checkednow also checks ids 2, 3 and 27.tools::test::networks_are_labelled_for_peopleadds Base Sepolia, Optimism and Polygon.FAILED. 0 passed; 2 failed.ledger::paging_test::records_past_one_read_window_survive_a_restart(in its own file, sinceledger/mod_tests.rsis already past the line cap) (513 records, reopen, all reload).FAILED(left: 512, right: 513).committing_a_reservation_records_the_payment_and_frees_the_hold,a_global_reservation_committed_is_recorded_onceand the two hold-interleaving tests (now durable only) pass.ok. 7 passed; 0 failed.Documentation
Module and type docs that referenced the removed methods now reference
commit_durable.Checklist
#[allow(...)],#[ignore], or relaxed lints.envcontents in the diff or the description