Update EIP-5792: Propose changes to EIP-5792 #8826

forshtat · 2024-08-25T20:35:40Z

List of changes proposed in this PR and their motivations:

Rephrase "The items in the calls field are only those that are shared by all transaction types" and "method only returns a subset of the fields that eth_getTransactionReceipt returns, 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.

eth-bot · 2024-08-25T20:36:05Z

File `EIPS/eip-5792.md`

Requires 1 more reviewers from @arein, @drortirosh, @jxom, @lukasrosario, @moodysalem, @wilsoncusack

EIPS/eip-5792.md

shahafn · 2024-08-26T11:28:22Z

EIPS/eip-5792.md

 };
 ```

+##### Status Codes for `batchStatus` field


Maybe we should define what a batch is first? it's the first time we use it

Co-authored-by: shahafn <[email protected]>

wilsoncusack · 2024-08-27T11:20:21Z

EIPS/eip-5792.md

  calls: {
    to?: `0x${string}` | undefined;
    data?: `0x${string}` | undefined;
    value?: `0x${string}` | undefined; // Hex value
-    chainId?: `0x${string}` | undefined; // Hex chain id
+    capabilitiesData?: Record<string, any> | undefined;


hmm, I don't love how this kind of muddles the simplicity of the calls struct. Is the complaint that capabilities makes it hard to specify info on a per call basis. Could we get a better example of when this would be needed 🤔

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

This should be possible with a general capability? Or you're saying one call in particular can revert?

You don't have to implement it, if our wallet doesn't have any per-call capability.
Also, an app doesn't have to know about it, unless it needs that capability.
The idea that if someone ever want to add per-call capability, it doesn't mess directly with the "call" structure, or with such params of another capability.

Is the complaint that capabilities makes it hard to specify info on a per call basis?

Yes, that pretty much sums it up. Of course, the capability could provide an array of details, which would have to match the calls array's length exactly and contain null values for calls that do not require extra parameters, which is functionally equivalent but seems like a much more quirky API to me.

Or you're saying one call in particular can revert?

In my example, yes, I was talking about an "atomic" batch with a single "optional" call that should not revert the batch;
It is hard to reason about potential future additional examples, so these are a little hypothetical, but in my opinion, worth considering:
If a "per-call gas limit capability" is defined, it can be used to provide a per-call gas limit;
In case a "multi-chain capability" is defined, the 'capabilitiesData' field can be used to specify a per-call 'chainId' and L2-specific parameters, something like gas_per_pubdata_limit;
In case a "conditional call capability" is defined, it can provide some description of a condition to include or exclude a call from the batch;
etc.

Does this make sense? Or should dapps never need to go to the trouble of defining per-call parameters for a sendCalls request?

wilsoncusack · 2024-08-27T11:21:09Z

EIPS/eip-5792.md

+The purpose of the `batchStatus` field is to provide a short summary of the current status of the batch.
+It provides some off-chain context to the array of inner transaction `receipts`.
+
+| Name      | Description                                                                                                          |


I like this idea, thoughts @jxom @lukasrosario ?

yeah i think this makes sense if we're talking about single chain. would be tough if multichain.

also not clear to me where DISCARDED would come up. @forshtat can you share more on how you were thinking this might come up?

Yes, it sounds similar to "FAILURE" but the difference I had in mind was that a "FAILURE" means the action failed due to on-chain reverts, and "DISCARDED" means the wallet gave up on trying to send the batch to the network.
Maybe the names do not make the distinction clear enough, or do you think the distinction is not worth the extra status code?

One example could be a batch that specifies a paymasterService capability, and given the wallet need to go through these steps before sending anything on-chain:

The wallet cannot block the wallet_sendCalls() for all this time, so returns some identifier and proceeds with preparing the UserOperation.
During the preparation phase the wallet_getCallsStatus returns PENDING as nothing really happened.
However, if the "Paymaster Service" fails with an error at some stage, wallet could keep retrying to reach it for some time, and the transaction has not technically "failed", but after 2 minutes of trying to reach the "Paymaster Service" it gives up. I thought that this action's status should be marked as "DISCARDED" instead of "FAILURE".

Another example is a UserOperation that has some condition in its validation function. Canonical ERC-4337 limits this use-case a lot, but does not completely prevent it. A wallet may want to accept a batch that is not yet valid, check if it has become valid during the next 2 hours, and mark the entire operation as "DISCARDED" after that.

Does this makes sense?

yet another example of "discarded": assume you put a maxFeePerGas that is too low, and after sometime , because the network gas raised, the maxFee becomes too low below the baseFee, and the transaction is completely dropped. If we are sending a "sequence", it can happen to any transaction along the sequence.
Now this transaction didn't technically "fail", since it didn't get anything on-chain - but definitely there's no reason to continue and wait for it.

wilsoncusack · 2024-08-27T11:22:14Z

EIPS/eip-5792.md

 * MUST NOT await for any calls to be finalized to complete the batch
 * MUST NOT send any calls from the request if the user rejects the request
 * MAY revert all calls if any call fails
 * MAY send all calls as part of one or more transactions, depending on wallet capability
 * SHOULD stop executing the calls if any call fails
 * MAY reject the request if the from address does not match the enabled account
 * MAY reject the request if one or more calls in the batch is expected to fail, when simulated sequentially
+* MUST reject the request if it contains a `capability` that is not supported by the wallet


Hmm I wonder if this is best 🤔 . E.g. why not allow specifying a paymaster which the app would like to be used but is not a necessity.

How a wallet can tell if the cap (e.g. paymaster) is required by this app or optional?
The app can always query the wallet caps first, to decide how to continue.

capability specs should specify whether they're required or not / how optional ones are handled. paymaster, for example, notes that the app must not assume the provided paymaster service is the one that ends up being used.

so if an app wants the capability to be required, the capability spec should include a required field, and that is how a wallet can tell if the capability is required by the app. i dont think we should blanket all capabilities to be required by default.

There are two issues raised here:

App providing a "capability" that is not supported by the wallet:

There is a wallet_getCapabilities method that provides the application with a list of supported "capabilities".
Shouldn't the app that ignores that list and submits a wallet_sendCalls with a "capability" that is not supported be considered to behave incorrectly? I think it should reject the call, which would prevent dapps from yoloing their sendCalls requests.

Wallet discretion to use or ignore a supported "capability" requested by the dapp:

As the API defined here deals with real value and assets, making capabilities required by default makes a lot of sense to me. If using a paymaster for a transaction is indeed optional, it is trivial to specify optional: true, but forgetting to set required: true for a security-oriented capability may be catastrophic.

What do you think?

So @forshtat is the conclusion here that each capability could have an optional: flag? But passing a capability the wallet doesn't have would be hard failure?

So @forshtat is the conclusion here that each capability could have an optional: flag? But passing a capability the wallet doesn't have would be hard failure?

In my personal opinion, yes, this seems pretty reasonable.

wilsoncusack · 2024-08-27T11:23:47Z

EIPS/eip-5792.md

    blockHash: `0x${string}`;
    blockNumber: `0x${string}`;
    gasUsed: `0x${string}`;
    transactionHash: `0x${string}`;
  }[];
+  capabilitiesData?: Record<string, any> | undefined;


I find it kinda confusing that this is the same phrase as on the request, but would likely be different data?

yes, it is per-capability result data, in the results structure. Should it be named capabilitiesResultsData ?
I think it is clear enough, simply because it is in the response .

I fully support renaming this parameter. capabilitiesResultsData is a bit long, how about just capabilitiesResults or capabilitiesStatus?

I prefer clarity even if verbose so I think I like capabilitiesResultsData

I prefer clarity even if verbose so I think I like capabilitiesResultsData

Done.

lukasrosario · 2024-08-28T12:29:21Z

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?

forshtat · 2024-08-28T13:29:16Z

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?

lukasrosario · 2024-08-28T19:02:08Z

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.

forshtat · 2024-08-29T12:14:10Z

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?

lukasrosario · 2024-09-03T15:00:39Z

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.

…ResultsData'

forshtat · 2024-09-04T13:16:31Z

For now though assuming we stay with one chain as you have here can we say 64 bytes?

Done.

lukasrosario · 2024-11-15T06:31:57Z

EIPS/eip-5792.md

@@ -122,38 +136,55 @@ Returns the status of a call batch that was sent via `wallet_sendCalls`. The ide
 type GetCallsParams = string;

 type GetCallsResult = {
-  status: 'PENDING' | 'CONFIRMED';
+  batchId: `0x${string}`;


should keep the name the same

ajhodges · 2024-11-15T14:42:59Z

EIPS/eip-5792.md

+
+| Name      | Description                                                                                                          |
+|-----------|----------------------------------------------------------------------------------------------------------------------|
+| PENDING   | Batch has been received by the wallet but has not completed execution on-chain                                       |


How do you feel about possibly expanding PENDING to capture more of the offchain context around the pending state? How can a caller know if it needs to retry a batch with a higher gas price, or if there is some transient infra issue with getting batches included?

For example

PENDING: Batch has been received by the wallet but has not completed execution on-chain

UNDERPRICED or PENDING_RETRYABLE: Batch has been received by the wallet but the gas prices are too low for the current chain conditions

STALLED: Batch has been received by the wallet but is stuck for non-pricing related reasons; i.e. ERC-4337 bundler is down or the chain has halted. The definition of 'stuck' is left up to the wallet, i.e. batch is priced appropriately but has not been included in >60s

@forshtat would be good to get your thoughts here. imo UNDERPRICED specifically seems like a reasonable addition, as it could be retried.

@ajhodges i'm not a huge fan of the ambiguity for STALLED though. to me this is effectively still PENDING, and each wallet should have their own interpretations of PENDING taking "too long". if we and others feel strongly about it being its own status, i would prefer to be more opinionated in the spec and define when a PENDING batch becomes STALLED, instead of leaving it open-ended.

What if we just left it as PENDING and CONFIRMED, and had a sibling statusCode or detail property that could be extensible enough to accommodate for what ever addition?
This would make it a lot easier to coerce on frontend apps.

What if we just left it as PENDING and CONFIRMED, and had a sibling statusCode or detail property that could be extensible enough to accommodate for what ever addition? This would make it a lot easier to coerce on frontend apps.

Should the detail property be a human-readable string in this case, to provide the error from the wallet directly to the app?
This can be done as a last resort but should not be the default way of providing a data to the application in my opinion. Wallets may have very technical error messages and apps don't want to show them to users, and will try to parse the detail strings.
If detail is not supposed to be human-readable, it is still basically a second "status code" parameter and maybe we can merge it back to the statusCode field?

UNDERPRICED specifically seems like a reasonable addition, as it could be retried.

I don't have any strong feelings against UNDERPRICED or any other status code we can clearly define.

About UNDERPRICED specifically, I guess its difference with the regular PENDING state is that the user needs to confirm a modification to the original request? And if the wallet could just increase the price internally and retry, it would not be different from PENDING, would it?

If so, maybe a better broader category would be ACTION_REQUIRED or something like that, what do you think?

Yeah, I guess statusCode might be more realistic than detail as an application can choose which message they want to display to an end-user (if anything).

Perhaps this could be a similar pattern to HTTP status codes where 2xx ≈ success, 4xx ≈ client error, 5xx ≈ server error, etc. For this case, maybe 1xx represents PENDING sub-statuses, 2xx represents CONFIRMED sub-statuses, and so on. For example, a status of PENDING and a statusCode of 101 could mean “stalled”, and an application could either choose to show a generic loading indicator, or a loading indicator with “this tx is taking a while, pls wait”.

I guess the motivation of a sub-status field like statusCode is that it would make the branching logic within an application so much easier — they can rely on the simplicity of status and choose to show extra information by utilizing statusCode.

I guess the motivation of a sub-status field like statusCode is that it would make the branching logic within an application so much easier — they can rely on the simplicity of status and choose to show extra information by utilizing statusCode.

Yep, exactly. As an app, I want to know if things are healthy. Is my transaction stuck? Is there anything I can do about it if it's stuck? (i.e. underpriced vs stalled). I think the display logic/messaging to the user should be left up to the app, but the wallet can oftentimes provide additional context about what happened to make the UX a little better.

I agree UNDERPRICED is maybe a bit vague. But the wallet should be the expert on the current chain conditions and the bundle pricing so it's best suited to detect if the calls should be retried (and we shouldn't prescribe any conditions in this EIP). So maybe RETRYABLE is a better sub-status than UNDERPRICED, signifying that the calls might go through quicker if you retry. I think I like this better than ACTION_REQUIRED because technically an underpriced tx can still go through if chain conditions change

Propose changes to EIP-5792

40d79c7

forshtat requested a review from eth-bot as a code owner August 25, 2024 20:35

github-actions bot added c-update Modifies an existing proposal t-interface labels Aug 25, 2024

eth-bot added the a-review Waiting on author to review label Aug 25, 2024

eth-bot changed the title ~~Propose changes to EIP-5792~~ Update EIP-5792: Propose changes to EIP-5792 Aug 25, 2024

shahafn reviewed Aug 26, 2024

View reviewed changes

EIPS/eip-5792.md Outdated Show resolved Hide resolved

shahafn reviewed Aug 26, 2024

View reviewed changes

Update EIPS/eip-5792.md

a97f3a2

Co-authored-by: shahafn <[email protected]>

wilsoncusack reviewed Aug 27, 2024

View reviewed changes

Rename status response field from 'capabilitiesData' to 'capabilities…

d14ce17

…ResultsData'

github-actions bot added c-status Changes a proposal's status and removed c-update Modifies an existing proposal labels Sep 4, 2024

Change batch identifier size from 32 bytes to 64 bytes

0ecea7b

poojaranjan mentioned this pull request Oct 29, 2024

EIP Editing Office Hour Meeting 46 ethcatherders/EIPIP#363

Closed

58 tasks

lukasrosario reviewed Nov 15, 2024

View reviewed changes

ajhodges reviewed Nov 15, 2024

View reviewed changes

Rename 'batchId' to 'callsId'

b364b92

Update EIP-5792: Propose changes to EIP-5792 #8826

Are you sure you want to change the base?

Update EIP-5792: Propose changes to EIP-5792 #8826

Conversation

forshtat commented Aug 25, 2024

eth-bot commented Aug 25, 2024 • edited Loading

File EIPS/eip-5792.md

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

forshtat Aug 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lukasrosario commented Aug 28, 2024 • edited Loading

forshtat commented Aug 28, 2024

lukasrosario commented Aug 28, 2024 • edited Loading

forshtat commented Aug 29, 2024

lukasrosario commented Sep 3, 2024

forshtat commented Sep 4, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ajhodges Nov 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jxom Nov 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jxom Nov 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eth-bot commented Aug 25, 2024 •

edited

Loading

File `EIPS/eip-5792.md`

forshtat Aug 28, 2024 •

edited

Loading

lukasrosario commented Aug 28, 2024 •

edited

Loading

lukasrosario commented Aug 28, 2024 •

edited

Loading

ajhodges Nov 15, 2024 •

edited

Loading

jxom Nov 16, 2024 •

edited

Loading

jxom Nov 21, 2024 •

edited

Loading