ethereum / EIPs

The Ethereum Improvement Proposal repository
https://eips.ethereum.org/
Creative Commons Zero v1.0 Universal
12.9k stars 5.29k forks source link

EIP-1191: Extend EIP-55: Add chain id to mixed-case checksum address encoding #1121

Closed juli closed 2 years ago

juli commented 6 years ago
eip: 1191 
title: Add chain id to mixed-case checksum address encoding 
author: Juliano Rizzo (@juli)
status: Last Call
review-period-end: 2019-11-18
type: Standards Track
category: ERC
created: 2018-03-18
requires: 55, 155
discussions-to: https://github.com/ethereum/EIPs/issues/1121

Simple Summary

This EIP extends EIP-55 by optionally adding a chain id defined by EIP-155 to the checksum calculation.

Abstract

The EIP-55 was created to prevent users from losing funds by sending them to invalid addresses. This EIP extends EIP-55 to protect users from losing funds by sending them to addresses that are valid but that where obtained from a client of another 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.

Motivation

The motivation of this proposal is to provide a mechanism to allow software to distinguish addresses from different Ethereum based networks. This proposal is necessary because Ethereum addresses are hashes of public keys and do not include any metadata. By extending the EIP-55 checksum algorithm it is possible to achieve this objective.

Specification

Convert the address using the same algorithm defined by EIP-55 but if a registered chain id is provided, add it to the input of the hash function. If the chain id passed to the function belongs to a network that opted for using this checksum variant, prefix the address with the chain id and the 0x separator before calculating the hash. Then convert the address to hexadecimal, but if the ith digit is a letter (ie. it's one of abcdef) print it in uppercase if the 4*ith bit of the calculated hash is 1 otherwise print it in lowercase.

Rationale

Benefits:

Backwards Compatibility

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.

Implementation

#!/usr/bin/python3
from sha3 import keccak_256
import random
"""
   addr (str): Hexadecimal address, 40 characters long with 2 characters prefix
   chainid (int): chain id from EIP-155 """
def eth_checksum_encode(addr, chainid=1):
    adopted_eip1191 = [30, 31]
    hash_input = str(chainid) + addr.lower() if chainid in adopted_eip1191 else addr[2:].lower()
    hash_output = keccak_256(hash_input.encode('utf8')).hexdigest()
    aggregate = zip(addr[2:].lower(),hash_output)
    out = addr[:2] + ''.join([c.upper() if int(a,16) >= 8 else c for c,a in aggregate])
    return out

Test Cases

eth_mainnet = [
"0x27b1fdb04752bbc536007a920d24acb045561c26",
"0x3599689E6292b81B2d85451025146515070129Bb",
"0x42712D45473476b98452f434e72461577D686318",
"0x52908400098527886E0F7030069857D2E4169EE7",
"0x5aAeb6053F3E94C9b9A09f33669435E7Ef1BeAed",
"0x6549f4939460DE12611948b3f82b88C3C8975323",
"0x66f9664f97F2b50F62D13eA064982f936dE76657",
"0x8617E340B3D01FA5F11F306F4090FD50E238070D",
"0x88021160C5C792225E4E5452585947470010289D",
"0xD1220A0cf47c7B9Be7A2E6BA89F429762e7b9aDb",
"0xdbF03B407c01E7cD3CBea99509d93f8DDDC8C6FB",
"0xde709f2102306220921060314715629080e2fb77",
"0xfB6916095ca1df60bB79Ce92cE3Ea74c37c5d359",
]
rsk_mainnet = [
"0x27b1FdB04752BBc536007A920D24ACB045561c26",
"0x3599689E6292B81B2D85451025146515070129Bb",
"0x42712D45473476B98452f434E72461577d686318",
"0x52908400098527886E0F7030069857D2E4169ee7",
"0x5aaEB6053f3e94c9b9a09f33669435E7ef1bEAeD",
"0x6549F4939460DE12611948B3F82B88C3C8975323",
"0x66F9664f97f2B50F62d13EA064982F936de76657",
"0x8617E340b3D01Fa5f11f306f4090fd50E238070D",
"0x88021160c5C792225E4E5452585947470010289d",
"0xD1220A0Cf47c7B9BE7a2e6ba89F429762E7B9adB",
"0xDBF03B407c01E7CD3cBea99509D93F8Dddc8C6FB",
"0xDe709F2102306220921060314715629080e2FB77",
"0xFb6916095cA1Df60bb79ce92cE3EA74c37c5d359",
]
rsk_testnet = [
"0x27B1FdB04752BbC536007a920D24acB045561C26",
"0x3599689e6292b81b2D85451025146515070129Bb",
"0x42712D45473476B98452F434E72461577D686318",
"0x52908400098527886E0F7030069857D2e4169EE7",
"0x5aAeb6053F3e94c9b9A09F33669435E7EF1BEaEd",
"0x6549f4939460dE12611948b3f82b88C3c8975323",
"0x66f9664F97F2b50f62d13eA064982F936DE76657",
"0x8617e340b3D01fa5F11f306F4090Fd50e238070d",
"0x88021160c5C792225E4E5452585947470010289d",
"0xd1220a0CF47c7B9Be7A2E6Ba89f429762E7b9adB",
"0xdbF03B407C01E7cd3cbEa99509D93f8dDDc8C6fB",
"0xDE709F2102306220921060314715629080e2Fb77",
"0xFb6916095CA1dF60bb79CE92ce3Ea74C37c5D359",
]
test_cases = {30 : rsk_mainnet, 31 : rsk_testnet, 1 : eth_mainnet}

for chainid, cases in test_cases.items():
    for addr in cases:
        assert ( addr == eth_checksum_encode(addr,chainid) )

Adoption

Adoption Table

Network Chain id Supports this EIP
RSK Mainnet 30 Yes
RSK Testnet 31 Yes

Implementation Table

Project Adopted this EIP Implementation
MyCrypto Yes JavaScript
MyEtherWallet Yes JavaScript
Ledger Yes C
Trezor Yes Python and C
Web3.js Yes JavaScript
EthereumJS-util Yes JavaScript
ENS address-encoder Yes TypeScript

Copyright

Copyright and related rights waived via CC0.

SergioDemianLerner commented 6 years ago

This is an excellent idea, because it prevents using addresses of Ethereum in Ethereum Classic or RSK by mistake and vice-versa.

juli commented 5 years ago

EIP-1191 already adopted by Trezor, Ledger, MEW, MyCrypto and web3

axic commented 5 years ago

@juli can you please check this comment: https://github.com/ethereum/EIPs/pull/1191#issuecomment-495398739 ?

MicahZoltu commented 5 years ago

The rationale says:

By means of a minimal code change on existing libraries, users are protected from losing funds by mixing addresses of different Ethereum based networks.

Is the idea here that a user will receive a contract address from some source that uses this new checksum, and then when interacting with that contract address their tooling will tell them the address is invalid if they choose a different network during signing?

I'm assuming that the intention is that EOA addresses would continue to use the EIP-55 checksum, since EOA addresses do work across chains? Presumably, the same for contracts that deploy to a standardized address across chains (like EIP 1820)? If so, I think these things should be mentioned in the EIP to prevent implementers from going overboard and using this new checksum everywhere (it is not appropriate everywhere).


Why do the test case arrays have different addresses for each network? It feels like the same set of addresses should be encoded for each network in the tests.

axic commented 5 years ago

Why do the test case arrays have different addresses for each network? It feels like the same set of addresses should be encoded for each network in the tests.

This is debated on #2143.

MicahZoltu commented 5 years ago

@axic I don't see that particular topic debated over there. Am I missing something?

MicahZoltu commented 5 years ago

I do agree with the arguments made in #2143 that "Ethereum" doesn't adopt this EIP, instead wallets adopt this EIP. A wallet checking a checksum can check both the EIP-55 checksum and the EIP-1191 checksum and if either match green light the transaction. When generating a checksum, it would be up to the wallet whether to generate an EIP-1191 checksum or an EIP-55 checksum for the user. Getting adoption on generation is a bit harder, since it means creating addresses that are incompatible with wallets that do checksum validation but don't support EIP-1191.

juli commented 5 years ago

@MicahZoltu Thanks for your comments:

  1. It should be used for EAO addresses. For example, if you send ETC or RSK funds to your Ethereum address and you use a hardware wallet, the wallet can prevent you from signing transactions cross chain (derivation path checks) and you'll be forced to load the same seed in a software wallet to recover the funds. In some cases accessing the private key won't be possible for end users or will be too risky for organizations.
  1. Networks decide and specify what address format to use, not wallets alone. Ideally, Copy&paste of addresses should work across exchanges, explorers, and wallets without requiring users to do a conversion. In the case of EIP-155, wallets can decide to protect users or not by adding the chain id to the signature but networks decide to implement EIP-155 in nodes and whether nodes allow signatures without chain id or not.

  2. If a wallet knows about EIP-55 and EIP-1191 then it should at least warn the user when a EIP-1191 RSK address is being used in a ETH transaction instead of green lighting it.

  3. Yes, adoption on generation before other wallets and exchanges implement EIP-1191 could be a problem and force users to convert the addresses for a while but wallets can decide to protect their own users by generating EIP-1191 addresses for networks that adopted it, we can consider it to be "secure by default" logic.

  4. The first item of each test case list is an all-uppercase valid checksum addresses for each chain id and that is why it is different for each chain id. The order in each list is different due to the upper/lower changes. RSK testnet has an extra test case. I can make changes to the test cases if you think it is important.

axic commented 5 years ago

The "network" does not care, i.e. the RPC interface ignores the checksum. And the RPC interface is the closest you can get to the network, anything below that just deals with bytes and not hex strings. Checksums are only enforced by wallets.

This was the case for years, correct me if I'm wrong and the JSON RPC in sendTransaction and others enforce checksum now.

MicahZoltu commented 5 years ago

@axic I'm confident that checksums are not required. It is possible the client will error on invalid checksum in a JSON-RPC request (I have never tried).

@juli Some users intentionally use the same address across multiple chains as that allows them to only have to secure a single private key. It certainly is unfortunate that hardware wallets chain lock like you have described. The reason they do it is to protect users from a hardware wallet app attack, where some malicious app gets onto the hardware wallet and leaks a private key, it is restricted to what derivation paths it can leak keys for

Given that, I would suggest that EOA addresses be optionally checksumed in this way, to cater to both usage scenarios. It would be up to the wallet which solution was appropriate for a given address.

Nitpick: I would like to see the test cases sorted the same for each network and the same set of addresses included for each network. It may also be valuable to show the same set of addresses encoded with EIP-55 checksum only, mainly for illustration purposes.

juli commented 5 years ago

@axic I know the checksum is for user facing applications, I know how Ethereum nodes work. But what I tried to say is that deciding address format is something that should be part of the standards of a cryptocurrency system and not a decision of individual wallets. In the same way a chain id and BIP44/SLIP-0044 ID is decided.

@MicahZoltu Those users are doing it wrong, reusing the same private key for transactions in different networks is risky. They can use the same seed without reusing keys, that is why HD wallets were invented. This EIP makes sense if it is used for EAO and contract addresses.

Nitpick: done #2345

The addresses encoded with EIP-55 only are in the eth_mainnet list, EIP-1191 is backward compatible.

axic commented 5 years ago

The rendering for the "Adoption" table seems to be broken here: https://eips.ethereum.org/EIPS/eip-1191

juli commented 5 years ago

@axic Fixed and a new implementation added.

MicahZoltu commented 5 years ago

Rendering of "Backward Compatibility" is also broken at that link.

MicahZoltu commented 5 years ago

FWIW, I don't agree with the EIP as currently worded for the reasons I mentioned above. I think that checksumming like this should NOT be applied to all addresses, but be opt-in per address (a decision a wallet can choose to make). I also agree with other commenters that the decision to support this is a wallet decision, not a chain decision, just like EIP-55. Because of this, I don't think that the Adoption Table should be included.

That being said, I see nothing technically wrong with this EIP, thus the above concerns are not a blocker for moving this EIP to final should the authors disagree with my assessment.

juli commented 4 years ago

@MicahZoltu thanks for your comments. EIP-55 nor EIP-1191 can solve the initial mistake: using hexadecimal addresses. At least one very popular wallet fixed their EIP-55 implementation while adding EIP-1191. The EIP-55 bug of that wallet had not been reported before, that probably means most users don't pay attention to the upper/lower case differences. My main motivation to work on this EIP to make it final is that it has been already implemented by wallets and libraries and having a peer reviewed EIP can contribute to adoption by other wallets and exchanges and allow users to copy&paste without doing conversions while reducing the probability of mistakes. I don't think this EIP is enough to solve the problem with hexadecimal addresses but it is at least an attempt. Using a better address format is the real solution. If you think the current version is good enough I'll move to work on something more useful than this for Ethereum users. Thank you.

fulldecent commented 4 years ago

The list

It is not clear to me how enumerating specific networks in the EIP is part of the specification.

If a network is NOT listed in the EIP then can they use the checksums?

Listing networks adds a cost to maintaining this document.

Recommendation: remove this part

Mutable

The specification is mutable. It requires implementations to constantly check for an updated version of the EIP to know if a specific checksumming is valid:

... If the chain id passed to the function belongs to a network that opted for using this checksum variant, prefix the address with the chain id and the 0x separator before calculating the hash

It is not specified how to know if "a network that opted for using this checksum variant". Presumably this is indicated by including the network on the list.

The reference implementation does not load https://eips.ethereum.org/EIPS/eip-1191 to check the list and so it does not properly implement the specification.

Spec undefined behavior

The spec states:

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

It is not clear if chain id shall be lowercase, uppercase, or as provided, if it is provided as a string.

Not backwards compatible

Given the above, I believe it is not correct to say this is backwards compatible. It is fine to be not backwards compatible, but it should be clearly said that the new EIP replaces the old EIP for certain cases. But EIP-1191 will not implement and extend EIP-155

Limited utility / Will's explanation of why everyone is using CHAIN ID wrong

This item is a comment. It is meant to open discussion. This criticism is not a technical reason to reject this EIP.

I believe this EIP has limited or no utility because chain IDs are being used wrong currently and they should be changing more frequently in the future. There is a full discussion on this here https://ethereum-magicians.org/t/eip-1344-add-chain-id-opcode/1131/13

Background: nowadays it is popular when you are installing software to just give your root password to the software and let it install itself, or if you are setting up a brokerage account you give the brokerage your bank account password to login as you and do whatever it wants with your account. Similarly, when you sign a transaction on Ethereum, you permit that transaction to execute today on the current version of Ethereum or any future version of the consensus client that might be created in the future.

Details: The attack vector is: [SIGN+SUBMIT TRANSACTION] -> [ALL CLIENTS UPGRADE] -> [TRANSACTION EXECUTES]. Here the transaction you signed is not the one that executes. I consider this unacceptable. This is a design weakness and the solution is that every change to the consensus client or state hardfork must result in the chain id changing.

This affects the current EIP because if chain ids are changing frequently then the address checksumming is changing. It that would be a bad customer experience.

juli commented 4 years ago

Chain ids are integers so they can't be lowercase. Everything in Ethereum, including the EVM is vaguely defined I can't fix that. I agree identifying genesis + network upgrade/hard forks makes more sense than a fixed short id but the chain id was the only id available when I looked at this problem.

About mutability: keeping a JSON document for wallets sounds like a potential solution.

Wallets and libraries already have their own static list of networks with their BIP44 values. Example: MyCrypto Ledger

SergioDemianLerner commented 4 years ago

We should always put users first. This EIP improves the user experience and protects them from costly mistakes. I think is our duty to finalize this EIP, which has been de-facto standardized by wallets and is technically sound. In following EIPs the community can create new address formats and protect from more exotic problems regarding network forks.

fulldecent commented 4 years ago

@juli Currently in the specification the integer is lowercase.

"add it to the input of the hash function."

"Add" is underspecified.

If the specification is that a JSON document is required and there is a reference document published in the EIP then the reference implementation must include an HTTP client to retrieve this JSON file.

fulldecent commented 4 years ago

@SergioDemianLerner This EIP may be technically sound after it addresses the technical issues I have raised above.

To be clear, and as noted above, my complaint about chain ID in general is not a technical complaint against this EIP. But it is relevant for future readers and that's why I put it here.

juli commented 4 years ago

@fulldecent

fulldecent commented 4 years ago

Proposed new specification

An address (string of 40 hex digits) and an EIP-155 chain ID (string of digits with no loading zeros) are required to calculate an EIP-1191 checksummed address.

First calculate a reference string. This is the chain ID with the lowercased address concatenated after, the result sha3 hashed into hexadecimal format. The EIP-1191 checksummed address shall be equal to the original address with each hexdigit A through F at location i in the address shown in uppercase iff the hexdigit i in the reference string is greater than or equal to 8. Typically the checksummed address is shown with "0x" prefixed.

Proposed new implementation

I could not get the existing implementation to run (library missing). And then assertion failures.

Attached is an update including modern import, PEP8 styling, docstring documentation, and Python 3 static typing.

The output is wrong. Or the test case is wrong. Please help.

#!/usr/bin/python3
import hashlib

def eip1191_checksum(ethereum_address: str, eip155_chainid: str) -> str:
    """
    @param address: an Ethereum address, 40 hex digits
    @param eip_155_chainid: digits with no loading zeros
    """
    assert(len(ethereum_address) == 40)
    assert(int(ethereum_address, 16) > 0)
    assert(int(eip155_chainid, 10) > 0)
    assert(str(int(eip155_chainid, 10)) == eip155_chainid)
    hash_input = eip155_chainid + ethereum_address.lower()
    reference_string = hashlib.sha3_256(hash_input.encode('utf8')).hexdigest()
    aggregate = zip(ethereum_address.lower(), reference_string)
    return "0x" + "".join([c if a < "8" else c.upper() for c, a in aggregate])

eth_mainnet = [
    "0x27b1fdb04752bbc536007a920d24acb045561c26",
    "0x3599689E6292b81B2d85451025146515070129Bb",
    "0x42712D45473476b98452f434e72461577D686318",
    "0x52908400098527886E0F7030069857D2E4169EE7",
    "0x5aAeb6053F3E94C9b9A09f33669435E7Ef1BeAed",
    "0x6549f4939460DE12611948b3f82b88C3C8975323",
    "0x66f9664f97F2b50F62D13eA064982f936dE76657",
    "0x8617E340B3D01FA5F11F306F4090FD50E238070D",
    "0x88021160C5C792225E4E5452585947470010289D",
    "0xD1220A0cf47c7B9Be7A2E6BA89F429762e7b9aDb",
    "0xdbF03B407c01E7cD3CBea99509d93f8DDDC8C6FB",
    "0xde709f2102306220921060314715629080e2fb77",
    "0xfB6916095ca1df60bB79Ce92cE3Ea74c37c5d359",
]
rsk_mainnet = [
    "0x27b1FdB04752BBc536007A920D24ACB045561c26",
    "0x3599689E6292B81B2D85451025146515070129Bb",
    "0x42712D45473476B98452f434E72461577d686318",
    "0x52908400098527886E0F7030069857D2E4169ee7",
    "0x5aaEB6053f3e94c9b9a09f33669435E7ef1bEAeD",
    "0x6549F4939460DE12611948B3F82B88C3C8975323",
    "0x66F9664f97f2B50F62d13EA064982F936de76657",
    "0x8617E340b3D01Fa5f11f306f4090fd50E238070D",
    "0x88021160c5C792225E4E5452585947470010289d",
    "0xD1220A0Cf47c7B9BE7a2e6ba89F429762E7B9adB",
    "0xDBF03B407c01E7CD3cBea99509D93F8Dddc8C6FB",
    "0xDe709F2102306220921060314715629080e2FB77",
    "0xFb6916095cA1Df60bb79ce92cE3EA74c37c5d359",
]
rsk_testnet = [
    "0x27B1FdB04752BbC536007a920D24acB045561C26",
    "0x3599689e6292b81b2D85451025146515070129Bb",
    "0x42712D45473476B98452F434E72461577D686318",
    "0x52908400098527886E0F7030069857D2e4169EE7",
    "0x5aAeb6053F3e94c9b9A09F33669435E7EF1BEaEd",
    "0x6549f4939460dE12611948b3f82b88C3c8975323",
    "0x66f9664F97F2b50f62d13eA064982F936DE76657",
    "0x8617e340b3D01fa5F11f306F4090Fd50e238070d",
    "0x88021160c5C792225E4E5452585947470010289d",
    "0xd1220a0CF47c7B9Be7A2E6Ba89f429762E7b9adB",
    "0xdbF03B407C01E7cd3cbEa99509D93f8dDDc8C6fB",
    "0xDE709F2102306220921060314715629080e2Fb77",
    "0xFb6916095CA1dF60bb79CE92ce3Ea74C37c5D359",
]
test_cases = {"30": rsk_mainnet, "31": rsk_testnet, "1": eth_mainnet}

for eip155_chainid, cases in test_cases.items():
    for address in cases:
        assert (address == eip1191_checksum(address[2:], eip155_chainid))

Discussion

The list in EIP-155 is provided for informational purposes only. There is no effect on the interpretation of EIP-155 based on the entries in the table list. That is distinguishable from EIP-1191 (DRAFT) which currently requires that output be different based on whether a chain ID "belongs to a network that opted...". Networks are not human and they do not opt. That specification is undefined behavior.

The solution is: add this paragraph to the specification:

Clients should use EIP-1191 addresses for networks which are known to support EIP-1191, see Known Supported Networks, below. For other networks, clients should continue to use EIP-155 addresses.

juli commented 4 years ago

@fulldecent

fulldecent commented 4 years ago

Please read about the differences between SHA3 and Keccak 256 here.

Please help to correct my implementation. I'm just here to point out problems.

Python 3 code from current version of EIP-1191 works for me.

Here is how I executed the current version of EIP-1191 (DRAFT). It does not compile.

python3 <<EOL
#!/usr/bin/python3
from sha3 import keccak_256
import random
"""
   addr (str): Hexadecimal address, 40 characters long with 2 characters prefix
   chainid (int): chain id from EIP-155 """
def eth_checksum_encode(addr, chainid=1):
    adopted_eip1191 = [30, 31]
    hash_input = str(chainid) + addr.lower() if chainid in adopted_eip1191 else addr[2:].lower()
    hash_output = keccak_256(hash_input.encode('utf8')).hexdigest()
    aggregate = zip(addr[2:].lower(),hash_output)
    out = addr[:2] + ''.join([c.upper() if int(a,16) >= 8 else c for c,a in aggregate])
    return out
EOL

Result

Traceback (most recent call last): File "", line 2, in ModuleNotFoundError: No module named 'sha3'

Your code has the following problems:

it does not include '0x' between the chain id and the address

Yes, this is intentional. EIP-55 does not use "0x" in the hash. Source https://eips.ethereum.org/EIPS/eip-55. Currently, EIP-1191 (DRAFT) also does not use "0x", source "if a registered chain id is provided, add it to the input of the [EIP-55] hash function." So my implementation does not use "0x".

it uses EIP-1191 for the eth_mainnet test cases

Perhaps the mainnet test cases should be removed.

Or, if a more complete test case is desired then we can add a function checksum_address() which decides whether to use EIP-1191 (DRAFT) or EIP-55.

Who opts to implement an EIP?

"Opt" is not used in my revised specification or my revised DRAFT implementation. I do not understand your question. But in general, anybody opts an EIP. This is specified at https://github.com/ethereum/EIPs

Ethereum Improvement Proposals (EIPs) describe standards for the Ethereum platform, including core protocol specifications, client APIs, and contract standards.

This must be read implicitly. Because this project scope statement says nothing of who opts to use EIPs, it is understood that anybody, or nobody can use an EIP.

IMO the list of chain ids in the EIP-1191 implementation is more clear than adding the text you suggest.

Agreed. Here is an update to be more clear:

An address (string of 40 hex digits) and an EIP-155 chain ID (string of digits with no loading zeros) are required to calculate an EIP-1191 checksummed address.

First calculate a reference string. This is the hexadecimal SHA-3 hash of the concatenation of the address and chain ID. The EIP-1191 checksummed address shall be equal to the original address with each hexdigit A through F at location i in the address shown in uppercase iff the hexdigit i in the reference string is greater than or equal to 8. Typically the checksummed address is shown prefixed with "0x".

Clients should use addresses in EIP-1191 format for networks listed in the Adoption Table below. For other networks, clients should use addresses in EIP-55 format.


Please note that another benefit of doing it this way ^^ is that it allows mainnet to upgrade to EIP-1191 (DRAFT) in the future. Clients will recognize EIP-1191 (DRAFT) addresses and EIP-55 addresses. There will probably even be a common emoji or color that clients show to differentiate chain-specific (1191) and chain-general (55) addresses. Color and emoji are out of the scope of this proposal. And supporting this use case is NOT a motivation for the above changes.

juli commented 4 years ago

@fulldecent

juli commented 4 years ago

replying to @axic comment

It makes a lot more sense to specify how to calculate the checksum and leave the adoption outside the scope of this EIP.

@alcuadrado @MicahZoltu

What do you think about:

  1. replacing the adoption table by a link to the ethereum-lists repository.
  2. Adding a JSON field such as eip55 or eip1191 or checksumType to the JSON documents where the chain id and name are specified?
    1. If you agree, what would be the preferred JSON variable name and where should it be specified?

Thanks

MicahZoltu commented 4 years ago

What do you mean "add a JSON field"?

My argument is still the same as before, checksum support isn't something that is decided "per chain", it is something that is decided "per tool". If tools think 1191 is useful, they will implement it. If they think 55 is useful, they'll implement it, if they don't think either is useful, they'll implement neither.

Some tools may defer the choice to users.

Some chain dev teams will strongly encourage the use of checksums to tool authors.

However, there isn't a centralized authority that is able to declare that all Ethereum tools MUST use EIP1191, or even SHOULD. At best, you can write an EIP that says all Ethereum chains MUST and then people can choose to follow that EIP or not. Keep in mind, there are many EIPs that are unfollowed.

juli commented 4 years ago

I mean a JSON attribute. Wallets need chain ids and BIP44 paths and that is why BIP44/SLIP-0044 and ethereum-lists were created, looks like an appropriate place to inform the preferred checksum or address format for a network.

fulldecent commented 4 years ago

Recommend to please demote this EIP to draft status. We have a dozen other EIPs in last review and which are implemented and ready to go live on mainnet and are scheduled to hard fork in a week or so.

This EIP needs some updates, is not ready, and is better suited to go to draft status.

MicahZoltu commented 4 years ago

I somewhat disagree with @fulldecent here (though, maybe just semantics). If you (the author) think this EIP is final, you should change it to final as the review period is long over. If you think this EIP needs more work, then you should, as @fulldecent suggests, and move it back to draft and continue iterating on it.

axic commented 4 years ago

I wonder what constitutes as final, given a question was brought up by at least 3 independent people and it seems to be without any resolution/agreement.

MicahZoltu commented 4 years ago

@axic Final is whatever the author wants. EIPs do not require consensus to become final, though core EIPs do require consensus to become deployed to mainnet.

fulldecent commented 4 years ago

Here is the original precedent for promoting an EIP from Last Call to Final:

https://github.com/ethereum/EIPs/pull/1170

The official process for promotion is in EIP-1 it is:

A successful Last Call without material changes or unaddressed technical complaints will become Final.

This EIP does not meet that definition which is why it should be moved to Draft. And quickly. Because there are other EIPs in Last Call which are very active and which are deploying to mainnet imminently.

MicahZoltu commented 4 years ago

Hmm, good point about the technical issues. While I don't agree that all of the issues you brought up above are "technical", I do agree that some of them are (like the fact the Python sample doesn't compile). I would be satisfied with just a comment indicating that the sample code is pseudocode, rather than claiming it was Python if that resolves that particular issue.

fulldecent commented 4 years ago

The specification is not implementable. This is a technical deficiency. One deficiency makes the EIP disqualified to promote to Final.

These has been an extended period of time where this initiative has arrested progress. So it should be moved to Draft. Then after it has been improved (I have proposed fixes) it can come to Last Call again. At that point it is my responsibility (as a reviewer) to review all past comments and see if there is any remaining deficiency. Then I should argue to show if any deficiency makes the new version disqualified. In the meantime, other things (not disqualifiers) are found and the EIP is improved before getting to Final.

If rsk was my client or I was coauthor here then I will be more involved in updating the EIP and getting it done. But for now I have other work so my interest here is just to ensure only qualified EIPs of the highest quality are promoted to Final.

3esmit commented 4 years ago

Some required changes:

Please Author, do those changes (or equivalent), otherwise we would have to fork this EIP to a new one to solve the problem.

3esmit commented 4 years ago

Is possible to detect what is the checksum mode being used? I.e. function that when entering the checksumed address, it returns what chainid was used on it

juli commented 4 years ago

@3esmit yes, wallets and other front-end systems (e.g., explorers) displaying a "warning this address has a valid checksum for Ethereum but you are using it in a wallet for RSK" is possible. But unfortunatelly in some cases the checksum will be valid for different networks. Unfortunally the big mistake was the hexadecimal address format, the checksum was a good attempt but to patch it but it is insufficient.

fulldecent commented 3 years ago

This should be demoted to Draft status.

It entered Last Call. I thoroughly demonstrated that the implementation or the spec was wrong and/or not implementable. https://github.com/ethereum/EIPs/issues/1121#issuecomment-552121956 Then the EIP was either updated or it wasn't. Either way, it should be booted from Last Call and go back to draft.

When it enters Last Call again I can review again and hopefully it is in better shape.

MicahZoltu commented 3 years ago

@juli Do you plan on continuing to pursue this EIP? If not, we can move it to Withdrawn. If we hear nothing, we will probably move it to Stagnant.

Ultimately, the EIP author is the one that decides when an EIP is finished. I do encourage you to heed the feedback of people who have reviewed the EIP, but you aren't required to agree with them on everything.

marianasoff commented 3 years ago

Hello everybody, nice to meet you. I made a few changes to @Juli 's project here

I am trying to help it being accepted because I found it really usefull and hope you will too. Please let me know if the changes are enough or ask if you need a specific change for it to be aproved.

Thank you!

ricmoo commented 3 years ago

I just became aware of this EIP.

I consider it to have massively unwanted consequences and am seeking out some advice from others I trust in the field, before I sound the alarm too far.

As I understand it;

My two cents. I find it highly unlikely (unless there is wide community adoption, which from the sentiment I’m reading here, I doubt) that I would add this to ethers, unless I am missing something?

