feat(core): delivery protection v2 — OrConditions + PaymentIndexRecorder

[A1igator]

Summary

Updates deployDeliveryProtectionOperator with proper access control and payment indexing.

New Operator Config

Slot	Before	After
releaseCondition	SAC(arbiter) only	OrCondition([SAC(arbiter), PayerCondition])
refundInEscrowCondition	EscrowPeriod only	OrCondition([EscrowPeriod, ReceiverCondition, SAC(arbiter)])
authorizeRecorder	EscrowPeriod only	RecorderCombinator([EscrowPeriod, PaymentIndexRecorder])

Why

• Payer can release: satisfied payer can release without waiting for arbiter
• Arbiter can refund immediately: on FAIL verdict, arbiter calls refundInEscrow() without waiting for escrow period
• Receiver can refund: merchant voluntary refund
• PaymentIndexRecorder: keepers can discover payments on-chain for independent refund triggering

Changes

• config/index.ts: Add RecorderSingletonAddresses (paymentIndexRecorder: 0x3134...), recorderCombinatorCodehash, getRecorderSingletons()
• deploy/presets.ts: Rewrite preview + deploy with batched OrCondition/RecorderCombinator computation
• Tests: Full coverage for both default (6 contracts) and fallback (paymentIndexRecorderAddress: zeroAddress, 5 contracts) paths

Deploy count

6 contracts (was 3): EscrowPeriod, SAC(arbiter), OrCondition(release), OrCondition(refund), RecorderCombinator, Operator.

Test plan

• Build passes
• Typecheck passes
• Biome check passes
• Unit tests — 195 passing
• Fork integration tests (deploy.fork.test.ts, delivery-protection.fork.test.ts)

🤖 Generated with Claude Code

[codecov]

Codecov Report

❌ Patch coverage is 85.36585% with 6 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
packages/core/src/deploy/presets.ts	85.36%	0 Missing and 6 partials ⚠️

📢 Thoughts on this report? Let us know!

[vraspar]

SDK Review

Found 7 issues (reviewed: tests, conventions, dead code, SDK design):

1. [SDK Design] Mutable idx counter for parsing multicall existence results diverges from the named-entries + existsMap pattern already established by deployMarketplaceOperator in the same file. The positional counter with implicit if (hasCombinator) idx++ skip is fragile and order-dependent.

   x402r-sdk/packages/core/src/deploy/presets.ts

   Lines 1245 to 1254 in 353e5ea

   	let idx = 0
   	const escrowPeriodExists = existenceResults[idx++].result !== zeroAddress
   	const arbiterCondExists = existenceResults[idx++].result !== zeroAddress
   	const releaseCondExists = existenceResults[idx++].result !== zeroAddress
   	const refundCondExists = existenceResults[idx++].result !== zeroAddress
   	const combinatorExists = hasCombinator
   	? existenceResults[idx++].result !== zeroAddress
   	: true // no combinator needed
   	const operatorExists = existenceResults[idx++].result !== zeroAddress

   Fix: Adopt the same existenceEntries: { name, contract }[] + existsMap approach used by the marketplace preset. The conditional combinator entry is already conditionally spread — idx would not be needed.

2. [SDK Design] recorderCombinatorCodehash is a bytecode hash (computed via forge script), not a deployment address. Embedding it in X402rChainConfig implies it might differ per chain, which the JSDoc explicitly contradicts ("same for all instances"). Uniswap keeps INIT_CODE_HASH / POOL_INIT_CODE_HASH as module constants separate from chain address registries.

   x402r-sdk/packages/core/src/config/index.ts

   Line 48 in 353e5ea

   recorderCombinatorCodehash: Hex

   Fix: Remove recorderCombinatorCodehash from X402rChainConfig and PROTOCOL_ADDRESSES. Keep it as the existing module-level export. Presets can import it directly rather than reading it off getChainConfig().

