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

EIP-1191: Set status Final and remove adoption table #2430

Closed
wants to merge 1 commit into from

Conversation

juli
Copy link
Contributor

@juli juli commented Dec 7, 2019

No description provided.

@eip-automerger
Copy link

Hi! I'm a bot, and I wanted to automerge your PR, but couldn't because of the following issue(s):

  • Trying to change EIP 1191 state from Last Call to Final

@github-actions
Copy link

github-actions bot commented Sep 8, 2020

There has been no activity on this pull request for two months. It will be closed in a week if no further activity occurs. If you would like to move this EIP 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 stale label Sep 8, 2020
@axic
Copy link
Member

axic commented Sep 8, 2020

I still think the comments on #2143 make sense. The reason: once this EIP is marked Final it can no longer be edited, and since the specification prescribes that the hash is calculated based on some adoption list, there is no way for new adoptees to utilise this standard.

@juli
Copy link
Contributor Author

juli commented Sep 11, 2020

The PR removes the adoption table from the document. I can remove the eth_mainnet list from the test cases. The example code has the list of opt-in chain IDs, in practice this is how wallets implemented it and that is why the example code and the test cases are useful this way, Ledger fixed their checksum code for Ethereum while implementing this EIP thanks to this test cases (See here) . EIP-155 also includes a partial list of chain IDs. IMO adding EIP-1191 checksums for chain id 1 to the EIP will only contribute to implementations mistakes.

@github-actions github-actions bot removed the stale label Sep 11, 2020
Copy link
Contributor

@MicahZoltu MicahZoltu left a comment

Choose a reason for hiding this comment

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

The backward compatibility section says:

This proposal is fully backward compatible. The checksum calculation is changed only for new networks that choose to adopt this EIP and add their chain numbers to the Adoption Table included in this document.

But I don't believe this is true at all. If you have an EIP-55 checksumed address, it will fail validation EIP-1191 validation, and if you have a EIP-1191 checksummed address it will fail EIP-55 validation. Also, you mention the adoption table which has since been removed, so the reference here should be removed as well.

I recommend describing in the backward compatibility section that checksums generated/validated by this EIP will not be interoperable with tools that generate/validate EIP-55 checksums and perhaps recommend strategies for tools to deal with this situation.

I believe this was mentioned elsewhere previously, but keep in mind that this is not a consensus change so there is no way to ensure that an entire ecosystem follows this EIP, so in many cases people will need to deal with this incompatibility.


If the chain id passed to the function belongs to a network that opted for using this checksum variant

Since this is not a consensus change, a network cannot opt to use this variant. A given tool can choose to use this variant, or a user can choose to use this variant, but a network as a whole cannot make such a decision because there is not a mechanism for achieving consensus on the issue. Note: Just because all of the tools for a given network happen to follow this doesn't mean that the network has opted to use this variant, it just means that all of the individual tools that happen to be written for that ecosystem today have chosen to use this variant, and that could change at any moment as soon as someone introduces a new tool.

Given that, this part of the specification needs to be reworded to correctly indicate who is making the choice to use this change. Also, a specification really should avoid conditionals where possible, so I would reword it to something like:

When calculating the checksum for an address, prefix the address with the chain ID and the 0x separator before calculating the hash.


prefix the address with the chain id and the 0x separator before calculating the hash.

This isn't specific enough. Is the chain ID and ['0', 'x'] prepended to the address bytes as a number (if so, how is it encoded), or is it prepended as a base 10 string number followed by the string 0x followed by the hexadecimal encoded address (all lowercase or all uppercase?).


The current rationale section contains content more appropriate for the ## Motivation section. The Motivation section is where you provide context on why this EIP is a good idea, while the Rationale section describes why you made some of the choices that you made. An example of something the rationale section should probably contain is "why is the 0x prefix included in the middle of the value" and "why is the hex encoded address hashed instead of hashing a number or a byte array?"


network.For example, if this EIP is implemented, a wallet can alert the user that is trying to send funds to an Ethereum Testnet address from an Ethereum Mainnet wallet.

Need a space between sentences here, and also the EIP should be worded as an assertion, not a conditional. Consider the following (with a couple minor grammatical fixes included):

A wallet that implements this EIP can alert the user that they are trying to send funds to an Ethereum Testnet address from an Ethereum Mainnet wallet.

@github-actions
Copy link

There has been no activity on this pull request for two months. It will be closed in a week if no further activity occurs. If you would like to move this EIP 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 stale label Nov 11, 2020
@github-actions
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 Nov 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants