Improves checksum error message #3031
Conversation
libdevcore/CommonData.cpp
Outdated
| @@ -103,3 +103,26 @@ bool dev::passesAddressChecksum(string const& _str, bool _strict) | |||
| } | |||
| return true; | |||
| } | |||
|
|
|||
| std::string dev::getChecksumAddress(std::string const& _addr) | |||
There was a problem hiding this comment.
This could use some refactor and common parts be shared between this and passesAddressChecksum.
There was a problem hiding this comment.
Sure, I'll push something up in a moment. One thing I think I could do as well, is just get rid of the check to see if addressCharacter is a number or not - according to isupper docs, it'll return the same character if a number (or something without an uppercase component) is passed in. I wasn't sure if I should leave the check in for clarity though.
|
Refactored. Let me know if that's what you had in mind. |
|
Sorry, by refactoring I've meant to pull out the common parts between the two functions. |
libsolidity/analysis/TypeChecker.cpp
Outdated
| "If this is not used as an address, please prepend '00'." | ||
| "If this is not used as an address, please prepend '00'. " | ||
| "Correct checksummed address: '" + _literal.getChecksumAddress() + "'. " | ||
| "For more information, please see https://github.com/ethereum/EIPs/issues/55" |
There was a problem hiding this comment.
I think this should instead point to https://solidity.readthedocs.io/en/develop/types.html#address-literals
There was a problem hiding this comment.
Gotcha. I'll change that.
|
Okay - for clarification, you want a few helper functions that both functions call? Like "getAddressSubstring," "getAddressHash," and "getNibble"? |
|
There should be one which converts an address (like the one you have right now) to the mixed-case version and that can be used in the verify function to compare with the supplied one. |
|
Changed and fixed. Let me know if there's anything else you see |
axic
force-pushed
the
develop
branch
4 times, most recently
from
ad54a55
to
d0dd7b1
Compare
|
@chriseth can you review this now? |
libdevcore/CommonData.cpp
Outdated
| h256 hash = keccak256(boost::algorithm::to_lower_copy(s, std::locale::classic())); | ||
|
|
||
| assertThrow(s.length == 40, ""); | ||
| assertThrow(hash.find_first_not_of("abcdef") == string::npos, ""); |
There was a problem hiding this comment.
hash is a h256, i.e. it is binary data. I don't think we should search for hex digits there.
There was a problem hiding this comment.
Yeh it should be done before on the input.
libsolidity/analysis/TypeChecker.cpp
Outdated
| @@ -2000,7 +2000,9 @@ void TypeChecker::endVisit(Literal const& _literal) | |||
| m_errorReporter.warning( | |||
| _literal.location(), | |||
| "This looks like an address but has an invalid checksum. " | |||
| "If this is not used as an address, please prepend '00'." | |||
| "If this is not used as an address, please prepend '00'. " | |||
| "Correct checksummed address: '" + _literal.getChecksumAddress() + "'. " | |||
There was a problem hiding this comment.
looksLikeAddress cat retur true for strings of length 39, 40 and 41, but getChecksumAddress asserts that the string is exactly 40 hex digits long.
There was a problem hiding this comment.
Either we add another condition here and display the "correct" checksum if it is 40 characters or we just don't display it at all. It is worth displaying it?
There was a problem hiding this comment.
We should display the correct checksum for 40 characters. Otherwise, we tell the user the correct length. I think a compiler should always supply an "easy fix" method, if it exists. It is in the user's responsibility to accept it or not.
There was a problem hiding this comment.
What is the idea behind doing this for 41 hex digits?
There was a problem hiding this comment.
I basically see the following reasons for a checksum failure:
- typing a wrong hex digit
- swapping two hex digits
- repeating a hex digit (or generally, adding a non-existing hex digit)
- leaving out a hex digit
Because of that, we should treat hex digit sequences of lengths 39, 40 and 41 as possible addresses.
axic
force-pushed
the
develop
branch
4 times, most recently
from
f51577c
to
0e7742c
Compare
|
@chriseth ready for review |
|
Should add separate tests for |
Now displays the correct checksummed address and gives a link to ethereum/EIPs#55
Issue 2094: #2094