Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

add salt to EIP712 domain #1260

Merged
merged 2 commits into from
Oct 30, 2020

Conversation

alpa-coder
Copy link
Contributor

What does this PR do?

Adds salt to the EIP712Domain. According to the EIP, a salt can be added to the domain:

an disambiguating salt for the protocol. This can be used as a domain separator of last resort

I double checked the results with what I got back from an EIP712 demo together with metamask. I also verified it in remix.

Where should the reviewer start?

4 changes:

  • Added the salt in StructuredData - EIP712Domain
  • Made sure the salt is not present when it is null (StructuredDataEncoder)
  • Added a valid JSON test file with salt
  • Created a new test to validate the result

Why is it needed?

If you use a valid EIP712 json with a salt, you get a deserialisation exception (unknown field).
Since this is part of the EIP it should be implemented.

@iikirilov-wmt
Copy link

LGTM - although someone needs to decide something about backwards compatibility something

@alpa-coder
Copy link
Contributor Author

LGTM - although someone needs to decide something about backwards compatibility something

I checked, inside @JsonProperty the required is by default false:

boolean required() default false;

All tests inside "StructuredDataTest.java" still pass, which still has examples without salt and they are still green 👍
A test was added (and example json file) that tests the structured data with salt. I did not touch the ones without salt.

@xaviarias xaviarias merged commit 8cb1dce into hyperledger-web3j:master Oct 30, 2020
@conor10
Copy link
Contributor

conor10 commented Nov 12, 2020

This pull request has been mentioned on Web3 Labs Community. There might be relevant details there:

https://community.web3labs.com/t/web3j-v4-8-0-released/112/1

rach-id pushed a commit to rach-id/web3j that referenced this pull request Dec 10, 2021
* add salt to EIP-712 domain

* ran `gradlew spotlessApply`
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants