-
-
Notifications
You must be signed in to change notification settings - Fork 252
Eth sign typed data #36
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
Conversation
Codecov Report
@@ Coverage Diff @@
## master #36 +/- ##
=====================================
Coverage 100% 100%
=====================================
Files 18 20 +2
Lines 932 998 +66
Branches 103 114 +11
=====================================
+ Hits 932 998 +66
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great. Just a couple of questions (non blockers)!
switch (version) { | ||
case 'V1': | ||
return sigUtil.signTypedDataLegacy(privateKeyBuffer, { data: messageParams.data }); | ||
case 'V3': |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we make version optional and default to v3?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
type: 'uint32', | ||
value: '1337' | ||
} | ||
]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it worth to add a test for a message with data that has >= 2 levels deep ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Honestly, I could have put any compatible data there, would we want to change it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just making sure that works for complex objects
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The way I understand it, these managers only have the responsibility to handle messages flow and not the data itself, that would be a responsibility for eth-sig-util
in this case. But I can be wrong and we could add more complex data structures extending from object[]
. For that reason there are also tests for eth_signTypedData_v3
with typed messages validation according to the EIP712.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good so far. I think PersonalMessageManager
and TypedMessageManager
should extend from a common base class since so much functionality is identical. Right now, we're duplicating a lot of underlying code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really great refactoring. This is definitely good to merge.
* Bump Yarn to v3 and patch jest-worker * Fix lint issues * Fix CI config
* TypedMessageManager with signTypedMessageV3 data type validation missing * add signTypedMessageV3 data validation tests * rename util methods for sign typed data * update doc * add signTypedMessage * const naming changes * update test * signTypedData working as expected * use data object to signTypedData V3 * throw error when signTypedData fails * improve doc * reuse const typedMessage on typedMessageManager tests * refactor PersonalMessageManager using base MessageManager * refactor TypedMessageManager using base MessageManager * add MessageManager tests * fiz util import * fix TypedMessageParams on keyring controller * update docs
* TypedMessageManager with signTypedMessageV3 data type validation missing * add signTypedMessageV3 data validation tests * rename util methods for sign typed data * update doc * add signTypedMessage * const naming changes * update test * signTypedData working as expected * use data object to signTypedData V3 * throw error when signTypedData fails * improve doc * reuse const typedMessage on typedMessageManager tests * refactor PersonalMessageManager using base MessageManager * refactor TypedMessageManager using base MessageManager * add MessageManager tests * fiz util import * fix TypedMessageParams on keyring controller * update docs
* Add files field to package.json Co-authored-by: Mark Stacey <markjstacey@gmail.com>
This PR is mostly a migration to ts of this to have both
eth_signTypedData
andeth_signTypedData_v3
as proposed by EIP712It also exposes signing methods from eth-sig-util