Skip to content

feat(validation): require a minimum refund window between the expiries - #53

Merged
pviti merged 1 commit into
v1.4.0from
feature/minimum-refund-window
Jul 31, 2026
Merged

feat(validation): require a minimum refund window between the expiries#53
pviti merged 1 commit into
v1.4.0from
feature/minimum-refund-window

Conversation

@pviti

@pviti pviti commented Jul 29, 2026

Copy link
Copy Markdown
Member

authorizationExpiry == refundExpiry was accepted — the ordering check uses >, not >= — which collapses the refund/dispute window to zero: both become unreachable the instant the authorization ends, so a payment could be created that is refundable in name only.

_validatePayment now requires refundExpiry - authorizationExpiry >= MIN_REFUND_WINDOW, a new public constant.

Days, not hours

MIN_REFUND_WINDOW = 1 days. The window has to survive a buyer noticing a problem, a merchant responding, and a transaction confirming — a window measured in hours is one the buyer can lose to a weekend. test_Validation_RejectsAnHoursLongRefundWindow asserts a 6-hour window is refused even though it is non-zero, so the unit is pinned, not just commented.

One day is a floor against a collapsed window, not a recommendation: the README's guidance stays 14–30 days aligned with consumer-protection practice.

Chosen so it cannot reject real traffic

The gateway's defaults are a 7-day authorization and a 30-day refund window — a 23-day gap, comfortably clear of the floor. The strongest evidence is that the entire existing test corpus passes unchanged; only the test that existed to lock in the old behaviour had to go.

The subtraction cannot underflow: the ordering check immediately above already guarantees refundExpiry >= authorizationExpiry.

Gas

+328 on authorize/charge (151,848 → 152,176, measured on test_Authorize_Success), and nothing on any other entrypoint — the check lives only on the creation path.

Two things this PR deliberately does NOT do

Both are release decisions across six live deployments, not code changes to slip in:

  1. VERSION is left at 1.3.0. It must be bumped before deploying — it is part of the EIP-712 domain, so every configHash depends on it.
  2. The gateway's Payment#validate is not mirrored. A hand-built payment with too tight a window would fail on-chain rather than as a clean 422. Worth a follow-up in rail0-gateway.

This is BREAKING for any caller that relied on equal expiries.

Tests

test_Validation_AcceptsExpiriesEqual is replaced — it existed to lock in the old behaviour. New coverage:

test pins
RejectsExpiriesEqual the zero-window case itself
RefundWindowBoundary exactly MIN_REFUND_WINDOW accepted, one second under refused — so the constant is the real gate
RejectsAnHoursLongRefundWindow the metric is days
ChargeRejectsACollapsedRefundWindow charge shares _validatePayment; a validation added to one entrypoint only is the classic miss

Suite: 117 tests pass. forge fmt --check clean; forge lint reports the same 9 pre-existing block-timestamp warnings.

Closes #41

🤖 Generated with Claude Code

authorizationExpiry == refundExpiry was accepted — the ordering check uses `>`, not
`>=` — which collapses the refund/dispute window to zero: both become unreachable the
instant the authorization ends, so a payment could be created that is refundable in
name only.

_validatePayment now requires refundExpiry - authorizationExpiry >=
MIN_REFUND_WINDOW, a new public constant set to 1 days.

Days, not hours, as the metric: the window has to survive a buyer noticing a problem,
a merchant responding and a transaction confirming, and a window measured in hours is
one the buyer can lose to a weekend. There is a test asserting a 6-hour window is
refused even though it is non-zero.

One day is a FLOOR against a collapsed window, not a recommendation — the README's
guidance stays 14–30 days aligned with consumer-protection practice. Chosen so it
cannot reject what integrators already produce: the gateway's defaults are a 7-day
authorization and a 30-day refund window, a 23-day gap. The whole existing test
corpus passes unchanged, which is the evidence that the floor is not disruptive.

The subtraction cannot underflow: the ordering check immediately above already
guarantees refundExpiry >= authorizationExpiry.

Gas: +328 on authorize/charge (151,848 → 152,176 measured on
test_Authorize_Success), and nothing on any other entrypoint — the check lives only
on the creation path.

BREAKING for any caller that relied on equal expiries. Two things this PR does NOT
do, deliberately, because they are release decisions across six live deployments:
  - VERSION is left at 1.3.0. It must be bumped before deploying, since it is part
    of the EIP-712 domain and every configHash depends on it.
  - the gateway's Payment#validate is not mirrored, so a hand-built payment with too
    tight a window would fail on-chain rather than as a 422. Worth a follow-up.

Replaces test_Validation_AcceptsExpiriesEqual, which existed to lock in the old
behaviour. New tests cover: equal expiries rejected, the boundary on both sides
(exactly MIN_REFUND_WINDOW accepted, one second under refused), an hours-long window
refused, and charge rejecting the same shape — a validation added to only one
entrypoint being the classic miss.

Suite: 117 tests pass. forge fmt --check clean; forge lint reports the same 9
pre-existing block-timestamp warnings.

Closes #41

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@pviti
pviti marked this pull request as ready for review July 29, 2026 20:33
@pviti
pviti requested a review from fconforti July 29, 2026 20:33
@pviti
pviti changed the base branch from main to v1.4.0 July 29, 2026 23:20
@pviti

pviti commented Jul 29, 2026

Copy link
Copy Markdown
Member Author

Note on the base and on issue closure

This PR now targets v1.4.0, not main. Two consequences worth stating here, since the PR had no comments recording them:

Closes #41 will not fire. GitHub auto-closes a linked issue only when the PR merges into the default branch (main). The link stays visible in the sidebar, but merging this into v1.4.0 leaves #41 open.

That is the intended outcome, not an oversight. #41 describes behaviour that is live on all six 1.3.0 deployments: they still accept equal expiries. Closing it when the fix lands on a version branch would mark as resolved something still true on chain. It closes when v1.4.0 reaches main and the 1.4.0 rollout is out — noted on #41 itself.

Why v1.4.0 rather than v1.3.1: this rejects expiry pairs the deployed contract currently accepts, so it is not a backward-compatible patch. A minor bump is the honest semver signal.

Still outstanding, unchanged from the description: VERSION is deliberately left at 1.3.0 here — bumping it is a release decision across six live deployments — and the gateway's Payment#validate does not yet mirror this rule, so a hand-built payment with too tight a window would fail on-chain rather than as a clean 422.

@pviti pviti added this to the v1.4.0 milestone Jul 30, 2026
@pviti pviti added the target: v1.4.0 Merges into the v1.4.0 branch — non-breaking contract release label Jul 30, 2026
@pviti
pviti merged commit cc0cbfb into v1.4.0 Jul 31, 2026
1 check passed
@pviti
pviti deleted the feature/minimum-refund-window branch July 31, 2026 14:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

target: v1.4.0 Merges into the v1.4.0 branch — non-breaking contract release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug(low): authorizationExpiry == refundExpiry is accepted — collapses the refund/dispute window to zero

1 participant