-
-
Notifications
You must be signed in to change notification settings - Fork 90
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
Add verifyContract
address normalization
#309
Changes from 2 commits
c0920f5
06c86e2
d9fc14d
2ebe685
af2a8c2
a8e8e71
ba7ec73
5398f03
4baeb92
7c2e141
8da62e9
aa90200
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,124 @@ | ||
import { normalizeTypedMessage } from './normalize'; | ||
|
||
const MESSAGE_DATA_MOCK = { | ||
types: { | ||
Permit: [ | ||
{ | ||
name: 'owner', | ||
type: 'address', | ||
}, | ||
{ | ||
name: 'spender', | ||
type: 'address', | ||
}, | ||
{ | ||
name: 'value', | ||
type: 'uint256', | ||
}, | ||
{ | ||
name: 'nonce', | ||
type: 'uint256', | ||
}, | ||
{ | ||
name: 'deadline', | ||
type: 'uint256', | ||
}, | ||
], | ||
EIP712Domain: [ | ||
{ | ||
name: 'name', | ||
type: 'string', | ||
}, | ||
{ | ||
name: 'version', | ||
type: 'string', | ||
}, | ||
{ | ||
name: 'chainId', | ||
type: 'uint256', | ||
}, | ||
{ | ||
name: 'verifyingContract', | ||
type: 'address', | ||
}, | ||
], | ||
}, | ||
domain: { | ||
name: 'Liquid staked Ether 2.0', | ||
version: '2', | ||
chainId: '0x1', | ||
verifyingContract: '996101235222674412020337938588541139382869425796', | ||
}, | ||
primaryType: 'Permit', | ||
message: { | ||
owner: '0x6d404afe1a6a07aa3cbcbf9fd027671df628ebfc', | ||
spender: '0x63605E53D422C4F1ac0e01390AC59aAf84C44A51', | ||
value: | ||
'115792089237316195423570985008687907853269984665640564039457584007913129639935', | ||
nonce: '0', | ||
deadline: '4482689033', | ||
}, | ||
}; | ||
|
||
describe('normalizeTypedMessage', () => { | ||
function parseNormalizerResult(data: Record<string, unknown>) { | ||
return JSON.parse(normalizeTypedMessage(JSON.stringify(data))); | ||
} | ||
|
||
it('should normalize verifyingContract address in domain', () => { | ||
const normalizedData = parseNormalizerResult(MESSAGE_DATA_MOCK); | ||
expect(normalizedData.domain.verifyingContract).toBe( | ||
'0xae7ab96520de3a18e5e111b5eaab095312d7fe84', | ||
); | ||
}); | ||
|
||
it('should handle octal verifyingContract address by normalizing it', () => { | ||
const expectedNormalizedOctalAddress = '0x53'; | ||
const messageDataWithOctalAddress = { | ||
...MESSAGE_DATA_MOCK, | ||
domain: { | ||
...MESSAGE_DATA_MOCK.domain, | ||
verifyingContract: '0o123', | ||
}, | ||
}; | ||
|
||
const normalizedData = parseNormalizerResult(messageDataWithOctalAddress); | ||
|
||
expect(normalizedData.domain.verifyingContract).toBe( | ||
expectedNormalizedOctalAddress, | ||
); | ||
}); | ||
|
||
it('should not modify if verifyingContract is already hexadecimal', () => { | ||
const expectedVerifyingContract = | ||
'0xae7ab96520de3a18e5e111b5eaab095312d7fe84'; | ||
const messageDataWithHexAddress = { | ||
...MESSAGE_DATA_MOCK, | ||
domain: { | ||
...MESSAGE_DATA_MOCK.domain, | ||
verifyingContract: expectedVerifyingContract, | ||
}, | ||
}; | ||
|
||
const normalizedData = parseNormalizerResult(messageDataWithHexAddress); | ||
|
||
expect(normalizedData.domain.verifyingContract).toBe( | ||
expectedVerifyingContract, | ||
); | ||
}); | ||
|
||
it('should not modify other parts of the message data', () => { | ||
const normalizedData = parseNormalizerResult(MESSAGE_DATA_MOCK); | ||
expect(normalizedData.message).toStrictEqual(MESSAGE_DATA_MOCK.message); | ||
expect(normalizedData.types).toStrictEqual(MESSAGE_DATA_MOCK.types); | ||
expect(normalizedData.primaryType).toStrictEqual( | ||
MESSAGE_DATA_MOCK.primaryType, | ||
); | ||
}); | ||
|
||
it('should return data as is if not parsable', () => { | ||
expect(normalizeTypedMessage('Not parsable data')).toBe( | ||
'Not parsable data', | ||
); | ||
}); | ||
}); |
Original file line number | Diff line number | Diff line change | ||||||
---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,94 @@ | ||||||||
import { add0x, isValidHexAddress } from '@metamask/utils'; | ||||||||
import type { Hex } from '@metamask/utils'; | ||||||||
import BN from 'bn.js'; | ||||||||
|
||||||||
type EIP712Domain = { | ||||||||
verifyingContract: string; | ||||||||
}; | ||||||||
|
||||||||
type SignTypedMessageDataV3V4 = { | ||||||||
types: Record<string, unknown>; | ||||||||
domain: EIP712Domain; | ||||||||
primaryType: string; | ||||||||
message: unknown; | ||||||||
}; | ||||||||
|
||||||||
/** | ||||||||
* Normalizes the messageData for the eth_signTypedData | ||||||||
Check warning on line 17 in src/utils/normalize.ts GitHub Actions / Build, lint, and test / Lint (16.x)
Check warning on line 17 in src/utils/normalize.ts GitHub Actions / Build, lint, and test / Lint (18.x)
|
||||||||
* | ||||||||
* @param messageData - The messageData to normalize. | ||||||||
* @returns The normalized messageData. | ||||||||
*/ | ||||||||
export function normalizeTypedMessage(messageData: string) { | ||||||||
let data; | ||||||||
try { | ||||||||
data = parseTypedMessage( | ||||||||
messageData, | ||||||||
) as unknown as SignTypedMessageDataV3V4; | ||||||||
} catch (e) { | ||||||||
// Ignore normalization errors and pass the message as is | ||||||||
return messageData; | ||||||||
} | ||||||||
|
||||||||
const { verifyingContract } = data.domain ?? {}; | ||||||||
digiwand marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||
|
||||||||
if (!verifyingContract) { | ||||||||
return messageData; | ||||||||
} | ||||||||
|
||||||||
data.domain.verifyingContract = normalizeContractAddress(verifyingContract); | ||||||||
|
||||||||
return JSON.stringify(data); | ||||||||
} | ||||||||
|
||||||||
/** | ||||||||
* Parses the messageData to obtain the data object for EIP712 normalization | ||||||||
Check warning on line 45 in src/utils/normalize.ts GitHub Actions / Build, lint, and test / Lint (16.x)
Check warning on line 45 in src/utils/normalize.ts GitHub Actions / Build, lint, and test / Lint (18.x)
|
||||||||
* | ||||||||
* @param data - The messageData to parse. | ||||||||
* @returns The data object for EIP712 normalization. | ||||||||
*/ | ||||||||
function parseTypedMessage(data: string) { | ||||||||
if (typeof data !== 'string') { | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we have a test for this? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done in af2a8c2 |
||||||||
return data; | ||||||||
} | ||||||||
|
||||||||
try { | ||||||||
return JSON.parse(data); | ||||||||
} catch (e) { | ||||||||
throw new Error(`Invalid message data for normalization. data: ${data}`); | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there value in throwing a custom error here since we catch it above anyway? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Removed in 4baeb92 |
||||||||
} | ||||||||
} | ||||||||
|
||||||||
/** | ||||||||
* Normalizes the address to a hexadecimal format | ||||||||
Check warning on line 63 in src/utils/normalize.ts GitHub Actions / Build, lint, and test / Lint (16.x)
Check warning on line 63 in src/utils/normalize.ts GitHub Actions / Build, lint, and test / Lint (18.x)
|
||||||||
* | ||||||||
* @param address - The address to normalize. | ||||||||
* @returns The normalized address. | ||||||||
*/ | ||||||||
function normalizeContractAddress(address: string): Hex { | ||||||||
const addressHex = address as Hex; | ||||||||
if (isValidHexAddress(addressHex)) { | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It appears that we need to use a type assertion because
Suggested change
That means we shouldn't need to use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good call, done in a8e8e71 |
||||||||
return addressHex; | ||||||||
} | ||||||||
|
||||||||
// Check if the address is in octal format, convert to hexadecimal | ||||||||
if (addressHex.startsWith('0o')) { | ||||||||
// If octal, convert to hexadecimal | ||||||||
return octalToHex(addressHex as string); | ||||||||
} | ||||||||
|
||||||||
// Check if the address is in decimal format, convert to hexadecimal | ||||||||
const parsedAddress = parseInt(addressHex, 10); | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we need to be worried that this will be a large number? I see that you use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done in ba7ec73 |
||||||||
if (!isNaN(parsedAddress)) { | ||||||||
const hexString = new BN(addressHex.toString(), 10).toString(16); | ||||||||
return add0x(hexString); | ||||||||
} | ||||||||
|
||||||||
// Returning the original address without normalization | ||||||||
mcmire marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||
return addressHex; | ||||||||
} | ||||||||
|
||||||||
function octalToHex(octalAddress: string): Hex { | ||||||||
const decimalAddress = parseInt(octalAddress.slice(2), 8).toString(16); | ||||||||
return add0x(decimalAddress); | ||||||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -12,6 +12,7 @@ | |
} from '@metamask/utils'; | ||
|
||
import type { Block } from './types'; | ||
import { normalizeTypedMessage } from './utils/normalize'; | ||
|
||
/* | ||
export type TransactionParams = { | ||
|
@@ -276,7 +277,7 @@ | |
const params = req.params as [string, string]; | ||
|
||
const address = await validateAndNormalizeKeyholder(params[0], req); | ||
const message = params[1]; | ||
const message = normalizeTypedMessage(params[1]); | ||
mcmire marked this conversation as resolved.
Show resolved
Hide resolved
|
||
const version = 'V3'; | ||
const msgParams: TypedMessageParams = { | ||
data: message, | ||
|
@@ -306,7 +307,12 @@ | |
const params = req.params as [string, string]; | ||
|
||
const address = await validateAndNormalizeKeyholder(params[0], req); | ||
const message = params[1]; | ||
let message = params[1]; | ||
try { | ||
message = normalizeTypedMessage(message); | ||
} catch (e) { | ||
// Ignore normalization errors and pass the message as is | ||
} | ||
const version = 'V4'; | ||
const msgParams: TypedMessageParams = { | ||
data: message, | ||
|
@@ -458,7 +464,7 @@ | |
* | ||
* @param address - The address to validate and normalize. | ||
* @param req - The request object. | ||
* @returns {string} - The normalized address, if valid. Otherwise, throws | ||
Check warning on line 467 in src/wallet.ts GitHub Actions / Build, lint, and test / Lint (16.x)
Check warning on line 467 in src/wallet.ts GitHub Actions / Build, lint, and test / Lint (18.x)
Check warning on line 467 in src/wallet.ts GitHub Actions / Build, lint, and test / Lint (20.x)
|
||
* an error | ||
*/ | ||
async function validateAndNormalizeKeyholder( | ||
|
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.
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 theJSON.parse
. What are your thoughts on putting this type assertion in that function instead so it's more associated with theJSON.parse
visually?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.
That make sense @mcmire, fixed in 2ebe685