Skip to content

Rounding and billing fix#90

Open
RuoHan-Chen wants to merge 9 commits into
mainfrom
roundingAndBillingFix-contract-only
Open

Rounding and billing fix#90
RuoHan-Chen wants to merge 9 commits into
mainfrom
roundingAndBillingFix-contract-only

Conversation

@RuoHan-Chen

@RuoHan-Chen RuoHan-Chen commented Jun 22, 2026

Copy link
Copy Markdown
Collaborator

Summary

Replace basis-points-encoded fees (feeBps) with an absolute feeAmount (in raw token units) on capture() and charge(). The payer-approved minFeeBps / maxFeeBps on PaymentInfo are unchanged and continue to bound the fee, but they are now applied to the operator-supplied absolute amount rather than used to compute it:

minFee = amount * minFeeBps / 10_000
maxFee = amount * maxFeeBps / 10_000
require(minFee <= feeAmount <= maxFee)

This lets operators pass cent-aligned fees computed off-chain without losing precision to on-chain BPS truncation, while preserving the same payer-side guarantees.

Design doc: https://docs.google.com/document/d/1RCIXH0Q46XcwyYcu3027g9bKXHljrEX-y_ciBIdGNIQ

Implementation follows the approach used in PPS.

Changes

Contracts

  • AuthCaptureEscrow.capture(...) and charge(...): uint16 feeBpsuint256 feeAmount.
  • _validateFee now takes amount + absolute feeAmount and checks it against the min/max bounds derived from bps.
  • _distributeTokens uses the supplied feeAmount directly (no on-chain multiplication/division).
  • Error FeeBpsOutOfRange(feeBps, minFeeBps, maxFeeBps)FeeAmountOutOfRange(feeAmount, minFee, maxFee).
  • PaymentCharged / PaymentCaptured events: uint16 feeBpsuint256 feeAmount.
  • Collector call sites (ERC3009PaymentCollector, Permit2PaymentCollector) updated to the new signature.

Docs

  • docs/Fees.md, docs/operations/Capture.md, docs/operations/Charge.md rewritten to describe absolute-fee semantics with worked examples and updated error list.

Tooling

  • Added [profile.ci] to foundry.toml.

Testing

Tests updated across the suite to pass absolute feeAmount values instead of bps:

  • test/src/PaymentEscrow/{authorize,capture,charge,refund,reentrancy,e2eCoinbaseSmartWallet}.t.sol
  • test/src/collectors/SpendPermissionPaymentCollector.t.sol
  • test/gas/gasBenchmark.t.sol
  • test/base/AuthCaptureEscrowBase.sol, test/base/AuthCaptureEscrowSmartWalletBase.sol

FeeAmountOutOfRange bounds cases (below-min, above-max, and both-zero) are exercised in capture.t.sol / charge.t.sol.

Backwards compatibility

Breaking public ABI change: charge(), capture(), PaymentCharged, and PaymentCaptured all change parameter/field types. Integrators must migrate before upgrading.

@cb-heimdall

cb-heimdall commented Jun 22, 2026

Copy link
Copy Markdown
Collaborator

🟡 Heimdall Review Status

Requirement Status More Info
Reviews 🟡 0/1
Denominator calculation
Show calculation
1 if user is bot 0
1 if user is external 0
2 if repo is sensitive 0
From .codeflow.yml 1
Additional review requirements
Show calculation
Max 0
0
From CODEOWNERS 0
Global minimum 0
Max 1
1
1 if commit is unverified 1
Sum 2

@ilikesymmetry ilikesymmetry left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Did not review tests, but looks like a relatively simple modification. Good principle learned: prefer custom net-amount args as calldata with math + checks against constraints versus passing in fields that derive the net-amount and restrict rounding preferences.

Comment thread src/AuthCaptureEscrow.sol Outdated
Comment thread src/interfaces/IMulticall3.sol

// Pull tokens into this contract
IERC3009(token).receiveWithAuthorization({
IERC3009(token)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

are you on the latest version of foundry? wonder if there's a way to remove some of this lint thrashing... not blocking

Comment thread src/AuthCaptureEscrow.sol
/// @param feeAmount Absolute fee in token units (must fall within payer-approved bounds)
/// @param feeReceiver Address to receive fees (should match the paymentInfo.feeReceiver unless that is 0 in which case it can be any address)
function capture(PaymentInfo calldata paymentInfo, uint256 amount, uint16 feeBps, address feeReceiver)
function capture(PaymentInfo calldata paymentInfo, uint256 amount, uint256 feeAmount, address feeReceiver)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

one thing that came up in our original call was the question as to whether the new version of the AuthCaptureEscrow should retain its old interface too. I.e. should have have a net-new version of capture that offers the feeAmount while also offering a version with feeBps in case we want to migrate existing integrations to flow through the same AuthCaptureEscrow. A question for Fab and team. Given that we can run multiple authcaptureescrows in parallel, we would never be blocked on this, but might be good to consider one more time

@RuoHan-Chen RuoHan-Chen marked this pull request as ready for review June 25, 2026 22:48
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.

4 participants