juli commented 3 years ago

@ricmoo What has massively unwanted consequences is the Ethereum address format. This EIP is a small improvement already implemented and has been working for years in many wallets as you can check at the bottom of the document. Unfortunatly invalid checksums should be a warning instead of an error because many very popular UIs (e.g., exchanges) do not show checksumed Ethereum addresses to users that then copy and paste them.

The community has implemented it in many wallets years ago including hardware wallet firmware. But then there are Github commenters with too much free time making noise in this thread, totally irrelevant compaed to working code.

ricmoo commented 3 years ago

I came to this EIP as there was an issue in ethers where a person passed in an EIP-1191 to ethers, which has not ever supported it, which resulted in an error.

Since this error hasn't ever come up before (otherwise I'd have been made aware of the EIP earlier), I would say its quite likely this format is not in wide usage. Keep in mind that Web3.js uses ethers ABI coder (which checks all addresses using EIP-55), so the fact this issue isn't common means that it is unused by anyone who uses contracts in Web3.js too.

"A checksum is a small-sized block of data derived from another block of digital data for the purpose of detecting errors...". (main point I'm trying to make is that a checksum indicates an error).

An address without mixed-case is not a checksum address, so if an exchange shows a non-checksum address, things work as usual; no checksum is checked. But if the case is mixed, and the checksum does not match, it is a hard error.

I also reached out to Web3.js and EthereumJS yesterday, and both agreed this EIP was dangerous.

At the very least the EIP should have a backwards compatibility section added (as per the current EIP-1) that this format "completely breaks backwards compatibility with any software which validates EIP-55 checksums" and this EIP should be moved to a status of Draft as mentioned above.

At the time of EIP-55, case had no meaning, so retconning the format by adding meaning did not break backwards compatibility (some things failed for other reasons; but that was a failure of assumptions). Changing the meaning of the case does have consequences, and breaking backwards compatibility is serious and requires rather convincing arguments. There are new competing attempts to solve this problem which do not break backwards compatibility, such as CAIP.

juli commented 3 years ago

This EIP is not dangerous, people keep loosing money sending mainnet ETH to testnet addresses for example, that is dangerous. What happened with @gudahtt is not a problem of the EIP, he was passing checksumed addresses to a library and that is not correct nor necesssary, he realised that himself later.

Checksums are only for UIs, to show checksumed addresses to users and to validate input from users. Internally addresses must be normalized (e.g., lower case it) before passing it to libraries.

Please go and write a new EIP if you have a better solution for the problem. This as good as possible, the big mistake was using hexadecimal addresses for Ethereum when better formats were already available.

fulldecent commented 3 years ago

I fully support this EIP for going to FINAL status as-is.

It is well-implemented, well-supported, well-specified.

If there are places that don't support this, I'm sure they will welcome a PR.

marianasoff commented 3 years ago

My humble opinion is that sadly there are people in this community that do not want something things to improve or evolve due to personal and selfish reason. It is very easy to critisize instead of trying to help promote minor useful new changes. Participants such as @ricmoo can seriously harm and stall the evolution of this ecosistem.

MicahZoltu commented 3 years ago

Please keep personal attacks and claims about motives out of the EIP process.

ricmoo commented 3 years ago

I fully support this EIP for going to FINAL status as-is.

This cannot be placed in Final without a backwards compatibility note. It (objectively) breaks backwards compatibility.

I believe that concern should qualify as an “unaddressed technical complaints”.