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-5749: Move to last call #6018

Merged
merged 14 commits into from
Nov 24, 2022
Merged

Update EIP-5749: Move to last call #6018

merged 14 commits into from
Nov 24, 2022

Conversation

kvhnuke
Copy link
Contributor

@kvhnuke kvhnuke commented Nov 21, 2022

please refer to #5912 (comment) to comments regarding title and the reason for evmproviders

@kvhnuke kvhnuke requested a review from eth-bot as a code owner November 21, 2022 23:17
@github-actions github-actions bot added c-status Changes a proposal's status t-interface labels Nov 21, 2022
@eth-bot
Copy link
Collaborator

eth-bot commented Nov 21, 2022

A critical exception has occurred:
Message: pr 6018 is already merged; quitting
(cc @alita-moore, @mryalamanchi)

@Pandapip1 Pandapip1 changed the title EIP-5749: Move to last call Update EIP-5749: Move to last call Nov 21, 2022
Copy link
Member

@Pandapip1 Pandapip1 left a comment

Choose a reason for hiding this comment

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

Line 18 has an incomplete sentence.
Line 33 has a random ..etc. This should be removed, since it doesn't add anything and doesn't make sense in the context of the list ("et cetera" is not an example).
Line 35 has a small mistake: it says "only display" when it should really be "display" (web3modal and web3onboard will still show potentially uninstalled options by the nature that they don't inject either window.ethereum or window.evmproviders)
Line 68: The "well" in "well documented" can be removed
Line 71 and 78: ``` should be replaced with ```text
Line 87-91: This list should be inlined so that it's all one sentence.
Line 122: "make sure" should be replaced with "ensure"

@kvhnuke kvhnuke requested a review from Pandapip1 November 21, 2022 23:54
@kvhnuke
Copy link
Contributor Author

kvhnuke commented Nov 22, 2022

@Pandapip1 let me know if I miss anything, all requested changes should be done

EIPS/eip-5749.md Outdated Show resolved Hide resolved
EIPS/eip-5749.md Outdated Show resolved Hide resolved
EIPS/eip-5749.md Show resolved Hide resolved
EIPS/eip-5749.md Outdated Show resolved Hide resolved
EIPS/eip-5749.md Outdated Show resolved Hide resolved
EIPS/eip-5749.md Outdated Show resolved Hide resolved
EIPS/eip-5749.md Outdated Show resolved Hide resolved
kvhnuke and others added 10 commits November 22, 2022 13:29
Co-authored-by: Pandapip1 <45835846+Pandapip1@users.noreply.github.com>
Co-authored-by: Pandapip1 <45835846+Pandapip1@users.noreply.github.com>
Co-authored-by: Pandapip1 <45835846+Pandapip1@users.noreply.github.com>
Co-authored-by: Pandapip1 <45835846+Pandapip1@users.noreply.github.com>
Co-authored-by: Pandapip1 <45835846+Pandapip1@users.noreply.github.com>
Copy link
Member

@Pandapip1 Pandapip1 left a comment

Choose a reason for hiding this comment

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

Apart from this one minor nit, LGTM

EIPS/eip-5749.md Outdated Show resolved Hide resolved
EIPS/eip-5749.md Outdated Show resolved Hide resolved
kvhnuke and others added 2 commits November 23, 2022 09:05
Co-authored-by: Pandapip1 <45835846+Pandapip1@users.noreply.github.com>
Co-authored-by: Pandapip1 <45835846+Pandapip1@users.noreply.github.com>
@kvhnuke kvhnuke requested a review from Pandapip1 November 23, 2022 20:15
Copy link
Contributor

@kdenhartog kdenhartog left a comment

Choose a reason for hiding this comment

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

How do you expect this to interact with EIP-5593? I'd think we should resolve this before moving this to last call otherwise we may hit incompatibilities that have to be resolved separately. Out of curiosity how many wallets have reviewed this work currently and are supportive of it? Also are you aware that MetaMask is looking to start modifying their ETH provider based on Chain Agnostic Standards Alliance's CAIP-25 so much of this work may become incompatible with the largest wallet in this space?

For me, I don't object to the content in this, but I do to the quick status update without making sure that wallet providers plan to implement this breaking change and that this work doesn't interfere with other efforts to update the provider object.

@kvhnuke
Copy link
Contributor Author

kvhnuke commented Nov 24, 2022

How do you expect this to interact with EIP-5593? I'd think we should resolve this before moving this to last call otherwise we may hit incompatibilities that have to be resolved separately. Out of curiosity how many wallets have reviewed this work currently and are supportive of it? Also are you aware that MetaMask is looking to start modifying their ETH provider based on Chain Agnostic Standards Alliance's CAIP-25 so much of this work may become incompatible with the largest wallet in this space?

For me, I don't object to the content in this, but I do to the quick status update without making sure that wallet providers plan to implement this breaking change and that this work doesn't interfere with other efforts to update the provider object.

I believe discussions regarding the proposal should happen under the discussion link, but to answer your question,
this will not interfere with EIP-5593, it is still upto the extension wallet to decide when to inject and it could be based on user settings.

Whole goal of this EIP is for wallets to open up a new avenue so dapps can detect which wallets are installed by the user, fight
for window.ethereum will continue to happen for the near future as this EIP is not a breaking change. However, this EIP will let wallets to inject also under window.evmproviders={} then dapps can detect that.
we already received support from few major wallets and major web3 onboarding libraries. I know MM team is aware and we are willing to make a PR to the MM team once this is finalized

chain agnostic standards are different topic where we have to redo the whole communication layer, I think we should visit that once that kind of standard turn into a reality

@Pandapip1
Copy link
Member

Pandapip1 commented Nov 24, 2022

I'd say it would be up to EIP-5593 to specify when window.evmproviders is injected. Compatibility with EIP-5593 seems out of the scope of this EIP, but compatibility with this EIP seems to me to be in the scope of EIP-5593.

Copy link
Member

@Pandapip1 Pandapip1 left a comment

Choose a reason for hiding this comment

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

LGTM.

@eth-bot eth-bot enabled auto-merge (squash) November 24, 2022 01:12
@eth-bot eth-bot merged commit 232a490 into ethereum:master Nov 24, 2022
@kvhnuke
Copy link
Contributor Author

kvhnuke commented Nov 24, 2022

LGTM.

Thank you!!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c-status Changes a proposal's status t-interface
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants