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

Implement EIP712 #911

Merged
merged 3 commits into from May 29, 2019

Conversation

Projects
None yet
4 participants
@stringray55
Copy link
Contributor

commented Apr 1, 2019

What does this PR do?

Fixes #898

@stringray55 stringray55 requested review from conor10 and iikirilov as code owners Apr 1, 2019

@stringray55

This comment has been minimized.

Copy link
Contributor Author

commented Apr 1, 2019

@iikirilov could you please give me a preliminary review? The hashes are matching as per the spec. I haven't written the test cases specifically, but will do it. I just need to know the style of code that you would recommend (Hence a preliminary review). Thankyou.

@stringray55 stringray55 changed the title Add Base Structure for EIP712 [WIP] Implement EIP712 Apr 1, 2019

@iikirilov

This comment has been minimized.

Copy link
Member

commented Apr 2, 2019

@stringray55 I am happy that your implementation seems to work. Let's get some tests going. That will help to restructure the code in the future.

@stringray55 stringray55 force-pushed the stringray55:implement_EIP_712 branch 2 times, most recently from eacb0e4 to ac48d10 Apr 3, 2019

@stringray55

This comment has been minimized.

Copy link
Contributor Author

commented Apr 3, 2019

@iikirilov I have added the test cases taken from the spec. Could you please review it? Thankyou.

@iikirilov
Copy link
Member

left a comment

Looks good so far. Can we refactor this though - let's have a data class and encoder and a decoder.

@iikirilov

This comment has been minimized.

Copy link
Member

commented Apr 3, 2019

I would also like to have a failing test so we can see how your code reacts if some tries to encode/decode some garbage.

@iikirilov iikirilov added this to the 4.3.x milestone Apr 3, 2019

@iikirilov

This comment has been minimized.

Copy link
Member

commented Apr 3, 2019

Also please rebase from the 4.3 branch

@stringray55

This comment has been minimized.

Copy link
Contributor Author

commented Apr 3, 2019

@iikirilov I can split into the Data Class and the Encoder Class, but what is the Decoder Class here? Because I don't think that there is a concept of Decdoing in the EIP (Please correct me if wrong).

@iikirilov

This comment has been minimized.

Copy link
Member

commented Apr 4, 2019

@iikirilov I can split into the Data Class and the Encoder Class, but what is the Decoder Class here? Because I don't think that there is a concept of Decdoing in the EIP (Please correct me if wrong).

This may be my lack of understanding but I assume that if you can encode a struct to a string you should be able to decode the string back to a struct

@iikirilov

This comment has been minimized.

Copy link
Member

commented Apr 4, 2019

Nevermind - of-course you cannot "decode" a hash so a "decoder" does not make sense here

@stringray55 stringray55 force-pushed the stringray55:implement_EIP_712 branch from ac48d10 to 71c3032 Apr 4, 2019

@stringray55

This comment has been minimized.

Copy link
Contributor Author

commented Apr 4, 2019

@iikirilov I have added more testcases and refactored the code as you suggested. Please take a look at it.

@iikirilov iikirilov changed the base branch from master to release/4.3 Apr 6, 2019

@stringray55

This comment has been minimized.

Copy link
Contributor Author

commented Apr 15, 2019

@iikirilov please review. I have completely finished the implementation of EIP712. WIP has been removed now also.

@stringray55 stringray55 changed the title [WIP] Implement EIP712 Implement EIP712 Apr 15, 2019

@iikirilov iikirilov requested a review from snazha-blkio Apr 16, 2019

@iikirilov

This comment has been minimized.

Copy link
Member

commented Apr 16, 2019

@snazha-blkio can I have a second pair of eyes here

@stringray55 stringray55 force-pushed the stringray55:implement_EIP_712 branch from 7f42874 to cc8cff3 Apr 17, 2019

@stringray55

This comment has been minimized.

Copy link
Contributor Author

commented Apr 17, 2019

@serso I have made the changes, please review again. Thankyou.

@serso

This comment has been minimized.

Copy link
Contributor

commented Apr 17, 2019

