Add optional id #20
Add optional id #20
Conversation
EIPS/eip-5792.md
Outdated
| The identifier MUST be unique 64 bytes represented as a hex encoded string. | ||
| If the `id` field was provided by the app, the wallet MUST return the same value in the response. | ||
|
|
||
| Otherwise, the identifier MUST be a unique string up to 4096 bytes (8194 characters including leading `0x`). |
There was a problem hiding this comment.
Should the application have the same restriction? I think any app would be able to deal with it, and it can offer more guarantees to the wallet.
EIPS/eip-5792.md
Outdated
| The identifier MUST be unique 64 bytes represented as a hex encoded string. | ||
| If the `id` field was provided by the app, the wallet MUST return the same value in the response. | ||
|
|
||
| Otherwise, the identifier MUST be a unique string up to 4096 bytes (8194 characters including leading `0x`). |
There was a problem hiding this comment.
I suggest rephrasing this so that the "4096 bytes" limitation also applies in case the id is provided by the app.
Also, is there still a need to have 4096 byte IDs if we have client provided IDs? I still think the use-cases for large IDs with information encoded into them should be discussed in more detail.
There was a problem hiding this comment.
agreed that the same limit should apply for both app-provided and wallet-provided ids. will update.
i dont think data encoded in the ids should be discussed in the EIP -- apps should generally treat the ids as opaque , and if they have data encoded in them it is fully the wallet's responsibility to encode / decode how they wish. if the app chooses to encode data in the ids, it is still up to them to keep track of that however they wish. i dont see a need to put any other constraints
EIPS/eip-5792.md
Outdated
| The identifier MUST be unique 64 bytes represented as a hex encoded string. | ||
| If the `id` field was provided by the app, the wallet MUST return the same value in the response. | ||
|
|
||
| Otherwise, the identifier MUST be a unique string up to 4096 bytes (8194 characters including leading `0x`). |
There was a problem hiding this comment.
I would just remove Otherwise as it might imply the dapp has no limitations on the ID while the wallet does:
The identifier MUST be a unique string up to 4096 bytes (8194 characters including leading `0x`).
EIPS/eip-5792.md
Outdated
| @@ -73,6 +77,7 @@ type Capability = { | |||
|
|
|||
| type SendCallsParams = { | |||
| version: string; | |||
| id?: string | undefined; | |||
There was a problem hiding this comment.
| id?: string | undefined; | |
| id?: string; |
I think this covers the optional case on its own? I'm no typescript developer though.
There was a problem hiding this comment.
it does but the goat @jxom does it in viem so im doing it here. maybe he can explain why
There was a problem hiding this comment.
Depending on your tsconfig id?: string and id?: string | undefined are different.
You can set tsc to reject passing undefined if it was declared without | undefined.
There was a problem hiding this comment.
Since this is a JSON interface, undefined isn't possible.
| @@ -62,6 +62,10 @@ The wallet: | |||
| * 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` (either top-level or call-level) that is not supported by the wallet and the `capability` is not explicitly marked as optional. | |||
| * Applications may mark a capability as optional by setting `optional` to `true`. See the RPC Specification section below for details. | |||
| * If provided, the wallet MUST respect the `id` field and return it in the response. | |||
| * App-provided `id`s MUST be unique per sender per app. | |||
| * Wallets MUST reject requests with duplicate `id`s. | |||
There was a problem hiding this comment.
Should we talk about namespacing here? If two instances of the same domain are in two separate tabs, can they see each other's batches? How about different domains?
There was a problem hiding this comment.
glad you brought this up. imo "per app" should be interpreted as "per domain". example:
- i use app.com on my phone with address "0x123"
- i then use app.com on my desktop with the same address
in this case, the wallet should block a duplicate id and it is the wallet's responsibility to keep track.
open to other ideas though / suggestions on how to word here !
There was a problem hiding this comment.
glad you brought this up. imo "per app" should be interpreted as "per domain". example:
I think this is good, as it's aligned with web developer expectations, as it mimics Same Origin Policy. We may even use the word origin here, instead of app.
Another option is not making it unique per app, but rather globally unique, and recommend the app developers to use random ids. If they get two things for free: their ids are unique, and they can't be enumerated by other apps.
There was a problem hiding this comment.
would prefer to keep the per app / origin constraint. i imagine plenty of people, whether for testing reasons or other, will just have non-random, incrementing or other simple ids. i think it would be very strange behavior if these cases caused the wallet to reject because of the other apps that were using simple ids.
forshtat
merged commit 10f6d85
into
eth-infinitism:eip-5792-proposal
idto wallet_sendCalls parameters