3. [Tests] The releaseCondition is now OrCondition([SAC(arbiter), PayerCondition]) — payer is a first-class release path. The fork test only exercises the arbiter leg. There is no fork test for payerClient.payment.release(). The comment at L178 carves out negative condition-enforcement tests for Foundry, but a positive SDK flow test for payer releasing is distinct from that carve-out.

   x402r-sdk/packages/core/tests/integration/delivery-protection.fork.test.ts

   Lines 161 to 184 in 353e5ea

   	}, 60_000)
   	it('arbiter releases funds (no escrow wait required)', async () => {
   	// Arbiter can release immediately — releaseCondition is
   	// OrCondition([SAC(arbiter), PayerCondition]), no time delay needed.
   	const hash = await arbiterClient.payment.release(
   	paymentInfo,
   	DEFAULT_AMOUNT,
   	)
   	await publicClient.waitForTransactionReceipt({ hash })
   	const amounts = await merchant.payment.getAmounts(paymentInfo)
   	expect(amounts.capturableAmount).toBe(0n)
   	}, 60_000)
   	})
   	// ---------------------------------------------------------------------------
   	// Timeout path: authorize → escrow expires → anyone calls refundInEscrow()
   	//
   	// NOTE: Condition enforcement (non-arbiter cannot release, refundInEscrow
   	// reverts during escrow) is tested in Foundry contract tests, not here.
   	// The on-chain operator bytecode at the current fork block does not enforce
   	// condition checks via revert — no existing fork test uses rejects.toThrow().
   	// ---------------------------------------------------------------------------

   Fix: Add a 'payer releases funds' test: authorize a fresh payment, call payerClient.payment.release(), assert capturableAmount === 0n.

4. [Conventions] deployDeliveryProtectionOperator calls getConditionSingletons(options.chainId) twice inline in the existence-check block (for .payer and .receiver), then declares const singletons again at L1290 for Phase 3. All other resolved values (factoryAddrs, config, authorizedCodehash) are bound at the top of the function.

   x402r-sdk/packages/core/src/deploy/presets.ts

   Lines 1200 to 1220 in 353e5ea

   	abi: orConditionFactoryAbi,
   	functionName: 'getDeployed',
   	args: [
   	[
   	arbiterConditionAddress,
   	getConditionSingletons(options.chainId).payer,
   	],
   	],
   	},
   	{
   	address: factoryAddrs.orCondition,
   	abi: orConditionFactoryAbi,
   	functionName: 'getDeployed',
   	args: [
   	[
   	escrowPeriodAddress,
   	getConditionSingletons(options.chainId).receiver,
   	arbiterConditionAddress,
   	],
   	],
   	},

   Fix: Hoist const singletons = getConditionSingletons(options.chainId) to the top alongside factoryAddrs and config. Remove the duplicate declaration at L1290.

5. [Tests] Fork deploy test 'deploys all components matching preview' only asserts three address fields (operatorAddress, escrowPeriodAddress, arbiterConditionAddress). The three new fields introduced by this PR — releaseConditionAddress, refundInEscrowConditionAddress, authorizeRecorderAddress — are never asserted on the deployment result against real factory bytecode.

   x402r-sdk/packages/core/tests/integration/deploy.fork.test.ts

   Lines 276 to 291 in 353e5ea

   	// All deployed addresses should match previewed addresses
   	expect(deployment.operatorAddress).toBe(preview.operatorAddress)
   	expect(deployment.escrowPeriodAddress).toBe(preview.escrowPeriodAddress)
   	expect(deployment.arbiterConditionAddress).toBe(
   	preview.arbiterConditionAddress,
   	)
   	// escrowPeriod + arbiterCondition + 2 OrConditions + recorderCombinator + operator = 6
   	expect(deployment.deployments).toHaveLength(6)
   	expect(deployment.summary.newCount + deployment.summary.existingCount).toBe(
   	6,
   	)
   	expect(deployment.summary.txHashes).toHaveLength(
   	deployment.summary.newCount > 0 ? 1 : 0,
   	)

   Fix: Add expect(deployment.releaseConditionAddress).toBe(preview.releaseConditionAddress) and same for refundInEscrowConditionAddress and authorizeRecorderAddress.

