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

Web3.isAddress doesn't work for non prefixed checksumed values #715

Closed
Tracked by #2854
Uxio0 opened this issue Mar 21, 2018 · 5 comments · Fixed by #3248
Closed
Tracked by #2854

Web3.isAddress doesn't work for non prefixed checksumed values #715

Uxio0 opened this issue Mar 21, 2018 · 5 comments · Fixed by #3248
Assignees
Labels
priority: p3 normal v7 breaking changes considered for v7

Comments

@Uxio0
Copy link
Contributor

Uxio0 commented Mar 21, 2018

  • Version: 4.0.0b11
  • Python: 3.6
  • OS: linux

What was wrong?

As stated in the docs http://web3py.readthedocs.io/en/latest/overview.html#Web3.isAddress the function Web3.isAddress(value) should allow both 0x prefixed and non prefixed values.

If the address is not checksumed, it's ok not to have the 0x:

>>> Web3.isAddress('d3cda913deb6f67967b99d67acdfa1712c293601')
>>> True

But if it's checksumed

>>> Web3.isAddress('d3CdA913deB6f67967B99D67aCDFa1712C293601')
>>> False

No problem if we add the 0x:

>>> Web3.isAddress('0xd3CdA913deB6f67967B99D67aCDFa1712C293601')
>>> True

How can it be fixed?

Changing the documentation to state that checksumed addresses must have 0x or changing the function to accept checksumed addresses with 0x. I would just remove 0x at the beginning of the function (if found) and work with the address as that.

@pipermerriam
Copy link
Member

pipermerriam commented Mar 21, 2018

This is due to a discrepancy between is_checksum_formatted_address and is_checksum_address. The former returns True but the later False

In [79]: is_checksum_formatted_address('d3CdA913deB6f67967B99D67aCDFa1712C293601')
Out[79]: True

In [80]: is_checksum_address('d3CdA913deB6f67967B99D67aCDFa1712C293601')
Out[80]: False

We could:

  • Special case is_address to allow non-prefixed checksummed addresses.
  • Be more forgiving with is_checksum_address to allow non-prefixed values.
  • Remove the checksum checks since is_hex_address('d3CdA913deB6f67967B99D67aCDFa1712C293601') -> True

I'm torn. I like having a very strict representation of checksummed addresses (as well as an unambiguous requirement that they have 0x prefixes to remove ambiguity of whether it's a hex value). However, I can see a decent argument for being more forgiving with is_checksum_address and allowing non-prefixed values.

@carver
Copy link
Collaborator

carver commented Mar 21, 2018

The reason it works this way is because of how EIP-55 defines the checksum addresses. According to spec, it's not a valid checksum address if it does not have the "0x" prefix. Note that the test() method defined in the spec is only valid on prefixed addresses.

So this seems like the right solution:

Changing the documentation to state that checksumed addresses must have 0x

@pipermerriam
Copy link
Member

Ok, but the real question here is *what is the is_address function intended to be used for?

  • is this a valid address
  • is this an address

If it's either of the valid variations then I think the function is poorly named, so I'd posture that the right fix is to update the docs as you specified, but to remove the checksum checking from the is_address function and have it simply return something like return is_hex_address(v) or is_binary_address(v)

@carver
Copy link
Collaborator

carver commented Mar 22, 2018

the real question here is *what is the is_address function intended to be used for?

Good point. It used to be that isAddress was a test for "is this value valid as an input, where addresses are required in web3.py?" I don't think we've re-evaluated the use case of this method, given the rule "only checksum addresses are accepted" in web3 v4.

I suppose it should be: "is this address valid as an input to: toChecksumAddress". Something seems a bit wrong about accepting invalid checksums as input to toChecksumAddress, but that's not a check we're doing now AFAIK, and I don't have a well-formed opinion that we should add that check. So I guess that leads us to the same place @pipermerriam described:

def is_address(v):
    return is_hex_address(v) or is_binary_address(v)

@fselmo fselmo added the v7 breaking changes considered for v7 label Sep 25, 2023
@fselmo
Copy link
Collaborator

fselmo commented Sep 25, 2023

Once this is merged in eth-utils, major version bump for eth-utils in web3.py would be needed since it is a breaking change. Basically, for v7, we would need eth-utils>={relevant major version with this change} in setup.py.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority: p3 normal v7 breaking changes considered for v7
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants