-
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
EIP-1191: Extend EIP-55: Add chain id to mixed-case checksum address encoding #1121
Comments
This is an excellent idea, because it prevents using addresses of Ethereum in Ethereum Classic or RSK by mistake and vice-versa. |
EIP-1191 already adopted by Trezor, Ledger, MEW, MyCrypto and web3 |
@juli can you please check this comment: #1191 (comment) ? |
The rationale says:
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. |
This is debated on #2143. |
@axic I don't see that particular topic debated over there. Am I missing something? |
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. |
@MicahZoltu Thanks for your comments:
|
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 |
@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. |
@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. |
The rendering for the "Adoption" table seems to be broken here: https://eips.ethereum.org/EIPS/eip-1191 |
@axic Fixed and a new implementation added. |
Rendering of "Backward Compatibility" is also broken at that link. |
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. |
@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. |
The listIt 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 MutableThe specification is mutable. It requires implementations to constantly check for an updated version of the EIP to know if a specific checksumming is valid:
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 behaviorThe spec states:
It is not clear if chain id shall be lowercase, uppercase, or as provided, if it is provided as a string. Not backwards compatibleGiven 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 wrongThis 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. |
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 |
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. |
@juli Currently in the specification the integer is lowercase.
"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. |
@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. |
|
Proposed new specification
Proposed new implementationI 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)) DiscussionThe 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:
|
|
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
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 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
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:
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. |
|
What do you think about:
Thanks |
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. |
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. |
Please keep personal attacks and claims about motives out of the EIP process. |
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”. |
@fulldecent Having now read the entire thread of this discussion, can you explain to me why you went from |
Good question, just change of sentiment. There is plenty of room to improve this specification. If they want a coauthor, I will help. If you can be bothered to read the implementations (rather than the specification, which is just wrong) it is unambiguous. And also I think RSK network has the prerogative to set their preferred checksumming, which is mostly what this is about. So what I'm trying to say is that the people who wanted to adopt this already have and if this EIP won't get any better then I would rather it live on as a FINAL than a DRAFT. |
I like the idea that helps different EVM compatible chain users to have a better awareness of what chain's address they are willing to send.
Will create some issues on the related tools and see if they are interesting in supporting this EIP |
This does affect current Ethereum ecosystems. For example, if you cut and paste your address from Etherscan, or if you click your account info in MetaMask (to copy the address to the clipboard) you won’t be able to paste it into the ropsten Etherscan site or paste it into MetaMask on a different network. You would need a tool to convert the address every time. It is very common in Ethereum to use the same address across multiple networks, and this would break much of the existing UX around this. |
@ricmoo thanks to point it out. I guess you mean the current checksum address is widely adapted, changing it will cause more issues than the benefits during the transition?
regarding the above comment, does it refers https://github.com/ChainAgnostic/CAIPs/blob/master/CAIPs/caip-10.md#test-cases ? |
@gasolin Exactly! :) The CAIP system isn’t something I’ve followed closely, but it seems to be the front-runner for a backwards-compatible address format which is entangled with chain ID. |
Closing this for housekeeping reasons, but please feel free to continue using this issue as the discussions-to thread for EIP-1191 |
Hi from 2022, I like the idea of this EIP. One thing I love to bring up is that I happen to come across this particular line of code in geth which probably gets used widely. I wonder if we start adopting EIP-1191 would it break some software without a properly scheduled compatibility mitigation? |
@xinbenlv I’ve actually fought quite hard against that EIP. You should be able to see my comments on the EIP discussion. :) Support for that EIP has been marked in the documentation of Web3.js and EthereumVM has been identified as unsafe and I believe they plan to (or may have already?) removed support for it, as it is dangerous. This change is not backwards compatible with the Ethereum ecosystem, and would cause so much to break. For example, it means a checksum address copied from Etherscan for mainnet would fail when pasting it into MetaMask currently attached to Görli. There are new address formats available (E.g. CAIP) that could have this change made without affecting the ecosystem in a way that would cause everything to break, but to modify a widespread standard in a way that changes the meaning and value of the checksum would collapse everything. Another big problem with this idea in general. Is that many places when an address is needed, do not have access to the chain ID, so any checksum comparisons would be unable to be performed. For example, if a contract isn’t connected to a network, and you just wanted to use With ENS names, which I would prefer see get better cross-chain support (coming in v6) much of this is moot anyways, since we can just ask for that chain ID’s address, so it can be entirely different addresses, if need be. :) |
(apologies, I get a lot of emails per day from ether’s repo and thought that this was an issue opened against ethers, I am just now realizing that it was posted on the discussions-to issue. Sorry for the noise.) |
Hi @ricmoo , yes you are right. My comment was meant to bring up question / doubt about this EIP-1191, despite I can see merit about it. I came across this EIP partly because (1) I am working on #2294 which needs some research on what could cause a problem if boundary of In summary, I am not advocating for it, I am just concerning if it's still a last call. You raise compatibility issue from It's a weird state where an EIP is being adopted by some clients, but rejected by some other clients. I wonder what state the EIP should be, |
How about we use a different prefix for EIP-1191 addresses? Instead of |
This EIP will not break MetaMask and it will not stop you from copy pasting a Mainnet address to testnet. MetaMask adopts things very slowly and this behavior is opt-in. If a wallet, such as certain popular wallets, wanted to adopt EIP-1191 then if you tried to copy paste a Mainnet address it would probably give you a warning:
Even today, Etherscan still does not fully support EIP-55. But either way if nobody is driving this then it doesn't matter because it wont go forward. |
@fulldecent how does this look? |
|
@Pandapip1 I can see merit of EIP-1191 to avoid error for executing on wrong chains, and I also worry about backward compatibility. I wonder if a middle-ground we can leverage EIP-1191 but avoid backward incompatibility Would it be acceptable if we only use 1191 in the context of CAIP-10 or other chain-aware format (e.g. https://eips.ethereum.org/EIPS/eip-3770)? |
I'm well aware that |
I'm not sure where you are getting this idea from, but it definitely isn't the case. Addresses have always been presented as |
Well... there was also the IBAN format. I'm proposing that there be a new address format to intentionally break compatibility. Also, perhaps, instead of |
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 ofabcdef
) 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
Test Cases
Adoption
Adoption Table
Implementation Table
Copyright
Copyright and related rights waived via CC0.
The text was updated successfully, but these errors were encountered: