Added isAddress() validation method #134

assafY · 2017-07-14T14:11:20Z

While using the Web3j Numeric and Hash libraries to implement a isAddress() method in a local project, I have realised the method hexStringToByteArray() is not only unnecessary (you can just use the native .getBytes() on a string), but also returns an incorrect byte array.

To verify this, the isAddress() method in this PR fails if using the aforementioned method, but works if using the native .toBytes().

I have therefore removed the method content and replaced it simply with .getBytes(), and implemented the isAddress() method in a new class . This method is based on the web3.js implementation and is working perfectly in my own project.

… isAddress validation method

assafY · 2017-07-14T14:56:26Z

so I realise now many tests depend on the methods in Numeric.java and Hash.java to remain as they are. Therefore I have kept them as they are, and use the method in Hash.java that accepts a byte array directly.

codecov-io · 2017-07-14T15:00:49Z

Codecov Report

Merging #134 into master will increase coverage by <.01%.
The diff coverage is 78.94%.

@@             Coverage Diff              @@
##             master     #134      +/-   ##
============================================
+ Coverage     78.77%   78.77%   +<.01%     
- Complexity     1241     1251      +10     
============================================
  Files           181      182       +1     
  Lines          4339     4358      +19     
  Branches        662      668       +6     
============================================
+ Hits           3418     3433      +15     
- Misses          742      743       +1     
- Partials        179      182       +3

Impacted Files	Coverage Δ	Complexity Δ
core/src/main/java/org/web3j/utils/Account.java	`78.94% <78.94%> (ø)`	`10 <10> (?)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 41fdc38...bd23fe2. Read the comment docs.

conor10 · 2017-07-16T20:49:54Z

Hi @assafY, please can you provide a unit test that demonstrates this error you believe exists in hexStringToByteArray?

assafY · 2017-07-17T15:27:58Z

Sure, but it would be useful if you told me first whether you intended for hexStringToByteArray to transform a String into a byte array in a similar way that String.getBytes() does? Because the two return different results for the same string, so if that was the intention I can quickly write a unit test that demonstrates this error.

conor10 · 2017-07-17T18:18:15Z

If you call String.getBytes() on a hex string, you get the ASCII values for each of the characters in the String. This is not the same as a byte array which is a binary encoded array of data.

E.g. Using getBytes:

"123".getBytes() == new byte[] { '1', '2', '3'}

Or equivalently:

new byte[] { (byte) 49, (byte) 50, (byte) 51 };

Whereas with a byte array (remember the 123 is a hex not decimal value here):

hexStringToByteArray("123") == new byte[] { (byte) 1, (byte) 35 }

assafY · 2017-07-24T12:03:09Z

I understand - so I am assuming that the checksum validation for newer Ethereum account addresses, as described here, uses the ASCII value byte representation rather than the one you implemented in hexStringToByteArray.

In this case I think the comparison between the two is irrelevant, and the checksum validation method I added which uses getBytes works independently. Checksum validation is now used by most major ether wallets and I assumed it would be a useful addition to the Web3j library.

clowdrgn · 2017-07-26T02:28:57Z

Hi, I try to validate a address but cant pass.Maybe this line String hash = Numeric.toHexString(Hash.sha3(address.replace("0x", "").getBytes()));should toLowerCase when get address hash value ?and I can verify success when I change to String hash = Numeric.toHexStringNoPrefix(Hash.sha3(address.replace("0x", "").toLowerCase().getBytes())); Or do I miss some method ?

assafY · 2017-07-26T07:38:25Z

That is correct, thanks! You do need toLowerCase() before validation. I added that now.

Mawster · 2017-08-30T10:08:57Z

src/main/java/org/web3j/utils/Account.java

@@ -38,7 +38,7 @@ public static Boolean isAddress(String address) {

    private static Boolean validateChecksumAddress(String address) {
        String formattedAddress = address.replace("0x", "").toLowerCase();
-        String hash = Numeric.toHexString(Hash.sha3(formattedAddress.getBytes()));
+        String hash = Numeric.toHexStringNoPrefix(Hash.sha3(formattedAddress.getBytes()));
        for (int i = 0; i < 40; i++) {
            if (Character.isLetter(address.charAt(i))) {


Why do you check from the address? This seems to be wrong - address still contains the leading '0x'

Sure, I missed that method. In this case you can also remove the .replace("0x", "") above, which I assumes is what happens in toHexStringNoPrefix?

Ah I misunderstood your question. This is actually the correct way to do it. The hash needs to be computed from the lowercase address without a prefix, but the validation of the hash should happen against the original, untouched address.

ethereum/EIPs#55

No I don't think that you can remove it - if you remove it it would be used in the sha3 hashing.

compare with
https://ethereum.stackexchange.com/questions/1374/how-can-i-check-if-an-ethereum-address-is-valid

especially this line:
address = address.replace('0x','');

using the untouched address with leading '0x' is false

Oh you are correct - thank you for pointing that out. The checksum validation should be carried out on the address without the 0x prefix. Fixed that now, thanks!

Now you are ussing the lower cased version also in the comparision - thats not right.

Character.isUpperCase(address.charAt(i)) && charInt <= 7
can never happen

Slow day for me today isn't it? Fixed that now, thanks.

Mawster · 2017-08-30T10:10:53Z

src/main/java/org/web3j/utils/Account.java

@@ -38,7 +38,7 @@ public static Boolean isAddress(String address) {

    private static Boolean validateChecksumAddress(String address) {
        String formattedAddress = address.replace("0x", "").toLowerCase();


I have to validate eth addresses in a current project - I modified your version:

String formattedAddress = address.replace("0x", ""); String lowercaseAddress = formattedAddress.toLowerCase(); String hash = Numeric.toHexStringNoPrefix(Hash.sha3(lowercaseAddress.getBytes())); for (int i = 0; i < 40; i++) { if (Character.isLetter(formattedAddress.charAt(i))) {

…hout prefix

Mawster · 2017-08-30T12:59:41Z

Some tests wouldn't be bad

assafY · 2017-11-08T11:35:12Z

anything stopping this from being merged?

conor10 · 2017-11-09T14:52:18Z

core/src/main/java/org/web3j/utils/Account.java

+     * @param address given address in HEX
+     * @return is this a valid address
+     */
+    public static Boolean isAddress(String address) {


WalletUtils.isValidAddress achieves similar functionality.

conor10 · 2017-11-09T14:56:53Z

core/src/main/java/org/web3j/utils/Account.java

+        }
+    }
+
+    private static Boolean validateChecksumAddress(String address) {


Validating checksum addresses is useful functionality.

It might be better to have isChecksumAddress as a method on the Address type, and a toChecksumAddress that operates on an Address or String instance

conor10 · 2017-11-09T14:58:43Z

See comments. It's also useful to reference the original EIP for checksum addresses ethereum/EIPs#55, and include the corresponding issue in your commits.

I'd recommend you create a brand new PR, as there are a lot of commits in this PR.

Thanks for taking the time to get this far.

assafY added 3 commits July 14, 2017 15:06

changed hex to byte array to just return string .toBytes(), and added…

e5190d5

… isAddress validation method

reverted hexStringToByteArray() back, but removed use in Hash.java

171abcd

reverted Hash.java back, changed isAddress to use .getBytes() instead

e0b89f7

added toLowerCase() before address hashing and validation

f438dd6

assafY added 2 commits July 26, 2017 08:45

split address formatting and hashing to two lines

ed09f30

changed toHexString() to toHexStringNoPrefix()

6844e81

Mawster reviewed Aug 30, 2017

View reviewed changes

assafY added 2 commits August 30, 2017 11:51

using address without 0x prefix in checksum validation

2acd193

hash lower case address without prefix, validate checksum address wit…

c9c3053

…hout prefix

assafY added 7 commits September 13, 2017 17:10

added isAddress tests

0ee4848

changed test vars to private

141bb06

fixed style fails

4230ee5

small style fix. Local builds pass

1f480b1

Merge branch 'master' of https://github.com/web3j/web3j

e8a3d31

moved files to new folder structure

2c3d5fb

style fix

bd23fe2

conor10 reviewed Nov 9, 2017

View reviewed changes

assafY closed this Feb 16, 2018

Gelassen mentioned this pull request Dec 2, 2022

Address checksum check implemented in Web3j? #568

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added isAddress() validation method #134

Added isAddress() validation method #134

assafY commented Jul 14, 2017

assafY commented Jul 14, 2017

codecov-io commented Jul 14, 2017 •

edited

Loading

conor10 commented Jul 16, 2017

assafY commented Jul 17, 2017

conor10 commented Jul 17, 2017

assafY commented Jul 24, 2017

clowdrgn commented Jul 26, 2017

assafY commented Jul 26, 2017

Mawster Aug 30, 2017

assafY Aug 30, 2017

assafY Aug 30, 2017

Mawster Aug 30, 2017

Mawster Aug 30, 2017 •

edited

Loading

assafY Aug 30, 2017

Mawster Aug 30, 2017 •

edited

Loading

assafY Aug 30, 2017

Mawster Aug 30, 2017

Mawster commented Aug 30, 2017

assafY commented Nov 8, 2017

conor10 Nov 9, 2017

conor10 Nov 9, 2017

conor10 commented Nov 9, 2017

		@@ -38,7 +38,7 @@ public static Boolean isAddress(String address) {

		private static Boolean validateChecksumAddress(String address) {
		String formattedAddress = address.replace("0x", "").toLowerCase();

Added isAddress() validation method #134

Added isAddress() validation method #134

Conversation

assafY commented Jul 14, 2017

assafY commented Jul 14, 2017

codecov-io commented Jul 14, 2017 • edited Loading

Codecov Report

conor10 commented Jul 16, 2017

assafY commented Jul 17, 2017

conor10 commented Jul 17, 2017

assafY commented Jul 24, 2017

clowdrgn commented Jul 26, 2017

assafY commented Jul 26, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mawster Aug 30, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mawster Aug 30, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mawster commented Aug 30, 2017

assafY commented Nov 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

conor10 commented Nov 9, 2017

codecov-io commented Jul 14, 2017 •

edited

Loading

Mawster Aug 30, 2017 •

edited

Loading

Mawster Aug 30, 2017 •

edited

Loading