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

"from address mismatch" when tx.from is lowercased #1236

Closed
miguelmota opened this issue Jan 13, 2021 · 5 comments
Closed

"from address mismatch" when tx.from is lowercased #1236

miguelmota opened this issue Jan 13, 2021 · 5 comments
Labels
bug Verified to be an issue. fixed/complete This Bug is fixed or Enhancement is complete and published.

Comments

@miguelmota
Copy link

miguelmota commented Jan 13, 2021

If tx.from is lowercased then this line throws the from address mismatch because it's comparing against a checksummed address. Would it be better to checksum the tx.from address and then do the comparison? since they're both the same address so I wouldn't expect it to throw.

if (result[0] !== result[1]) {

@ricmoo
Copy link
Member

ricmoo commented Jan 13, 2021

Good catch. I'll roll this up into a few other changes going out soon.

Thanks! :)

@ricmoo ricmoo added bug Verified to be an issue. on-deck This Enhancement or Bug is currently being worked on. labels Jan 13, 2021
@ricmoo
Copy link
Member

ricmoo commented Jan 13, 2021

I had to go the other way. Use toLowerCase on them both. Since this is a low-level base class, I try to minimize the dependencies, and the address checksum would add the keccak256 dependency, and depending on tree shaking could also drag in BigNumber.

This does bring up an issue I will address in v6, which is this means that if you use an IBAN/ICAP address, in the from field things will get dicey. That said, I think ICAP is all but dead, and I've been considering dropping support for it in v6 (a utility function will still exist for it though; it just won't be a first-class address format).

@miguelmota
Copy link
Author

miguelmota commented Jan 13, 2021

Awesome! sounds good, thanks 🙂

@ricmoo
Copy link
Member

ricmoo commented Jan 14, 2021

This should be addressed in 5.0.26. Please try it out and let me know. :)

@miguelmota
Copy link
Author

Works great!! Closing as resolved.

@ricmoo ricmoo added fixed/complete This Bug is fixed or Enhancement is complete and published. and removed on-deck This Enhancement or Bug is currently being worked on. labels Jan 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Verified to be an issue. fixed/complete This Bug is fixed or Enhancement is complete and published.
Projects
None yet
Development

No branches or pull requests

2 participants