Add verifyContract address normalization
#309
Conversation
|
New dependencies detected. Learn more about Socket for GitHub ↗︎
|
|
@metamaskbot publish-preview |
src/utils/normalize.ts
Outdated
| try { | ||
| data = parseTypedMessage( | ||
| messageData, | ||
| ) as unknown as SignTypedMessageDataV3V4; |
There was a problem hiding this comment.
When using a type assertion it's good to know why it's necessary. After looking at parseTypedMessage I can see it's because of the JSON.parse. What are your thoughts on putting this type assertion in that function instead so it's more associated with the JSON.parse visually?
| * @returns The data object for EIP712 normalization. | ||
| */ | ||
| function parseTypedMessage(data: string) { | ||
| if (typeof data !== 'string') { |
There was a problem hiding this comment.
Do we have a test for this?
src/utils/normalize.ts
Outdated
| const addressHex = address as Hex; | ||
| if (isValidHexAddress(addressHex)) { |
There was a problem hiding this comment.
It appears that we need to use a type assertion because isValidHexAddress takes a Hex. However, we should be able to avoid the type assertion by using isStrictHexString, which narrows its argument to a Hex if given a hex string:
| const addressHex = address as Hex; | |
| if (isValidHexAddress(addressHex)) { | |
| if (isStrictHexString(address) && isValidHexAddress(address)) { |
That means we shouldn't need to use addressHex below, or upcast it to a string.
There was a problem hiding this comment.
Good call, done in a8e8e71
src/utils/normalize.ts
Outdated
| } | ||
|
|
||
| // Check if the address is in decimal format, convert to hexadecimal | ||
| const parsedAddress = parseInt(addressHex, 10); |
There was a problem hiding this comment.
Do we need to be worried that this will be a large number? I see that you use BN below, but you seem to only use it for conversion (and then, only for decimal -> hex and not octal -> hex).
src/utils/normalize.ts
Outdated
| try { | ||
| return JSON.parse(data); | ||
| } catch (e) { | ||
| throw new Error(`Invalid message data for normalization. data: ${data}`); |
There was a problem hiding this comment.
Is there value in throwing a custom error here since we catch it above anyway?
| signatureMethod: 'eth_signTypedData_v3', | ||
| }); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
[suggestion] could include tests for:
- octal addresses
- inputs that are not parsable
There was a problem hiding this comment.
As mentioned above (#309 (comment)), these test cases are already covered by the normalizer unit tests. I added this single eth_signTypedData_v3 test because it was missing from the wallet tests. Is there any advantage to adding essentially the same tests here as well?
Explanation
This PR aims to add address normalization for
EIP712Domain.verifyContractReferences
Please see https://github.com/MetaMask/MetaMask-planning/issues/2229
Blocked by