Skip to content

Conversation

@stevenleeg
Copy link
Contributor

This fixes an issue I noticed with #17789 (cc @PaulRBerg) which resulted in a bad PrettyPrint of the Domain struct when attempting to debug message signing.

The original code makes the assumption that setting a key in a map to nil results in a noop, however in Go the key is still added to the map with its value set to nil, which yields a value showing up in TypedData.Format() even though it may not be specified by the user's TypedData struct.

This PR fixes this, moving domain.ChainId from the map's initializer down to a separate if statement which checks the existance of ChainId's value, similar to the rest of the fields, before adding it. I've also included a new test to demonstrate the issue

@holiman
Copy link
Contributor

holiman commented Mar 19, 2019

Hm. But is it really valid without a domain ? Isn't that required?

@kumavis
Copy link
Member

kumavis commented Mar 20, 2019

@holiman not optional domain but optional domain.chainId

@stevenleeg
Copy link
Contributor Author

stevenleeg commented Mar 20, 2019

Yup, domains aren't any less required, this just makes it so chainId doesn't show up for users who chose to leave out that specific field (just as it does for the other fields).

Also not super sure what's going on with the CI– it appears my test is passing but something else is causing it to fail elsewhere?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants