ethereum
Update EIP-5792: Propose changes to EIP-5792
List of changes proposed in this PR and their motivations:
- Rephrase "The items in the
callsfield are only those that are shared by all transaction types" and "method only returns a subset of the fields thateth_getTransactionReceiptreturns, excluding any fields that may differ across wallet implementations".
There is a rapidly growing list of transaction types, and the definition of the "shared by all" can become confusing. Specifically, most transaction fields are removed in RIP-7560 transaction type which is part of the Account Abstraction roadmap.
- Add optional 'capabilitiesData' field to individual 'calls' entities.
It seems likely that more advanced capabilities will need to provide some data on a per-call basis. One example could be an "optional flag" capability that would allow a Smart Contract Wallet to proceed with the batch in case of a revert, which would otherwise default to requiring success from all calls. Whether this particular capability is needed or not, it serves as an example of why such an option might be needed in the future.
- Specify that an unsupported 'capability' cannot be ignored and the operation must be rejected.
I did not see it in the text anywhere, and ignoring data provided in the capabilities field is obviously extremely dangerous.
- Add 'chainId' back to the 'SendCallsParams'.
For now, it is defined as optional, but probably shouldn't be. This has been debated lately and I don't know what the conclusion is.
- Specify that the 'identifier' must be a 32 byte hex.
One reason is that "any string" seems to be too vague of a definition for an ERC. What about "", "ለከበሩጓደኛመርህመንገድይሆናል", or "😎🤏🕶😳", are these strings okay? Also, in other programming languages it could be more convenient to use the same identifier type as for transactions, blocks and all other things in Ethereum. If there is a motivation for leaving it as "any string", please at least specify it in the ERC.
- Added 'capabilitiesData' object to the 'wallet_getCallsStatus' response.
It seems very plausible that 'capabilities' will need to provide some additional information to the response. This was previously impossible, which could lead to wallets creating non-standard ways of querying that information.
- Made the 'receipts' field requirements stricter.
These rules were probably implied anyway, but now they are explicit.
- Rename 'status' to 'batchStatus'
There is a 'status' field in each 'receipt', which is a hex string. Having another 'status' field, that is not of the same type, appears to be confusing.
- Specified 5 separate 'batchStatus' values
It seems like the previous two values did not cover the entire set of possible 'sendCalls' outcomes. The new status values are: PENDING, SUCCESS, PARTIAL, FAILURE and DISCARDED.
- Added 'chainId' array to the 'wallet_getCapabilities' request
It seems like a wallet that supports a very long list of chains would be forced to return an unusually large JSON object as a response. However, as most dapps know which chains they may run on, it makes sense for them to limit the scope of the request.
- Added SemVer 'version' parameter to capabilities.
It is not likely that all 'capabilities' will reach their final version from day one. As there was no built-in mechanism for versioning, and having no centralized "capabilities authority", it would be pretty likely to see capabilities like 'paymasterServiceV2' and 'atomicBatch_3' proliferate, creating a lot of confusion.
Specify that the 'identifier' must be a 32 byte hex.
@forshtat I'm ok with constraining it to hex, but 32 bytes makes things difficult. For example, in our case the calls id packs the chain id and user op hash together, which we wouldn't be able to do with 32 bytes. Especially with conversations around allowing calls across multiple chains, it would be inevitable that wallets would need to have some more complex state to store + report statuses, which they would not be too happy about.
how would you feel about hex of any length?
Specify that the 'identifier' must be a 32 byte hex.
@forshtat I'm ok with constraining it to hex, but 32 bytes makes things difficult. For example, in our case the calls id packs the chain id and user op hash together, which we wouldn't be able to do with 32 bytes. Especially with conversations around allowing calls across multiple chains, it would be inevitable that wallets would need to have some more complex state to store + report statuses, which they would not be too happy about.
how would you feel about hex of any length?
Hmm. I assumed the identifier was just some kind of a hash of the request, so there is never a need to parse the identifier to learn any information, but to simply look it up in memory.
I guess the identifier could be used to contain some data, but intuitively it feels like a bit of an anti-pattern.
In your case the identity of a batch is {chainId, userOpHash}, right? Does it create a lot of overhead if you had to "remember" the chainId-of-the-userOpHash mapping for the period that the wallet_getCallsStatus is expected to remember the identifier?
If I recall correctly, the userOpHash includes a chainId anyways so there is basically no chance of hash collision between two User Operations on different chains.
Anyways, maybe instead of "any length" we should just pick a length that makes most sense and just left-pad it with zeroes?
Does it create a lot of overhead if you had to "remember" the chainId-of-the-userOpHash mapping for the period that the wallet_getCallsStatus is expected to remember the identifier?
It does. Because in our case, and in the case of some other wallet teams we've been chatting with, getCallsStatus will route to a server so an app can get a status without opening an extension / popup / app. So either:
- The relevant information needs to be packed into the identifier or
- This server needs to store a mapping of an arbitrary identifier to the relevant ops.
Maybe it's inevitable, but we (and presumably other wallets) would like to avoid (2) if possible, as it would require introducing some storage into these servers.
64 bytes would be sufficient if:
- We keep calls on a single chain and
- We are talking about 4337 wallets.
While user op hashes hash over the chain id, we would still need to pack the chain id and user op hash together in the identifier so we know which bundler to call.
This fails about if we start to talk about making the RPC multichain though, as we could never guarantee all the info we need would fit into whatever max length we specify. It also does not work well for EOAs that would need to send multiple transactions for multiple calls and keep track of multiple transaction hashes.
RE: 64 bytes would be sufficient if... This fails about if we start to talk about making the RPC multichain though, as we could never guarantee all the info we need would fit into whatever max length we specify.
Doesn't it raise a concern that the identifier size may end up growing out of control as long as extra information has to be injected into it? That basically the ability to use unrestrictedly large byte arrays as identifiers would be seriously abused?
Like, technically, Buffer.from(JSON.stringify(userOperationsArray)).toString('hex') would also be a valid "any length hex identifier", wouldn't it?
Doesn't it raise a concern that the identifier size may end up growing out of control as long as extra information has to be injected into it?
Yeah, that's why I'm starting to think it's inevitable that wallets will need to have some type of storage in their backends. Could alternatively tag on extra calldata with relevant info (unfortunately at the cost of the user).
For now though assuming we stay with one chain as you have here can we say 64 bytes? This would be sufficient for wallets that can do atomic batching.
For now though assuming we stay with one chain as you have here can we say 64 bytes?
Done.
Hi! I'm a bot, and I wanted to automerge your PR, but couldn't because of the following issue(s):
- Trying to change EIP 5792 state from Review to Last Call
