Changes reference to sha3 to keccak256.

[MicahZoltu]

Ethereum doesn't use sha3 anywhere, and later in the document it explicitly states that this is actually keccak256. To avoid people implementing SHA3 only to find out it doesn't work (like I did), I'm fixing this to be keccak256.

[axic]

I think this makes sense.

[axic]

@vbuterin @alexvandesande any objections?

[alexvansande]

So my understanding is that the sha3 is the standardization of the Keccak algorithm, but with a small implementation differences regarding padding of data, correct? I see no issue at all with the change honestly.

[axic]

Correct.

but with a small implementation differences regarding padding of data

That small difference is enough to result in an entirely different output and hence they are referred to independently for clarity.

[axic]

@MicahZoltu there is one annoying fact here, that piece of code actually works and uses this package https://pypi.org/project/ethereum/. Just checked, and it still only has sha3 and not keccak256 as a function. Perhaps keep the code as is and leave a disclaimer in a comment above it?

[MicahZoltu]

For a reference implementation, I don't think it is appropriate to have code that reads incorrectly but then has a comment indicating what the correct thing to do is. Instead, I would prefer to either:

1. turn the snippet into pseudocode (remove the import and just reference made-up functions with descriptive names)
2. find an alternative dependency library with proper names.
3. swap out the reference implementation with a different language that allows us to write a reference implementation that properly names things (e.g., JavaScript).

[carver]

2. find an alternative dependency library with proper names.

ethereum/eth-utils is a reasonable option for this. Some (working) example code:

import eth_utils
  

def checksum_encode(addr): # Takes a 20-byte binary address as input
    hex_addr = addr.hex()
    checksummed_buffer = ""
     
    # Treat the hex address as ascii/utf-8 for keccak256 hashing
    hashed_address = eth_utils.keccak(text=hex_addr).hex()
    
    # Iterate over each character in the hex address
    for nibble_index, character in enumerate(hex_addr):

        if character in "0123456789":
            # We can't upper-case the decimal digits 
            checksummed_buffer += character
        elif character in "abcdef":
            # Check if the corresponding hex digit (nibble) in the hash is 8 or higher 
            hashed_address_nibble = int(hashed_address[nibble_index], 16)
            if hashed_address_nibble > 7:
                checksummed_buffer += character.upper()
            else:
                checksummed_buffer += character
        else:
            raise eth_utils.ValidationError(
                f"Unrecognized hex character {character!r} at position {nibble_index}"
            )

    return "0x" + checksummed_buffer
 

def test(addr_str):
    addr_bytes = eth_utils.to_bytes(hexstr=addr_str)
    checksum_encoded = checksum_encode(addr_bytes)
    assert checksum_encoded == addr_str, f"{checksum_encoded} != expected {addr_str}"
   

test("0x5aAeb6053F3E94C9b9A09f33669435E7Ef1BeAed")
test("0xfB6916095ca1df60bB79Ce92cE3Ea74c37c5d359")
test("0xdbF03B407c01E7cD3CBea99509d93f8DDDC8C6FB")
test("0xD1220A0cf47c7B9Be7A2E6BA89F429762e7b9aDb")

If you want to add it to your branch @MicahZoltu , I opened a PR against it with this ^ code.

[MicahZoltu]

Updated with changes proposed by @carver

[axic]

Looking at the entire EIP (and not just the diff), I think it would make sense to change it as follows:

1. Specification to start with an explanation in english
2. Followed by some short pseudo-code
3. Moving the python code under the Implementations heading

It may be an unreasonable burden to @MicahZoltu trying to improve this EIP, so I am happy to merge it as is, but the above is what would be my preference.

[MicahZoltu]

I think the changes you suggest @axic are good improvements to this EIP, though I would prefer they appear as a separate PR that gets merged after this one. That being said, if you want to submit a PR to my branch with the proposed changes I'm willing to merge them prior to merging this if that gets this in sooner. 😊 This PR is rapidly growing larger than the original one-word change though...

I believe this EIP was written before we had good standards/practices for EIP writing, so it isn't as of high a quality as some of the newer stuff that is well structured.

[axic]

@alexvansande can you please check again?

[MicahZoltu]

@axic or @holiman Can one of you merge this? I see you both approved it, but it has been sitting for months with no feedback.

[axic]

I'd really prefer the original authors to give some indication, ping @vbuterin @alexvansande ?