Conversation
|
@phdargen is attempting to deploy a commit to the Coinbase Team on Vercel. A member of the Team first needs to authorize it. |
da1fca7 to
7c72fa4
Compare
CarsonRoscoe
left a comment
There was a problem hiding this comment.
One comment, might just be a documentation piece; There's a risk with the allowlist where upgradeable/proxy operator contracts could undermine the model. If the allowlist admits an address that was honest and later upgrades to a dishonest implementation, it effectively bypassed the allowlist.
We should document that facilitators' should prefer immutable operator contracts
CarsonRoscoe
left a comment
There was a problem hiding this comment.
Nothing stops a merchant from registering a route with operatorType: "custom" while leacving captureMode at its default ("sync"), or leaving cancel-triggerred auto-void wired. The resource server will still build and submit an automatic capture after settle or void on cancel, but the facilitator unconditionally rejects lifecycle relay for operatorType: "custom" (See ErrLifecycleNotRelayed).
This would lead to the initial authorize succeeding and escrowing the payer's funds, and the automatic follow-up capture/void always failing silently, leaving funds stuck in escrow with no way to unwind unless the merchant happens to know to force captureMode: "deferred".
While this isn't a theft risk, its a big UX/funds availability trap we should avoid.
I'd suggest adding to the spec that servers MUST not set this combination, and at the server SDK level, fail fast when operatorType: "custom" is combined with anything other than captureMode: "deferred", and not auto-stamping receiverAuthorizer onto custom-operator routes.
CarsonRoscoe
left a comment
There was a problem hiding this comment.
nit: verify() for custom operators runs a ~7-9-call eth_simulateV1 batch vs the delegated's single eth_call. This is a much higher RPC cost per request, so spam/DoS cost to a facilitator is higher specifically for custom-operator verify() calls. Worth documenting
CarsonRoscoe
left a comment
There was a problem hiding this comment.
In the facilitators' lifestyle.ts , if a capture succeeds on-chain but the trailing voil call fails for a mundane reason like RPC timeout (any error that isn't ZeroAuthorization or revert), the function returns success: false while discarding the txHash. imo this should be preserved the transaction regardless of the void outcome
CarsonRoscoe
left a comment
There was a problem hiding this comment.
settlementHooks.ts's persistCollect unconditionally overwrites storage without checking if the record exists. I'll let you decide what we should do in that case, but calling out as-is a duplicate or retried settlement for the same paymentInfoHash would reset capturableAmount/refundableAmount
CarsonRoscoe
left a comment
There was a problem hiding this comment.
I believe there's a ERC-6492 signature verify/settle mismatch in facilitator/connect.ts and utils.ts. verifyCollect unwraps and verifies the inner 6492 signature, but unpackForSettle also passes the original wrapped signature bytes as colelctorData for the real on-chain call. Circle's SignatureChecker / Permit2's verifier don't understand the 6492 wrapper, so any client submitting a 6492-wrapped signature will pass verify() and then revert on settle()
Similarly, there's no counterfactual/undeployed wallet path. If we're going straight to the facilitator implementation, this should be considered.
675bbc0 to
0513ce2
Compare
68d239f to
f393735
Compare
|
Reading #3197 while working against this — one boundary question. The answer settles an open question on a proposal of mine, not anything in this PR. v1.1 moves in two directions at once and I can't tell which is the rule. Toward the enum: Toward Read together that looks like a rule: anything that changes which phases run belongs in the enum, and Asking because #3182 leaves exactly this open — its first protocol question is whether response-before-settlement is a fourth core payment flow, an EVM-specific binding, a generic asynchronous lifecycle, or only an application pattern. Under the rule above it's the first, since the delay changes whether a settle can run at all rather than when a selected one finalizes. I'd rather you drew that line than that I picked the reading that suits me. One data point in case it makes the question cheaper than it sounds: on the permit2 binding the primitive is already merged and deployed. |
|
I noticed one narrow v1.1 fixture mismatch that may be worth correcting before the broader draft lands.
The current live custom-operator integration path exercises I prepared a small local patch that changes the wrapper to On another note, |
9979a20 to
9f72d29
Compare
9f72d29 to
9185dbf
Compare
711b833 to
575ccb9
Compare
Description
Continuation of #2308 implementing the v1.1 spec proposed in #3197
Tests
Checklist