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

Improve warning on failed address checksum #716

Closed
carver opened this issue Mar 21, 2018 · 14 comments · Fixed by #803
Closed

Improve warning on failed address checksum #716

carver opened this issue Mar 21, 2018 · 14 comments · Fixed by #803

Comments

@carver
Copy link
Collaborator

carver commented Mar 21, 2018

  • Version: 4

What was wrong?

Error message about checksum failure isn't clear enough, especially when the address is supplied as all-lower-case.

How can it be fixed?

Special-case the warning, so that an all-lower-case address gives a specific warning, along the lines of: "web3py only accepts checksummed addresses. Please report it as a bug on any project that does not provide you checksummed addresses. In the meantime, you can force an address into checksummed state with valid_address = w3.toChecksumAddress(questionable_address) after double-checking that there are no errors in the address. Alternatively, you can use an ENS name in its place."

But, you know, not a paragraph...

@dangell7
Copy link

dangell7 commented Apr 4, 2018

Why don’t you just force toChecksum on every call?

@pipermerriam
Copy link
Member

pipermerriam commented Apr 4, 2018

@dangell7 because that would defeat the purpose of checksummed addresses. We require that the actual input value be checksummed so that web3 can then verify validity.

@pipermerriam
Copy link
Member

I think there are multiple classes of error messages if we want to be extra friendly.

  • if all lower, give instructions on how to checksum an address.
  • if mixed-case and bad checksum, show a diff of sorts of the input value and the properly checksummed value.

@carver
Copy link
Collaborator Author

carver commented Apr 4, 2018

@voith this would be a helpful one to work on (see Piper's comments, too)

@dangell7
Copy link

dangell7 commented Apr 4, 2018

I understand the reason. I'm just saying, in every api call I make on my system, the "from" and "to" address has toChecksumAddress(address) to avoid any errors arising. I was just suggesting that on your end, you apply the to checksum address to the "to" and "from" address of the transaction. I get it though. It's your software.

@dangell7
Copy link

dangell7 commented Apr 4, 2018

I also wasn't aware that there were different kinds of errors on the address, and I haven't run into those errors yet

@carver
Copy link
Collaborator Author

carver commented Apr 4, 2018

in every api call I make on my system, the "from" and "to" address has toChecksumAddress(address) to avoid any errors arising.

Every output of from and to should already be checksummed. If it's not, it's a bug. Can I ask what source you're using to generate the addresses in a non-checksum way?

@dangell7
Copy link

dangell7 commented Apr 4, 2018

I meant every input. If I get the address from web3/metamask/dappbrowser and save it to the db or to a file. When I open that address its never "in checksum". And because in my code, sometimes I grab directly from web3 and sometimes I use the db, I ended up just adding the checksum on every call. I didn't test for performance, but it seems like a small call.

@dangell7
Copy link

dangell7 commented Apr 4, 2018

Reading more though, I understand why that is not a solution to the problem. I was just giving my input of how I solved the checksum problem in my system

@carver
Copy link
Collaborator Author

carver commented Apr 4, 2018

If I get the address from web3/metamask/dappbrowser

Manually copying addresses from other sources seems like a use case where you'd really want to make sure the checksum is valid.

and save it to the db or to a file

Saving it to a db or file and loading it should maintain the checksum. Is your database forcing all characters to lowercase at some point?

I didn't test for performance, but it seems like a small call.

Yeah, I wouldn't expect a big performance impact.

I was just giving my input of how I solved the checksum problem in my system

That's exactly what I wanted: some insight into how it's being used in production. Thanks!

One thing that I would like to see is more other sources producing checksum addresses. It seems totally reasonable to request these other vendors (metamask/dappbrowser/web3.js/etc) to produce checksum addresses everywhere. If you're not up for opening issues on them, let me know which services you're using that do not provide checksum addresses, and I'll open issues on them. :)

@dangell7
Copy link

dangell7 commented Apr 4, 2018

MetaMask/Cipher and both web3 (py & js) repos output checksum. Firebase DB and Firestore both "downgrade" the address. Also, using python, saving to json, "downgrades" the address. Those are the instances I have found. So its pretty much only when I use the db address. Thinking more, you're right. If I'm doing the saving, I should know if its checksum ed.

@carver
Copy link
Collaborator Author

carver commented Apr 4, 2018

Firebase DB and Firestore both "downgrade" the address.

Are you using the "text string" type on Firestore? That and Firebase should both preserve the case of the letters in the address.

Also, using python, saving to json, "downgrades" the address.

How are you writing it to json? json maintains the case of text:

import json

jsondata = {'addrs': ["0xbB902569a997D657e8D10B82Ce0ec5A5983C8c7C", "0xE853c56864A2ebe4576a807D26Fdc4A0adA51919"]}

assert all(Web3.isChecksumAddress(addr) for addr in jsondata['addrs'])

with open('/tmp/json-addrs', 'w') as f:
    f.write(json.dumps(jsondata))

with open('/tmp/json-addrs') as f:
    loaded_info = json.loads(f.read())

assert all(Web3.isChecksumAddress(addr) for addr in loaded_addrs['addrs'])

@dangell7
Copy link

dangell7 commented Apr 5, 2018

Huh, for the json I was doing it the same way. and for firestore and firebase as you noted. I wonder where my error occurred then. I would also note that I use web3.js on the client and web3.py on the server. I'm not really sure where the error occurred though. My code is 3 months old... not sure if that has anything to do with it. I was definitely getting the error at one point. Hence the reason I made every call use the toChecksumAddress. Maybe I'll try taking it off and testing it. Thanks for the info.

@dangell7
Copy link

dangell7 commented Apr 5, 2018

Yeah, so I now only do the checksum on the client side. So nothing to do with web3.py. Its either metamask/cipher or react or its been updated and I'm doing it for no reason now... lol

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants