Added support for eip-1191

[jonathansmirnoff]

It was implemented EIP-1191. By default for every network it will be used EIP-55.
It was necessary to update the version of Kethereum to support this PR. Also it was fixed chainId of RSK Network.

This PR is related to #305.

Please @ligi let me know what you think about these changes.

[ligi]

It needs to be a different address field as mentioned here: #305 (comment)
otherwise this can break upstream projects.

[jonathansmirnoff]

@ligi thanks for your answer, I thought that you asked me to not modified the token json file. So I need more clarification about the solution that you prefer:

• Add a new field to the json token file: address_eip1191, then I will need to change Token.kt. This field will be optional.
• TokenChecker.kt will recibe always the chainId. If the token to be checked has the address_eip1191 it will validated it throw the eip1191.

Are you agreed with this?

[ligi]

• Add a new field to the json token file: address_eip1191, then I will need to change Token.kt. This field will be optional.

yes

TokenChecker.kt will recibe always the chainId. If the token to be checked has the address_eip1191 it will validated it throw the eip1191.

yes

Are you agreed with this?

yes

[jonathansmirnoff]

@ligi I did the changes that we have talked about. Please check them. Thanks!

[ligi]

This should also throw an error when the address_eip1191 is non null. We need to make sure that the address field is always valid

[jonathansmirnoff]

I did because we need the name of the file has the checksum following the eip1191 when it is appropriate. This allow us to have a better integration with Ledger and Trezor wallets for the different tokens that are in RSK.

Maybe something that we can do is this:

• if address_eip1191 is empty or null, it will be validated using erc55 rules the address field. (as it is working today)
• if address_eip1191 has a value, it will be validate with erc1191 rules and also the address field. So basically the two fields has to be the same.

Another option is to modify the file name validation and allow to check the file name by address_eip1191 if it is appropriate.

Let me know what are your thought about this. Thanks!

[ligi]

unfortunately this will not work - we cannot break the guarantees that the address field brings.
To have better TREZOR integration you need to change it on the TREZOR side - meaning there the check if the address_eip1191 field is there and if it is use it instead of the address field.

[jonathansmirnoff]

Ok, let's do what you have suggested. I will do the modifications and update the PR.

[ligi]

Just this one point - otherwise the PR looks good to me

[ligi]

there are currently failing tests after this change - can you have a look? All tests need to pass before I can merge

[jonathansmirnoff]

there are currently failing tests after this change - can you have a look? All tests need to pass before I can merge

Yes, sure I will review.

[ligi]

for me the tests still fail - even with a clean:

✗ ~/g/e/tokens (pr/339 *$) ./gradlew clean test                                                                                                                       23:25:01

> Task :test FAILED

TheTokenChecker > shouldPassForValidTokenEip1191 FAILED
    java.lang.IllegalStateException at TheTokenChecker.kt:117

TheTokenChecker > shouldFailForInvalidChecksumEip1191 FAILED
    java.lang.Exception at ExpectException.java:28
        Caused by: java.lang.IllegalStateException at TheTokenChecker.kt:117

15 tests completed, 2 failed

[jonathansmirnoff]

for me the tests still fail - even with a clean:

✗ ~/g/e/tokens (pr/339 *$) ./gradlew clean test                                                                                                                       23:25:01

> Task :test FAILED

TheTokenChecker > shouldPassForValidTokenEip1191 FAILED
    java.lang.IllegalStateException at TheTokenChecker.kt:117

TheTokenChecker > shouldFailForInvalidChecksumEip1191 FAILED
    java.lang.Exception at ExpectException.java:28
        Caused by: java.lang.IllegalStateException at TheTokenChecker.kt:117

15 tests completed, 2 failed

I remember that I had the same issue. I fixed it removing the build and .gradle folder. I think this because kethereum version was updated and now it is used new functions.
I will try to do a rebase to get the last changes of the repo.

[jonathansmirnoff]

I cloned again my repo and did ./gradlew clean test it worked fine:

./gradlew clean test 
Starting a Gradle Daemon (subsequent builds will be faster)

Deprecated Gradle features were used in this build, making it incompatible with Gradle 7.0.
Use '--warning-mode all' to show the individual deprecation warnings.
See https://docs.gradle.org/6.3/userguide/command_line_interface.html#sec:command_line_warnings

BUILD SUCCESSFUL in 25s
5 actionable tasks: 4 executed, 1 up-to-date

[ligi]

I guess you ran these tests on a non case sensitive OS ..-)

[jonathansmirnoff]

@ligi I could fix the issue :)