-
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: Editorial changes #7134
Conversation
Just some quick random updates, still making changes as I go.
✅ All reviewers have approved. |
### Provider Info | ||
|
||
Each wallet provider will be announced with the following interface `EIP6963ProviderInfo` that will be used to display to the user: | ||
Each wallet provider will be announced with the following interface `EIP6963ProviderInfo`. The values in the `EIP6963ProviderInfo` MUST be used as follows: | ||
|
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.
Note, I dropped the walletId
property here because UUIDv4 + Wallet Display Name effectively achieves the same goal to uniquely identify the providers I believe. Curious to get some feedback on this change which is the only technical change in this PR at this point. I can split it out into a separate PR as well if it's a bit more controversial.
@@ -296,3 +229,100 @@ The use of SVG images introduces a cross-site scripting risk as they can include | |||
## Copyright | |||
|
|||
Copyright and related rights waived via [CC0](../LICENSE.md). | |||
|
|||
|
|||
[^rfc4122]: |
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.
|
||
```typescript | ||
function onPageLoad() { | ||
const providers: EIP6963ProviderDetail[] = []; | ||
function onPageLoad(): EIP6963ProviderDetail[] => { |
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.
function onPageLoad(): EIP6963ProviderDetail[] => { | |
function onPageLoad() => { |
Return types are inferred in TypeScript.
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.
Nit: Shouldn’t it ideally return a cleanup function if anything though? To clean up the addEventListener
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.
Potentially, but it's not required so that would be up to an implementer to decide if they want to take that approach. My goal with these changes is to rely upon the text to decide what should be done not the examples. The examples, are there more to make the text a bit more clear.
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.
Makes sense!
Looking good! |
The commit 30c0d5f (as a parent of b01fb07) contains errors. |
### Window Event | ||
|
||
Different wallet providers might inject their scripts at different times, plus there is no guarantees that the Ethereum library in the web page will be loaded before all injected scripts are present in the page. | ||
In order to prevent provider collisions, the DApp and the Wallet are expected to emit an event and instantiate an eventListener to discover the various Wallet's. This forms an Event concurrency loop. |
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.
While "Wallet" is used as a term, I don't think an apostrophe is needed for pluralizing 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.
All Reviewers Have Approved; Performing Automatic Merge...
### Interfaces | ||
|
||
Standardizing a provider info interface (`EIP6963ProviderInfo`) allows determining the necessary information to populate a wallet selection popup. This is particularly useful for Ethereum libraries such as Web3Modal, RainbowKit, Web3-Onboard, ConnectKit, etc. | ||
The DApp MUST listen for the `CustomEvent` dispatched by the Wallet via a `window.dispatchEvent()` function call. In order for the DApp to discover the `EIP6963AnnounceProviderEvent` this EventListener MUST have a `type` property with the string `eip6963:requestProvider`. |
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.
The DApp actually listens to the "announce" event:
The DApp MUST listen for the `CustomEvent` dispatched by the Wallet via a `window.dispatchEvent()` function call. In order for the DApp to discover the `EIP6963AnnounceProviderEvent` this EventListener MUST have a `type` property with the string `eip6963:requestProvider`. | |
The DApp MUST listen for the `CustomEvent` dispatched by the Wallet via a `window.dispatchEvent()` function call. In order for the DApp to discover the `EIP6963AnnounceProviderEvent` this EventListener MUST have a `type` property with the string `eip6963:announceProvider`. |
Just some editorial updates mainly so far, still making changes as I go. Wanted to get this published so others can provide feedback as I'm finishing up.