6. [Tests] The fallback test (paymentIndexRecorder = zeroAddress) and the main deploy test both assert result.authorizeRecorderAddress but never assert result.operatorConfig.authorizeRecorder. These are separate fields — operatorConfig is what gets passed to the on-chain factory. A bug that maps the combinator to the wrong OperatorConfig field would pass both tests.

   x402r-sdk/packages/core/tests/deploy/presets.test.ts

   Lines 782 to 800 in 353e5ea

   	it('falls back to EscrowPeriod recorder when PaymentIndexRecorder is zeroAddress', async () => {
   	const escrowAddr = '0xaAaAaAaaAaAaAaaAaAAAAAAAAaaaAaAaAaaAaaAa' as Address
   	const operatorAddr = '0xCcCCccccCCCCcCCCCCCcCcCccCcCCCcCcccccccC' as Address
   	const publicClient = createMockPublicClient({
   	computeAddress: escrowAddr,
   	[`${F.paymentOperator}:computeAddress`]: operatorAddr,
   	})
   	const result = await previewDeliveryProtectionOperator(
   	publicClient,
   	makeDeliveryProtectionOptions({
   	paymentIndexRecorderAddress: zeroAddress,
   	}),
   	)
   	// Without PaymentIndexRecorder, authorizeRecorder falls back to escrowPeriod
   	expect(result.authorizeRecorderAddress).toBe(escrowAddr)
   	expect(result.paymentIndexRecorderAddress).toBe(zeroAddress)
   	})

   Fix: Add expect(result.operatorConfig.authorizeRecorder).toBe(escrowAddr) in the fallback test, expect(result.operatorConfig.authorizeRecorder).toBe(combinatorAddr) in the deploy test.

7. [Tests / Dead Code] The test 'uses provided authorizedCodehash in escrowPeriod computation' was deleted with no replacement. The authorizedCodehash option is still public on DeliveryProtectionOperatorOptions, and the PR changes the default from pad('0x00') to recorderCombinatorCodehash — a behavioral change with zero test coverage.

   Fix: Restore a test that passes a custom authorizedCodehash and asserts a different escrowPeriodAddress, plus a test asserting the new default uses recorderCombinatorCodehash.

---

Generated with Claude Code using review-sdk skill

[A1igator]

All 7 fixed in a9fdbec:

1. idx counter → named entries + existsMap (matches marketplace preset)
2. recorderCombinatorCodehash in chain config → removed from X402rChainConfig + PROTOCOL_ADDRESSES, kept as module export
3. payer release fork test → added with salt 201n
4. duplicate getConditionSingletons → hoisted to top of function
5. missing fork deploy assertions → added releaseConditionAddress, refundInEscrowConditionAddress, authorizeRecorderAddress
6. operatorConfig.authorizeRecorder assertions → added in both 6-contract and 5-contract tests
7. authorizedCodehash tests → added default + custom override coverage

[vraspar]

Protocol Analysis: Delivery Protection v2

Deep review of the condition matrix changes and protocol implications.

---

Condition Matrix (v1 → v2)

Slot	v1	v2
releaseCondition	SAC(arbiter)	OrCondition([SAC(arbiter), Payer])
refundInEscrowCondition	EscrowPeriod	OrCondition([EscrowPeriod, Receiver, SAC(arbiter)])
authorizeRecorder	EscrowPeriod	RecorderCombinator([EscrowPeriod, PaymentIndexRecorder])
refundPostEscrowCondition	Receiver	Receiver (unchanged)

---

What works well

• Arbiter immediate refund (FAIL-fast) — Core win. In v1, if the arbiter detects garbage content, the payer waits the full escrow period for timeout-refund. Now the arbiter refunds instantly. For HTTP 402 micropayments where quality is checked in seconds, this is a significant UX improvement.
• Receiver voluntary refund — Honest merchant can proactively refund without waiting for arbiter or escrow expiry.
• RecorderCombinator — Clean composition pattern. On-chain payment indexing without external indexer dependency.

---

Concerns to discuss

1. Arbiter trust surface doubled

	v1	v2
Arbiter can release (funds → receiver)	✅	✅
Arbiter can refund (funds → payer)	❌	✅

A compromised/buggy arbiter now controls fund movement in both directions. A rogue arbiter could systematically refund all payments (denying receiver revenue) or release all payments (denying payer refunds). Probably acceptable for the EigenCloud verifiable arbiter model, but this is a trust assumption change worth documenting.

2. Payer self-release partially undermines the protection guarantee

v1: arbiter is the sole quality gate — every payment goes through quality inspection.
v2: payer can release immediately, arbiter never checks.

In the HTTP 402 flow where the payer is automated, client-side code could skip quality verification entirely. The "delivery protection" becomes opt-in from the payer's perspective rather than enforced. Is this intentional?

3. charge slots remain zeroAddress

Both chargeCondition and chargeRecorder are zeroAddress. If someone calls operator.charge(), it bypasses all conditions and RecorderCombinator — the payment won't be indexed by PaymentIndexRecorder. Pre-existing gap from v1, but more visible now that indexing is a feature. Consider whether charge should be explicitly disabled for this preset.

---

Missing fork test coverage

The 3 most important new paths have no fork tests:

New behavior	Fork test?
Payer self-release	✅ Added (salt 201)
Arbiter immediate refund (FAIL-fast)	❌ Missing
Receiver voluntary refund during escrow	❌ Missing
PaymentIndexRecorder data verification	❌ Missing

The arbiter-immediate-refund is the core motivation for this PR and has no fork test. Recommended additions:

• arbiterClient.payment.refundInEscrow() — without time-forwarding, should succeed immediately
• receiverClient.payment.refundInEscrow() — without time-forwarding, should succeed immediately
• After authorize, query PaymentIndexRecorder.getPayerPayments(payer, 0, 10) to verify on-chain indexing works end-to-end

---

Generated with Claude Code using review-sdk skill

[A1igator]

Protocol Analysis Response

Concerns

1. Arbiter trust surface doubled — The escrow period for delivery protection is 2 minutes. A rogue arbiter refunding during that window is operationally equivalent to the escrow expiring — the payer gets funds back either way. The arbiter shortcut just removes the 2-minute delay for legitimate FAIL verdicts. In practice the arbiter verdict lands in <5s, well before any receiver could capture. The EigenCloud arbiter is verifiable compute (deterministic + attestable) so this is a low-risk tradeoff for a significant UX win.

2. Payer self-release — Intentional. The HTTP 402 flow has two actors: automated client and automated arbiter. Payer self-release handles the case where the payer inspects content client-side and is satisfied before the arbiter round-trip completes. The protection guarantee is that someone (arbiter or payer) validates quality, not that the arbiter is the sole gate.

3. charge slots as zeroAddress — Pre-existing from v1. Delivery protection doesn't use the charge flow (authorize → release/refund, no partial captures). Not worth a RevertCondition for a flow that's never called.

Missing fork tests

Added in 77d00a0:

• Arbiter immediate refund — arbiterClient.payment.refundInEscrow() without time-forwarding (salt 203)
• Receiver voluntary refund — merchant.payment.refundInEscrow() during escrow (salt 204)
• PaymentIndexRecorder verification — queries getPayerPayments() and asserts indexed count > 0

[vraspar]

Configurability gap

The OrCondition compositions are hardcoded — deployers cannot opt out of individual legs:

Behavior	Configurable?
PaymentIndexRecorder	✅ paymentIndexRecorderAddress: zeroAddress to skip
Payer self-release	❌ Hardcoded in release OrCondition
Receiver voluntary refund	❌ Hardcoded in refundInEscrow OrCondition
Arbiter immediate refund	❌ Hardcoded in refundInEscrow OrCondition

A deployer who wants arbiter-only release (v1 behavior) or doesn't want receiver refund during escrow has no option — they'd have to skip the preset entirely.

Suggestion: Add optional flags to DeliveryProtectionOperatorOptions:

/** Allow payer to release without arbiter (default: true) */
allowPayerRelease?: boolean
/** Allow receiver to refund during escrow (default: true) */
allowReceiverRefund?: boolean
/** Allow arbiter to refund immediately during escrow (default: true) */
allowArbiterRefund?: boolean

When false, that leg is excluded from the OrCondition (or the OrCondition is skipped entirely and the single condition is used directly — matching v1 behavior). Low cost to add, gives deployers control over their trust model.

---

Generated with Claude Code using review-sdk skill

[A1igator]

Added allowArbiterRefund in ab188ec — only that leg is configurable.

Payer self-release and receiver voluntary refund are not optional — they're core to the delivery protection model:

• Payer self-release: if the payer verifies content client-side before the arbiter round-trip, they should be able to release immediately. Removing this would force unnecessary arbiter latency on satisfied payers.
• Receiver voluntary refund: honest merchants must always be able to proactively refund. There's no trust model where removing this makes sense — it only hurts the receiver's ability to act in good faith.

allowArbiterRefund (default: true) is the one that's genuinely situational — some deployers may want the v1 behavior where refund requires escrow expiry, especially if they don't trust the arbiter to refund correctly. When false, the refundInEscrow OrCondition drops from 3 legs to 2: [EscrowPeriod, ReceiverCondition].

[A1igator]

Follow-up: flipped allowArbiterRefund default to false (opt-in) in #107. Arbiter refund power should be a conscious choice, not implicit.