@stringray55 np. I normally don't review code in this repo but I stumbled upon this change and was curious. @iikirilov and @snazha-blkio should do a proper code review.

@stringray55

This comment has been minimized.

Copy link
Contributor Author

commented Apr 17, 2019

@serso Basically ArrayList is inherited from List so why would it crash and why would we need to cast it . (pls correct if iam wrong )

@stringray55

This comment has been minimized.

Copy link
Contributor Author

commented Apr 17, 2019

@serso I think you got the use case wrong. We initially have the message in the JSON string format. And then the Jackson library converts the whole string into a suitable JSON Object (which would have LinkedHashMap and ArrayList only). So I don't see any case where the data would be given as a LinkedList. I know that it is possible to give an argument of the type LinkedList for that function in general, but on the overall flow, I don't think that passing a LinkedList would ever happen.

That said, should that function be made a private method?
/cc @iikirilov @snazha-blkio

@serso

This comment has been minimized.

Copy link
Contributor

commented Apr 17, 2019

@stringray55 if it's possible to make it private - I'd do so. If you expect the argument to be ArrayList only, then I think you should revert the initial fix and check ArrayList.class.isInstanceOf. I wouldn't rely on Jackson as it's a separate library and if they suddenly decide to switch to some other List implementation then the code wouldn't work.

@snazha-blkio
Copy link
Collaborator

left a comment

Please refer to the inline comments, thanks.

@stringray55 stringray55 force-pushed the stringray55:implement_EIP_712 branch from cc8cff3 to de531f3 May 8, 2019

@stringray55

This comment has been minimized.

Copy link
Contributor Author

commented May 8, 2019

@iikirilov @snazha-blkio Sorry for the delay, but the new changes have been pushed which address the above mentioned comments. Please review again. Thankyou.

/cc @ceresstation

@snazha-blkio snazha-blkio modified the milestones: 4.3.0, 5.0 May 10, 2019

@snazha-blkio snazha-blkio self-requested a review May 15, 2019

@snazha-blkio
Copy link
Collaborator

left a comment

@stringray55 , LGTM, as long as you address the Pair concerns raised by @iikirilov .

@stringray55

This comment has been minimized.

Copy link
Contributor Author

commented May 15, 2019

@iikirilov @snazha-blkio I would like to create my own Pair class, as I feel that using a Pair would make sense in all of the cases. Please tell me where to place the pair.java class file. Would it be ok if I put it in the directory web3j/crypto/src/main/java/org/web3j/crypto/ (which is basically along with the StructuredData.java files)?

@iikirilov

This comment has been minimized.

Copy link
Member

commented May 15, 2019

@stringray55 you can add it in utils under the crypto package

@stringray55 stringray55 force-pushed the stringray55:implement_EIP_712 branch from 01557e9 to ee43d33 May 15, 2019

@stringray55

This comment has been minimized.

Copy link
Contributor Author

commented May 15, 2019

@iikirilov @snazha-blkio all set. Please review again. Thankyou.

@snazha-blkio
Copy link
Collaborator

left a comment

It looks better now, but there are some optimisations re: Pattern that we would like, and a clean up of the method declarations (use of base classes/interfaces).

Could you please look into the latest comments? We'd like to keep the interface as clean as possible.
TIA.

@stringray55 stringray55 force-pushed the stringray55:implement_EIP_712 branch from ee43d33 to 2a522c0 May 16, 2019

@stringray55

This comment has been minimized.

Copy link
Contributor Author

commented May 16, 2019

@snazha-blkio the new review has been addressed in the latest push. Please review. Thankyou.

@snazha-blkio snazha-blkio self-requested a review May 26, 2019

@snazha-blkio

This comment has been minimized.

Copy link
Collaborator

commented May 26, 2019

Thanks @stringray55 , looks good now.
@iikirilov , 4.3.1? 4.4.0? or 5.0?

@snazha-blkio snazha-blkio merged commit e7d1c1f into web3j:release/4.3 May 29, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@snazha-blkio snazha-blkio modified the milestones: 5.0, 4.3.0 May 29, 2019

@snazha-blkio snazha-blkio modified the milestones: 4.3.0, 4.3.1 May 29, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.