-
Notifications
You must be signed in to change notification settings - Fork 5.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update EIP-6963: Move to Review #7034
Conversation
File
|
|
Thank you @everdimension for the feedback! 🙏
|
The commit 082b591 (as a parent of b5ef6fb) contains errors. |
It works. Let's ship it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some additional feedback:
- We should be requiring the
EIP6963ProviderDetail
,EIP6963ProviderInfo
andprovider
objects to be frozen viaobject.freeze
. This prevents prototype pollution of the provider and information related to the provider. - Provider detail should not be explicitly coupled to EIP-1193. Doing so will unnecessarily lock this spec into EIP-1193 and require us to break compatibility again if this interface is changed.
- How do we plan to enforce the global unique identifier of the
walletId
parameter? To me this seems like it's implied that it should be done with reverse domain notation. How would you expect a test for this to be added to the wallet-test-framework under the current definition? - The identifiers in the
identifier
section are reversed. UUIDv4 is a globally unique identifier based on it's cryptographic entropy. The example identifiers are examples of localized identifiers because there's no coordinating registry nor conflict resolution mechanism to provide a reasonable guarantee that these are globally unique. We strongly discourage lossy formats like JPG/JPEG
should use normative language. Would we prefer to require these formats to not be used (MUST NOT) or make it a suggest based on SHOULD NOT/MAY NOT language?- Is there any reason to not require the
icon
property in theEIP6963ProviderInfo
is not defined as a data URI rather than allowing it to be any URI? By allowing images to be set via this property in a linking structure we encounter an issue. The URL can be taken down or encounter network issues in a way that leaves it possible that the image won't be displayed. - We should state that Wallet's "SHOULD NOT" support
window.ethereum
anymore. The reason for this is because otherwise DApp's will file issues on wallets saying that their DApp is broken now because they aren't usingwindow.ethereum
and this allows us to point to this spec as a reason for not adding support for it and suggest the DApp updates to support EIP-6963 or a compatible wallet adapter library. - We should add a privacy consideration here that provides a suggestion to consider whether the provider should be announced on every page load or if consent should be gathered from the user ahead of time. For example, extensions could check whether the page has registered a
requestProvider
Event and if so prompt the user if they'd like to announce their wallet to reduce the extension fingerprint. The reason this is optional today is because likely most wallets will announce the provider on page load right away to maintain compatibility with the EIP-1193 pattern. However, when combined with a different provider object (because number 2 would make this an intentional extension point) then we open the door for the user to be able to consent to the wallet being announced, wallet addresses being provided, and RPC namespaces being announced all within a single call after the user consents to it. Essentially, we'd have a pattern where the DApp can now initiate the interaction and the wallet can opt to respond to it or ignore it which would match the behavior of a user without a wallet and therefore eliminate the fingerprinting concern.
For number 2, instead we should add an interface
parameter to the EIP6963ProviderInfo
which allows us to indicate the provider interface such that we can expand beyond the EIP-1193 interface as it's currently defined. Then, inside the EIP6963ProviderDetail
object we can untie the provider
property in a way that allows for this interface to be extensible.
There's little reason for us to tie the discovery mechanism of the provider to the actual interface of the provider and this change would allow us to not ossify 1193 as the required method to achieve this.
Thanks @kdenhartog for the very thorough review and feedback 🙏 Here are my replies for each point you mentioned above:
TLDR' I personally agree with all of your feedback and I think we should start a PR to address all points except for points 2 and 7 |
Sounds good, I'll get a PR going for these changes. Regarding our disagreement on 2 and 7, here's my response. Can you define what qualifies as a breaking change for you? I think we keep talking past each other on this point and it's causing confusion on the goals each of us is trying to achieve with our positions on 2 and 7. Ultimately, I think our goals are the same. To increase support of EIP-6963 (purpose of 7) and create an eventual path of support for CAIP-25 (purpose of 2). Where we may differ is in how we believe that should be achieved is all. For me, a change in the default protocol used to communicate between DApp and Wallet constitutes a breaking change. I'm inferring that for you a breaking change ONLY occurs if the interface is changed. Is that correct? If so here's where I'm not aligned with that definition. Currently, the primary method used for extension wallets is EIP-1193 as an interface and window.ethereum as the transport of arbitrary messages between the wallet and DApp. The combination of the interface and the transport method defines the protocol in my definition. Therefore, changing the protocol is inherently a breaking change even if it's backwards compatible because it's possible that either participant of the protocol may opt to no longer support the old version and only support EIP-6963+EIP-1193. Similarly, either participant may opt to not support the new protocol. This is similar to a browser maintaining support of TLS 1.0/1.1/1.2/1.3 but a server only opting to support TLS 1.2 or 1.3 because 1.0 and 1.1 have been officially deprecated. Even though the TLS protocol hasn't undergone major breaking changes and maintains backwards compatibility, lack of support by either participant can still present interoperability constraints and therefore "break" compatibility. Hence, why I'm defining this change as a "breaking change". EIP-6963 is suggesting that wallets and DApps support new behavior so that we no longer prototype pollute the window.ethereum object. I understand it's possible to support both, but there's no guarantee that both parties will leading to potential compatibility issues. Furthermore, I don't believe we should encourage new DApps to support the old protocol. Wallets will likely need to maintain support even if we officially deprecated window.ethereum+EIP-1193. This is similar to the blink tag in HTML being deprecated officially yet still supported by most browsers. So, by adding language to suggest that supporters of EIP-6963 "SHOULD NOT" support the old method we're creating a scenario where DApps can say, "We no longer support the defacto standard of window.ethereum instead we now only support EIP-6963 and you should be using a wallet that supports it". This is the reason I chose "SHOULD NOT" rather than "MUST NOT". By doing this we're placing a signal within the standard to suggest "hey this is the best current practice of how to communicate between a DApp and a Wallet". At some point in the future when most DApps are supporting EIP-6963 we can then disable default support of the current window.ethereum approach and greatly reduce the prototype pollution issue that EIP-6963 has set out to resolve. Regarding number 2, I'm a bit confused. Are you of the opinion that CAIP-25 will reuse Event concurrency loops like we're doing here, but will define a different set of Event names and will define a similar but different ProviderInfo structure? I guess that's where I'm confused. Even if we use your definition of breaking change here, a wallet or DApp that wishes to support every method of communication won't reduce the number of code paths or code complexity to do so. However, by reusing EIP-6963 we can at least reduce the amount of code that needs to be written to support all 3 methods. For reference, I'm thinking of the following 3 methods: Current: window.ethereum + EIP-1193 (*) indicates a similar but non compatible method of arbitrary message transport may be used. |
re: point 7 I see your argument for point 7, I can agree that the language "SHOULD NOT" does not forbid a wallet provider from using I'm ok if we include changes in the next PR to include the feedback for point 7. re: point 2 Personally I don't see anything from EIP-6963 being re-used for future standardization in wallet providers when CAIP-25 and CAIP-27 become production-ready. There are two main reasons for this: a) EIP-6963 does NOT include any feature or capability discovery when the Ethereum Library prompts Wallet Providers to announce themselves. This is something that we would need to change to make it CAIP-25 compatible but it would be a premature optimization for an eventual upgrade that isn't ready b) EIPs can inherit or reference standards or interfaces from CAIPs because they are chain-agnostic. But the vice-versa is not possible because it would then "leak" dependencies on other blockchain ecosystems that are not EVM-compatible. This means that CAIP-25 would need its own discovery mechanism standard independent from EIPs Finally another bonus reason... EIP-1193 and CAIP-25 are too diferent from each other (one is object-based and other is message-based). Generalizing between the two would not only overcomplicate standardization but it also make EIP-1193 design flaws to prevail in the future unnecessarily. My take is that EIP-6963 should be compatible exclusively with EIP-1193 hence it should be deprecated together in the future. TLDR' let's include point 2 in the next PR but I still strongly disagree with point 7 |
Ok, let's chat about number 2 a bit more on a call or something some time soon. I'd at least like to make sure I fully understand your viewpoint on this before we finalize this EIP in case it would be useful to add this extension point since I'm not seeing what the future approach would do instead of EIP-6963. I'm missing a few pieces of the puzzle still (e.g. I think I just realized CAIP-25 is more a replacement of EIP-1102 and EIP-2255 not |
Is there even any benefit to removing Feel like |
Yes, right now any user that installs multiple extension wallets will find that the wallets behave in unexpected ways. For example, Brave regularly (about every 3 to 6 weeks) receives complaints from users that we're breaking their Metamask implementation because they think Brave Wallet is prototype polluting metamask when in reality it's a race condition between the various extensions they have installed. In Brave's case, we can override all extensions, but default to giving Extensions first dibs via |
Honestly this is kinda not true IMHO 😅 A lot of wallets wouldn't exist if it wasn't for some crazy hacks to circumvent the massive issues with Plus if it wasn't for WalletConnect many wallets wouldn't have had a way to get into the market at all because browser extensions were completely captured by users only installing one wallet simultaneously That being said I agree that we MUST NOT break compatibility with Yet what @kdenhartog has suggested is that we use the wording "SHOULD NOT" which essentially strongly recommends that Wallet Providers eventually deprecate the legacy pattern of |
I guess the point I was trying to make is that I don’t want to get in a situation where some Dapps may not work with a particular Wallet because they don’t support 6963 yet or vice-versa. Which is essentially what you are saying. However, I do not agree with adding in the “Wallet Provider SHOULD NOT support For Dapp consumers who DO NOT consume a Wallet Connection library like wagmi, RainbowKit, Web3Modal, etc, the migration path isn’t exactly smooth since we are now inverting control to the Dapp developer to handle the Provider instance themselves (as well as ensuring it is sensibly persisted, frozen, etc). Even those who DO depend on a library that handles Wallet Connection also won’t benefit until they upgrade, which might be a problem for those who do not have the capacity/resources to upgrade (and even maybe migrate across major versions of that library). I think we should be somewhat empathetic here. |
So what would you suggest as the proper wording to indicate this is now the best practice for how a wallet and DApp should communicate and the previous method with known issues should not be implemented any further? |
I believe we should make the intention clear for both Applications & Wallet Providers. In my opinion, it could be along the lines of: "For Applications, it is RECOMMENDED to implement and prioritize EIP-6963 over "For Wallet Providers, it is RECOMMENDED to keep compatibility of |
Ok, I'm generally aligned with this thinking.
I'd suggest one modification to this one, which is to suggest DApps SHOULD disable support for Here's my reasoning for the change. This standard could hold for another 5+ years, so without the push from either direction developer enertia suggests that To support my argument here's some analogous data. As an example, HTTP(s) was implemented in 2000 and it wasn't until Google changed their SEO methods in 2014 (as an incentive to force websites to change) that we started to see HTTP(s) become the more common method of usage. That change in 2014 took an international political scandal (Snowden leaks) to motivate the changes too. Luckily, we don't have as large of a web presence to affect here but the point I'm making here is that words have meaning so the stronger we can go the faster we can notice a large effect. I generally agree with your priority of constituents here as well, where we should lean on the wallets to make the majority of the changes rather than pressing for the DApp developers to take on the burden of change. The reasoning this makes sense to me is there's only 10s of wallets but there's 1000s of DApps, so bootstrapping adoption is going to be much more easily achieved by placing the burden on wallets. With that in mind, getting to default support of this feature in wallets is going to take substantial adoption by DApps. We're only now just talking about turning on HTTP(s) by default in browsers after 23 years. So the better we can encourage DApp's turning off support of Also, I like the suggestion you're making here by being more explicit about support between the various parties. |
I can agree with this. It'll also push wallets to support 6963 if Dapp developers are screaming at them to support it. |
Sounds good, I'll get that change in with my PR I'm currently working on for this. |
@kdenhartog some feedback collected from Telegram feedback: |
Here's the beginning of a PR for the changes being updated #7134 |
Ready
…On Fri, Jun 2, 2023, 9:11 PM Kyle Den Hartog ***@***.***> wrote:
Here's the beginning of a PR for the changes being updated
—
Reply to this email directly, view it on GitHub
<#7034 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/A25NI4EILDBQ3U7VND2AVE3XJKMLVANCNFSM6AAAAAAYDVPX3U>
.
You are receiving this because you are subscribed to this thread.Message
ID: ***@***.***>
|
Closed in favor of #7352 |
This PR updates EIP-6963 status from DRAFT to REVIEW
IMPORTANT! Please comment below any feedback that you might have