Add EIP: Flow Control Wallet Call Capability#9259
Conversation
|
✅ All reviewers have approved. |
64b1893 to
114bb1c
Compare
|
|
||
| #### Rollback | ||
|
|
||
| A rollback is informally defined as "causing no meaningful changes on chain." A |
There was a problem hiding this comment.
Is there any reason to break up lines like this? A paragraph could be on one line and use line wrapping, not sure what the standard is here
There was a problem hiding this comment.
I like a narrow line width, but we accept both wrapped and unwrapped lines.
| | `207` | Partial Success | | ||
|
|
||
| An "included" call, in this section, is defined as having either been | ||
| successfully or unsuccessfully executed. A call that has been recorded on chain, |
There was a problem hiding this comment.
| successfully or unsuccessfully executed. A call that has been recorded on chain, | |
| executed, even if execution was unsuccessful. A call that has been recorded on chain, |
|
|
||
| Status `102` SHALL be returned only when all of the following are true: | ||
|
|
||
| * At least one call in the batch has been included on chain; and |
There was a problem hiding this comment.
| * At least one call in the batch has been included on chain; and | |
| * At least one call in the batch has been included on chain. |
|
|
||
| Status `207` SHALL be returned only when all of the following are true: | ||
|
|
||
| * At least one call in the batch has been included and succeeded; |
There was a problem hiding this comment.
| * At least one call in the batch has been included and succeeded; | |
| * At least one call in the batch has been included and succeeded. |
Same for rest of list, for consistency with other lists
|
|
||
| ## Backwards Compatibility | ||
|
|
||
| <!-- |
114bb1c to
9968288
Compare
|
The commit a269e9d (as a parent of d421328) contains errors. |
|
|
||
| The following subsections are modifications to the API endpoints from EIP-5792. | ||
|
|
||
| If a request does not match the schema defined below, the wallet MUST reject the request with an error code of 0 <!-- TODO -->. |
There was a problem hiding this comment.
i would prefer to create a params/constants table and refer them by their param/constant name here. brings it out as well as confine the TODO to a particualr section.
similarly with other TODOS
|
small nit and linter/markup fixes required |
|
any particular reason this isn't an ERC @SamWilsn ? other capabilities are ERCs and 5792 technically does specify capabilities should be ERCs |
| Strict atomicity is simply naming the default behaviour of EIP-5792: calls | ||
| within a single batch MUST be contiguous and applied atomically (or the batch | ||
| rolled back.) |
There was a problem hiding this comment.
this is effectively atomicBatch right?
There was a problem hiding this comment.
Can I pitch a more explicit naming for atomicity
strict -> transaction
loose -> bundle
none -> none
I think this more clearly states what the app is requesting
There was a problem hiding this comment.
I'm not sure I find those more clear? What is a bundle? They also seem tied to particular implementations. I can imagine a multisig where each signature is a single transaction and only the last one executes the calls. You'd have multiple transactions, applied atomically.
There was a problem hiding this comment.
I think in that example I would only expect the last transaction to show up in the result (the signature transactions aren't really relevant to the app's request).
Fair that it's still not super clear. I guess I prefer these as it's more explicit about how the calls take place, i.e.
transaction = all in one transaction
bundle = in a bundle of transactions (Flashbots bundles as a reference), implying that transactions will be executed as a group, unless there's a reorg
none = no constraints
|
|
||
| ## Motivation | ||
|
|
||
| While the base EIP-5792 specification works extremely well for smart contract wallets, it does not allow the expression of the full range of flow control options that wallets can implement. For example, a dapp may only be submitting a batch for gas savings and not care about whether all calls are reverted on failure. A wallet may only be able to offer a limited form of atomicity through block builder backchannels, but that may be sufficient for a trading platform. |
There was a problem hiding this comment.
a dapp may only be submitting a batch for gas savings and not care about whether all calls are reverted on failure
Why wouldn't dapp care? What's the user flow in this case?
There was a problem hiding this comment.
Something like a token dispersement or several unrelated swaps. For example, I could be swapping several ERC-20 tokens into ETH. I just want as much of that ETH as possible, and I'll resubmit any failed swaps.
| "data": "0xfbadbaf01", | ||
| "capabilities": { | ||
| "flow-control": { | ||
| "on-failure": "continue" |
There was a problem hiding this comment.
what exactly is a wallet supposed to do in this case when the flow fails? what does continue mean? like to continue with the execution of the next calls? what if there's only one call in the array?
There was a problem hiding this comment.
Hopefully that's defined well enough later in the document (and let me know if it isn't), but a failed call with "on-failure": "continue" is just skipped, and the next call is executed.
| * MUST reject (with error code 0<!-- TODO -->) batches containing unsupported | ||
| `on-failure` modes. |
There was a problem hiding this comment.
We should rather set some default mode instead of forcing a wallet to reject such a batch. The reasoning is that on mobile devices MUST reject indicates that the wallet should reject such request immediately which is bad UX. Imagine a mobile wallet in the background that is triggered by a dapp aka it receives the batch request, a wallet is put to the foreground and immediately redirects back to a dapp without any information to the user. It should rather default to some mode and continue the flow.
There was a problem hiding this comment.
We should rather set some default mode instead of forcing a wallet to reject such a batch.
This is unacceptable behaviour. If the dapp requests a mode, it has to get that mode. Imagine asking for "rollback" and getting "continue". That could be catastrophic.
Imagine a mobile wallet in the background that is triggered by a dapp aka it receives the batch request, a wallet is put to the foreground
Does the wallet need to be in the foreground to reject the request? If so, how does the UX flow work for unsupported RPC endpoints, like for example eth_signTransaction on MetaMask? This doesn't seem unique to just this proposal.
immediately redirects back to a dapp without any information to the user
The standard doesn't say anything about not displaying information to the user. The wallet is free to display a message to the user before redirecting back to the dapp.
There was a problem hiding this comment.
I meant more like the scenario where the dapp requests the mode, but the wallet doesn't support that mode. The proposal suggests the rejection, which is a bad UX on mobile, hence reasoning for some default behaviour that mitigates that.
Yes, on mobile, a wallet must be foregrounded to receive the request. So, in the MetaMask case (and not only), they support the CAIP-25 handshake, which defines the methods, chains, and events supported by a wallet, so dapp knows before sending the request if a given method is supported by a wallet or not. Hence, there's no problem with automatic rejection on mobile.
Now, when I'm writing this, I realized that the wallet could use the CAIP-25 handshake (sessionProperties) to define what modes are supported, so the dapp could only request those that the wallet supports.
| #### `wallet_sendCalls` | ||
|
|
||
| The wallet MUST reject `wallet_sendCalls` requests with error code 0 | ||
| <!-- TODO --> where both: |
There was a problem hiding this comment.
As mentioned below, this creates a bad UX on mobile devices. We should rather default to one agreed behaviour to keep the user flow consistent instead of letting a wallet auto-reject such requests:
https://github.com/ethereum/EIPs/pull/9259/files#r1948151108
| The returned `receipts` array: | ||
|
|
||
| * MUST NOT contain more than one receipt for the same transaction. | ||
| * SHOULD NOT contain receipts for transactions without a call from the requested |
There was a problem hiding this comment.
Why not MUST NOT? Is it allowed to add a receipt for a call that was not icluded in the batch? If yes, why, and what is the use case?
There was a problem hiding this comment.
I went with "MUST NOT" for things that would break/be unsafe, and SHOULD NOT for things that would be weird but not dangerous.
| "flow-control": { | ||
| "loose": ["halt", "continue"], | ||
| "strict": ["continue"] |
There was a problem hiding this comment.
Shouldn't this define on-failure flow control for call-scope and atomicity flow control for batch-scope?
Not sure how to interpret this part:
"loose": ["halt", "continue"],
"strict": ["continue"]
There was a problem hiding this comment.
This snippet is saying the wallet supports on-failure modes of halt and continue when using loose atomicity, but only continue when using strict.
Co-authored-by: Marc <Marchhill@users.noreply.github.com>
a269e9d to
ca6955a
Compare
|
|
||
| ## Motivation | ||
|
|
||
| While the base EIP-5792 specification works extremely well for smart contract wallets, it does not allow the expression of the full range of flow control options that wallets can implement. For example, a dapp may only be submitting a batch for gas savings and not care about whether all calls are reverted on failure. A wallet may only be able to offer a limited form of atomicity through block builder backchannels, but that may be sufficient for a trading platform. |
There was a problem hiding this comment.
I think it is worth mentioning the EOA use case (for 7702 laggards, whether chains or wallets).
I think it is also worth mentioning that while atomicity might be a hard requirement for some use cases (particularly Defi), for others it is not important, and this allows those apps to benefit from 5792 as well. Also notably approve-transfer is currently non-atomic, and improving that UX is generally a priority.
| "atomicity": { | ||
| "enum": ["strict", "loose", "none"] | ||
| } | ||
| } |
There was a problem hiding this comment.
why not allow setting onFailure at the top level to set the default?
| Strict atomicity is simply naming the default behaviour of EIP-5792: calls | ||
| within a single batch MUST be contiguous and applied atomically (or the batch | ||
| rolled back.) |
There was a problem hiding this comment.
Can I pitch a more explicit naming for atomicity
strict -> transaction
loose -> bundle
none -> none
I think this more clearly states what the app is requesting
|
|
||
| A rollback is informally defined as "causing no meaningful changes on chain." A | ||
| rolled back batch only makes gas accounting and bookkeeping (eg. nonce) | ||
| changes. In other words, a rollback is the default behaviour of EIP-5792 when |
There was a problem hiding this comment.
I think it's worth calling out that with this EIP the default is strict / rollback (maybe not in this spot, but mentioning because it's alluded to here)
| changes. In other words, a rollback is the default behaviour of EIP-5792 when | ||
| a call fails. | ||
|
|
||
| #### Critical Calls |
There was a problem hiding this comment.
I don't see critical defined in the JSON schema?
There was a problem hiding this comment.
I'm defining critical here so I can use:
- "critical call" as short-form for call with
"onFailure": "rollback"and; - "non-critical call" as short-form for the other modes.
|
|
||
| * MUST treat a missing call-scope `flowControl` capability as equivalent to | ||
| setting `onFailure` to `rollback`. | ||
| * MUST treat a missing call-scope `onFailure` mode as equivalent to `rollback`. |
There was a problem hiding this comment.
I think this is another argument for request-level onFailure settings - as an example I don't think loose and none are necessarily capable of rollback?
| provide for any reason other than user rejection. | ||
| * Wallets supporting `strict` but not `loose` SHOULD NOT reject `loose` | ||
| batches and SHOULD instead upgrade the request to strict atomicity. | ||
| * Note that a batch with exactly one call _always_ satisfies the requirements |
There was a problem hiding this comment.
For such calls any specification of flow control capabilities is effectively ignored, as it's necessarily strict / rollback?
| batch. | ||
| * MUST contain exactly one receipt capturing each successful call. | ||
| * Multiple calls MAY be captured in one receipt, but the successful | ||
| execution of one call MUST NOT be captured by multiple receipts. |
There was a problem hiding this comment.
is it even possible for a successful call to be captured by multiple receipts?
| * `[(successful A, successful B)]` | ||
| * `[(successful A), (successful B)]` | ||
| * `[(successful A, unsuccessful B), (successful B)]` | ||
| * `[(unsuccessful A), (successful A), (successful B)]` |
There was a problem hiding this comment.
how are unsuccessful calls represented? obviously the receipt as a whole has a status, but you can have failed calls in a successful receipt (requiring tracing to track down what actually happened) - and I don't see where in the call result it's identified one way or the other
| { | ||
| "0x1": { | ||
| "flowControl": { | ||
| "none": [ "rollback", "continue" ] |
There was a problem hiding this comment.
I believe the options should be halt and continue. Right @SamWilsn?
There was a problem hiding this comment.
It would be quite weird to have none + rollback...
eth-bot
left a comment
There was a problem hiding this comment.
All Reviewers Have Approved; Performing Automatic Merge...
No description provided.