Skip to content
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

Merged
merged 12 commits into from
Jun 26, 2024
2 changes: 2 additions & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,8 @@
"@metamask/json-rpc-engine": "^8.0.2",
"@metamask/rpc-errors": "^6.0.0",
"@metamask/utils": "^8.1.0",
"@types/bn.js": "^5.1.5",
"bn.js": "^5.2.1",
"klona": "^2.0.6",
"pify": "^5.0.0",
"safe-stable-stringify": "^2.4.3"
Expand Down
154 changes: 154 additions & 0 deletions src/utils/normalize.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,154 @@
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 normalize verifyingContract address in domain when provided data is an object', () => {
const NON_STRINGIFIED_MESSAGE_DATA_MOCK = MESSAGE_DATA_MOCK;
const normalizedData = JSON.parse(
normalizeTypedMessage(
NON_STRINGIFIED_MESSAGE_DATA_MOCK as unknown as string,
),
);
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 if verifyingContract is not parsable', () => {
const expectedVerifyingContract =
'Notparsableaddress1234567890123456789012345678901234567890';
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',
);
});
});
89 changes: 89 additions & 0 deletions src/utils/normalize.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,89 @@
import { add0x, isValidHexAddress, isStrictHexString } 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

View workflow job for this annotation

GitHub Actions / Build, lint, and test / Lint (16.x)

JSDoc description does not satisfy the regex pattern

Check warning on line 17 in src/utils/normalize.ts

View workflow job for this annotation

GitHub Actions / Build, lint, and test / Lint (18.x)

JSDoc description does not satisfy the regex pattern

Check warning on line 17 in src/utils/normalize.ts

View workflow job for this annotation

GitHub Actions / Build, lint, and test / Lint (20.x)

JSDoc description does not satisfy the regex pattern
*
* @param messageData - The messageData to normalize.
* @returns The normalized messageData.
*/
export function normalizeTypedMessage(messageData: string) {
let data;
try {
data = parseTypedMessage(messageData);
} 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 43 in src/utils/normalize.ts

View workflow job for this annotation

GitHub Actions / Build, lint, and test / Lint (16.x)

JSDoc description does not satisfy the regex pattern

Check warning on line 43 in src/utils/normalize.ts

View workflow job for this annotation

GitHub Actions / Build, lint, and test / Lint (18.x)

JSDoc description does not satisfy the regex pattern

Check warning on line 43 in src/utils/normalize.ts

View workflow job for this annotation

GitHub Actions / Build, lint, and test / Lint (20.x)

JSDoc description does not satisfy the regex pattern
*
* @param data - The messageData to parse.
* @returns The data object for EIP712 normalization.
*/
function parseTypedMessage(data: string) {
if (typeof data !== 'string') {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have a test for this?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in af2a8c2

return data;
}

return JSON.parse(data) as unknown as SignTypedMessageDataV3V4;
}

/**
* Normalizes the address to a hexadecimal format

Check warning on line 57 in src/utils/normalize.ts

View workflow job for this annotation

GitHub Actions / Build, lint, and test / Lint (16.x)

JSDoc description does not satisfy the regex pattern

Check warning on line 57 in src/utils/normalize.ts

View workflow job for this annotation

GitHub Actions / Build, lint, and test / Lint (18.x)

JSDoc description does not satisfy the regex pattern

Check warning on line 57 in src/utils/normalize.ts

View workflow job for this annotation

GitHub Actions / Build, lint, and test / Lint (20.x)

JSDoc description does not satisfy the regex pattern
*
* @param address - The address to normalize.
* @returns The normalized address.
*/
function normalizeContractAddress(address: string): Hex | string {
if (isStrictHexString(address) && isValidHexAddress(address)) {
return address;
}

// Check if the address is in octal format, convert to hexadecimal
if (address.startsWith('0o')) {
// If octal, convert to hexadecimal
return octalToHex(address);
}

// Check if the address is in decimal format, convert to hexadecimal
try {
const decimalBN = new BN(address, 10);
const hexString = decimalBN.toString(16);
return add0x(hexString);
} catch (e) {
// Ignore errors and return the original address
}

// Returning the original address without normalization
mcmire marked this conversation as resolved.
Show resolved Hide resolved
return address;
}

function octalToHex(octalAddress: string): Hex {
const decimalAddress = parseInt(octalAddress.slice(2), 8).toString(16);
return add0x(decimalAddress);
}
69 changes: 68 additions & 1 deletion src/wallet.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,12 @@ import { providerFromEngine } from '@metamask/eth-json-rpc-provider';
import { JsonRpcEngine } from '@metamask/json-rpc-engine';
import pify from 'pify';

import type { TransactionParams, MessageParams, TypedMessageV1Params } from '.';
import type {
MessageParams,
TransactionParams,
TypedMessageParams,
TypedMessageV1Params,
} from '.';
import { createWalletMiddleware } from '.';

const testAddresses = [
Expand Down Expand Up @@ -326,6 +331,68 @@ describe('wallet', () => {
});
});

describe('signTypedDataV3', () => {
it('should sign data and normalizes verifyingContract', async () => {
const { engine } = createTestSetup();
const getAccounts = async () => testAddresses.slice();
const witnessedMsgParams: TypedMessageParams[] = [];
const processTypedMessageV3 = async (msgParams: TypedMessageParams) => {
witnessedMsgParams.push(msgParams);
// Assume testMsgSig is the expected signature result
return testMsgSig;
};

engine.push(
createWalletMiddleware({ getAccounts, processTypedMessageV3 }),
);

const message = {
types: {
EIP712Domain: [
{ name: 'name', type: 'string' },
{ name: 'version', type: 'string' },
{ name: 'chainId', type: 'uint256' },
{ name: 'verifyingContract', type: 'address' },
],
},
primaryType: 'EIP712Domain',
domain: {
verifyingContract: '996101235222674412020337938588541139382869425796',
},
message: {},
};

const stringifiedMessage = JSON.stringify(message);
const expectedStringifiedMessage = JSON.stringify({
...message,
domain: {
verifyingContract: '0xae7ab96520de3a18e5e111b5eaab095312d7fe84',
},
});

const payload = {
method: 'eth_signTypedData_v3',
params: [testAddresses[0], stringifiedMessage], // Assuming testAddresses[0] is a valid address from your setup
};

const signTypedDataV3Response = await pify(engine.handle).call(
engine,
payload,
);
const signTypedDataV3Result = signTypedDataV3Response.result;

expect(signTypedDataV3Result).toBeDefined();
expect(signTypedDataV3Result).toStrictEqual(testMsgSig);
expect(witnessedMsgParams).toHaveLength(1);
expect(witnessedMsgParams[0]).toMatchObject({
from: testAddresses[0],
data: expectedStringifiedMessage,
version: 'V3',
signatureMethod: 'eth_signTypedData_v3',
});
});
});

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[suggestion] could include tests for:

  • octal addresses
  • inputs that are not parsable

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?


describe('sign', () => {
it('should sign with a valid address', async () => {
const { engine } = createTestSetup();
Expand Down
5 changes: 3 additions & 2 deletions src/wallet.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
} from '@metamask/utils';

import type { Block } from './types';
import { normalizeTypedMessage } from './utils/normalize';

/*
export type TransactionParams = {
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -306,7 +307,7 @@
const params = req.params as [string, string];

const address = await validateAndNormalizeKeyholder(params[0], req);
const message = params[1];
const message = normalizeTypedMessage(params[1]);
const version = 'V4';
const msgParams: TypedMessageParams = {
data: message,
Expand Down Expand Up @@ -458,7 +459,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 462 in src/wallet.ts

View workflow job for this annotation

GitHub Actions / Build, lint, and test / Lint (16.x)

There must be no hyphen before @returns description

Check warning on line 462 in src/wallet.ts

View workflow job for this annotation

GitHub Actions / Build, lint, and test / Lint (18.x)

There must be no hyphen before @returns description

Check warning on line 462 in src/wallet.ts

View workflow job for this annotation

GitHub Actions / Build, lint, and test / Lint (20.x)

There must be no hyphen before @returns description
* an error
*/
async function validateAndNormalizeKeyholder(
Expand Down
Loading
Loading