Skip to content

Address escrow scheme spec review feedback#18

Merged
A1igator merged 10 commits intomainfrom
A1igator/address-pr-review-comments
Mar 10, 2026
Merged

Address escrow scheme spec review feedback#18
A1igator merged 10 commits intomainfrom
A1igator/address-pr-review-comments

Conversation

@A1igator
Copy link
Copy Markdown
Contributor

@A1igator A1igator commented Mar 9, 2026

Summary

Addresses reviewer feedback on the escrow scheme spec and implementation.

Spec changes

  • Network-agnostic scheme_escrow.md: Removed EVM-specific references (Commerce Payments, ERC-3009, Solidity function names). EVM details live exclusively in scheme_escrow_evm.md
  • Settlement Response: Defined PAYMENT-RESPONSE contents — includes full paymentInfo so clients can track payment state without retaining client-side state
  • Charge path clarity: Explains how refunds work (operator initiates, which may be a contract or authorized account) and the tradeoff vs authorize
  • Verification updates: Strict amount equality (=== not >=), authorization.to === tokenCollector check, settlement simulation step

Implementation changes

  • Settlement simulation: Verify now simulates operator.authorize/charge via eth_call before spending gas — catches balance issues, consumed nonces, domain mismatches, and contract errors
  • Diagnostic fallback: On simulation failure, runs balanceOf check for actionable error messages
  • Shared helper: Extracted buildPaymentInfo() used by both verify and settle

Test plan

  • All 51 tests pass (pnpm test)
  • Build succeeds (pnpm build)
  • Prettier passes (pnpm format:check)

🤖 Generated with Claude Code

A1igator and others added 2 commits March 9, 2026 03:17
- Enforce strict amount equality (=== not >=) in verification
- Add authorization.to === tokenCollector check
- Make scheme_escrow.md network-agnostic (remove EVM-specific references)
- Clarify charge path refund mechanics

Co-Authored-By: Claude Opus 4.6 <[email protected]>
Document how clients track post-settlement payment status
using nonce and escrowAddress from their original request.

Co-Authored-By: Claude Opus 4.6 <[email protected]>
@A1igator A1igator force-pushed the A1igator/address-pr-review-comments branch from e6263f5 to c8e63d9 Compare March 9, 2026 10:24
A1igator and others added 7 commits March 9, 2026 17:38
Addresses PR feedback: spec now defines what the PAYMENT-RESPONSE header
contains for escrow settlements. Includes full paymentInfo so clients can
track payment state and initiate post-settlement actions without retaining
client-side state. Network-agnostic spec defers struct details to EVM doc.

Co-Authored-By: Claude Opus 4.6 <[email protected]>
Remove EVM/commerce-payments specific language (operator, on-chain).
Clarify charge refund path: explain that refunds require a trusted party
since funds are already with the receiver, and the tradeoff vs authorize.

Co-Authored-By: Claude Opus 4.6 <[email protected]>
Simulate operator.authorize/charge via eth_call before spending gas.
Catches balance issues, consumed nonces, domain mismatches, and
contract errors. Falls back to balanceOf for actionable diagnostics.
Extract buildPaymentInfo helper shared between verify and settle.

Co-Authored-By: Claude Opus 4.6 <[email protected]>
@A1igator A1igator changed the title Address x402 PR #1425 review feedback Address escrow scheme spec review feedback Mar 10, 2026
@A1igator A1igator requested a review from vraspar March 10, 2026 01:43
@vraspar
Copy link
Copy Markdown
Contributor

vraspar commented Mar 10, 2026

Code review

No issues found. Checked for bugs and CLAUDE.md compliance.

Notes (below the review threshold, but worth considering):

  • Simulation failure is a hard reject — The old balance check was non-fatal (skipped on RPC failure, returned isValid: true). The new simulation catch block always returns { isValid: false, invalidReason: 'simulation_failed' }, meaning transient RPC errors will reject valid payments with no retry.

    })
    } catch {
    // Simulation failed — check balance for a more actionable error
    try {
    const balance = (await this.signer.readContract({
    address: requirements.asset as `0x${string}`,
    abi: ERC20_BALANCE_OF_ABI,
    functionName: 'balanceOf',
    args: [payer],
    })) as bigint
    if (balance < BigInt(requirements.amount)) {
    return {
    isValid: false,
    invalidReason: 'insufficient_balance',
    payer,
    }
    }
    } catch {
    // Balance check failed too
    }
    return {
    isValid: false,
    invalidReason: 'simulation_failed',
    payer,
    }
    }

  • Spec step 5 inconsistency — Settlement Logic step 5 says "Return result: Transaction hash, network, and payer address" but the new Settlement Response section defines paymentInfo as part of the response. Step 5 should be updated to match.

    4. **Wait for receipt**: Confirm transaction success with 60s timeout
    5. **Return result**: Transaction hash, network, and payer address

🤖 Generated with Claude Code

- If this code review was useful, please react with 👍. Otherwise, react with 👎.

vraspar
vraspar previously approved these changes Mar 10, 2026
Also clarify simulation hard-reject rationale in code comment.

Co-Authored-By: Claude Opus 4.6 <[email protected]>
@A1igator
Copy link
Copy Markdown
Contributor Author

A1igator commented Mar 10, 2026

Thanks for the review! Step 5 is fixed in 92d5b88.

Re: simulation hard reject — this is intentional. Both pending simulation PRs for the exact scheme also hard reject on any simulation failure (including transient RPC errors). The reasoning: if the RPC is down for simulation, the facilitator also can't submit the settlement transaction, so accepting the payment would just defer the failure. A false-negative (rejecting a valid payment) is safer than a false-positive (accepting one that reverts on-chain and wastes gas).


This scheme reuses audited commerce-payments contracts deployed on Base and other EVM chains.
Unlike `exact`, which transfers funds immediately and irrevocably, `escrow` supports refundable payments across both settlement paths.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, but make sure wording is what you expecting. The md file might have unnecessary information

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Acknowledged — the escrow abstract spec is longer than exact's, but escrow has more moving parts (two settlement paths, expiry tiers, fee system, post-settlement actions). I think the current content is justified. Happy to trim if you spot anything specific that feels redundant though.

@A1igator A1igator merged commit b018a2e into main Mar 10, 2026
11 checks passed
@A1igator A1igator deleted the A1igator/address-pr-review-comments branch March 10, 2026 03:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants