Skip to content
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: name change + term consistency + overall copy edits #7824

Closed
wants to merge 46 commits into from

Conversation

darrylyeo
Copy link

@darrylyeo darrylyeo commented Oct 7, 2023

EIP-6963 is all about the relationship between "Dapps" and "Wallet Providers" – I want to ensure the copy and code examples reflect this clearly.

Naming things is hard. This PR should make the explanations more concise and terminology more consistent.

New EIP title:

  • "Multi Injected Provider Discovery"
    • → "Event-Driven Discovery of Wallet Providers"

(The previous title does not do this EIP justice in my opinion – "provider" is an overloaded term, and "injected" is ambiguous as it could refer to the EIP-1193 provider object or the content script of a browser extension. Plus, it needs "Wallet" in the name!)

Notable terminology clarifications:

  • "DApp" / "application"
  • "Wallet" / "provider" / "wallet provider"
    • → "Wallet Provider" (explicitly defined as a "browser extension" for the purposes of this spec)
  • "library" / "Provider Discovery Library"
    • → "Wallet Provider Discovery Library" (usually interchangeable with "Dapp" for the purposes of this spec)
  • "injected Wallet Provider" / "Ethereum provider" / "Ethereum interface" / "EIP-1193 provider" / "Provider object" 😵‍💫
    • → "EIP-1193 provider object"
  • "injected script"
    • → "injected content script", "content script injected by a browser extension", etc.
  • "window.ethereum"
    • → "global JavaScript variable window.ethereum", "EIP-1193 provider object injected at window.ethereum", "legacy window.ethereum behavior", etc.
  • "web page" / "session" / "lifetime of the page" / "over time"
    • → "page session" or "webpage session"
  • "Eip6963ProviderDetail object" / "Wallet Provider"
    • → "EIP-6963 compliant Wallet Provider object", "Eip6963ProviderDetail object sent by Wallet Provider", etc.
  • Events (reference by their JavaScript event name instead of TypeScript interfaces specific to this spec):
    • "Eip6963AnnounceProviderEvent" → "eip6963:announceProvider event"
    • "Eip6963RequestProviderEvent" → "eip6963:requestProvider event"
  • "this EIP"
    • → "EIP-6963"

Minor structural changes:

  • "Wallet Provider info" section:
    • Link to "RDNS identifiers" sub-section (mirroring link to "Icon images" sub-section)
  • "Events" section:
    • Add "Event loop" sub-heading

Minor spec changes:

  • Allow Eip6963RequestProviderEvent to be an Event OR CustomEvent
  • Explicitly disallow Dapps from rendering <svg> tags as-is
  • Allow Dapps to render SVGs as an <img> tag OR XSS-resistant "equivalent means" (such as a CSS background)

@darrylyeo
Copy link
Author

Reverting all proposed normative changes to avoid restarting the Last Call period.

@darrylyeo
Copy link
Author

darrylyeo commented Oct 9, 2023

there are some left, but I consider these as breaking changes.

@glitch-txs As I understand it, this spec is defining JavaScript functionality; the TypeScript syntax is used illustratively, so changing TypeScript interface names used as internal references shouldn't break any JavaScript implementations.

The fact that this EIP doesn't explicitly mandate a TypeScript implementation also motivated my decision to reference events by their JavaScript name ("eip6963:announceProvider event") instead of their internal TypeScript interface ("Eip6963AnnounceProviderEvent") – example. With my proposed copy changes, the TypeScript interface names are for the most part relegated to the TypeScript code snippets + reference implementation.

Using strict TitleCase as in Eip6963 is my personal style preference – not the most important part of my proposal though. Happy to revert to fully capitalized (EIP6963) if others strongly prefer :)

@darrylyeo
Copy link
Author

darrylyeo commented Oct 9, 2023

Also, we shouldn't assume that just because a change is non normative that it doesn't matter. The implications of subtle interpretation differences can be profound. Especially when making changes to examples which are by definition non-normative but often times the most important parts of the spec.

@kdenhartog Agreed 100% with these points – my hope is that by consolidating terminology and applying them as consistently as possible across the document, we can prevent many of these subtle interpretation differences after the EIP moves to Final.

I took great care to preserve the original intent of both the normative and non-normative text as much as possible – happy to address any specific parts where you feel I have not!

@darrylyeo
Copy link
Author

darrylyeo commented Oct 9, 2023

Update: "EIP-1193 Provider Objects" are now front and center with a dedicated definition.

Also reverted some prescriptive language related to browser extensions, as they are not the only way Wallet Providers expose EIP-1193 Provider Objects to Dapps.

@darrylyeo
Copy link
Author

darrylyeo commented Oct 9, 2023

If it helps people read the code diff more easily — my revisions stem from using the four terms under the "Definitions" section as consistently as possible.

If anyone strongly prefers an alternative wording for any of the four terms, let me know and I can easily find-and-replace.


The **`rdns`** (Reverse-DNS) property serves to provide an identifier which DApps can rely on to be stable between sessions. The Reverse Domain Name Notation is chosen to prevent namespace collisions.
The Reverse-DNS convention implies that the value should start with a reversed DNS domain name controlled by the Provider. The domain name should be followed by a subdomain or a product name. Example: `com.example.MyBrowserWallet`.
The `rdns` (reverse-DNS) property serves as an identifier which Dapps can rely on to be stable between page sessions. The Reverse Domain Name Notation is chosen to prevent namespace collisions. The domain should be controlled by the Wallet Provider and be followed by a subdomain or a product name. Example: `com.example.MyBrowserWallet`.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
The `rdns` (reverse-DNS) property serves as an identifier which Dapps can rely on to be stable between page sessions. The Reverse Domain Name Notation is chosen to prevent namespace collisions. The domain should be controlled by the Wallet Provider and be followed by a subdomain or a product name. Example: `com.example.MyBrowserWallet`.
The `rdns` (reverse-DNS) property serves as an identifier which Dapps can rely on to be stable between page sessions. The Reverse Domain Name Notation is chosen to prevent namespace collisions. The domain SHOULD be controlled by the Wallet Provider and MAY be followed by a subdomain, product name, or other discriminant. Example: `com.example.MyBrowserWallet`.

@glitch-txs
Copy link
Contributor

glitch-txs commented Oct 10, 2023

I think we should keep the previous name for types. Imo it is a breaking change since most of us use typescript and have this EIP as reference/source of truth.

About the title I just wanted to point out that Wagmi has a library called mipd and some wallets are already using it, sharing it just to be aware of early implementations.

@darrylyeo
Copy link
Author

darrylyeo commented Oct 10, 2023

I think we should keep the previous name for types. Imo it is a breaking change since most of us use typescript and have this EIP as reference/source of truth.

@glitch-txs I'd be inclined to agree if the EIP explicitly mentioned using TypeScript as a normative requirement.

While most existing/early implementations today are TypeScript-based it may not be that way in the future. Along that front, I'm trying to avoid being overly prescriptive, especially for current or future implementers who may not prefer to use TypeScript.

Copy link

There has been no activity on this pull request for 2 weeks. It will be closed after 3 months of inactivity. If you would like to move this PR forward, please respond to any outstanding feedback or add a comment indicating that you have addressed all required feedback and are ready for a review.

@github-actions github-actions bot added the w-stale Waiting on activity label Mar 11, 2024
Copy link

This pull request was closed due to inactivity. If you are still pursuing it, feel free to reopen it and respond to any feedback or request a review in a comment.

@github-actions github-actions bot closed this Apr 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c-update Modifies an existing proposal t-interface w-stale Waiting on activity
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants