From 4048feeaac437f36745f996d3bb6e0400dbe6c12 Mon Sep 17 00:00:00 2001 From: Frederik Bolding Date: Fri, 17 Jun 2022 23:11:09 +0200 Subject: [PATCH] Increase likelyhood of valid method signatures being returned by `getMethodData` (#14937) * Increase likelyhood of valid method signatures being returned by getMethodData * Update coverage * Update coverage * Update coverage * add a migration to clear knownMethodData * Small typo changes Co-authored-by: Alex --- app/scripts/migrations/072.js | 30 ++ app/scripts/migrations/072.test.js | 427 +++++++++++++++++++++ app/scripts/migrations/index.js | 2 + jest.config.js | 4 +- ui/helpers/utils/transactions.util.js | 32 +- ui/helpers/utils/transactions.util.test.js | 46 +++ 6 files changed, 529 insertions(+), 12 deletions(-) create mode 100644 app/scripts/migrations/072.js create mode 100644 app/scripts/migrations/072.test.js diff --git a/app/scripts/migrations/072.js b/app/scripts/migrations/072.js new file mode 100644 index 000000000..4e5e8aebf --- /dev/null +++ b/app/scripts/migrations/072.js @@ -0,0 +1,30 @@ +import { cloneDeep } from 'lodash'; + +const version = 72; + +/** + * Should empty the `knownMethodData` object in PreferencesController + */ +export default { + version, + async migrate(originalVersionedData) { + const versionedData = cloneDeep(originalVersionedData); + versionedData.meta.version = version; + const state = versionedData.data; + const newState = transformState(state); + versionedData.data = newState; + return versionedData; + }, +}; + +function transformState(state) { + const PreferencesController = state?.PreferencesController || {}; + + return { + ...state, + PreferencesController: { + ...PreferencesController, + knownMethodData: {}, + }, + }; +} diff --git a/app/scripts/migrations/072.test.js b/app/scripts/migrations/072.test.js new file mode 100644 index 000000000..70fedab84 --- /dev/null +++ b/app/scripts/migrations/072.test.js @@ -0,0 +1,427 @@ +import migration72 from './072'; + +describe('migration #72', () => { + it('should update the version metadata', async () => { + const oldStorage = { + meta: { + version: 71, + }, + data: {}, + }; + + const newStorage = await migration72.migrate(oldStorage); + expect(newStorage.meta).toStrictEqual({ + version: 72, + }); + }); + + it('should empty knownMethodData object in PreferencesController', async () => { + const oldStorage = { + meta: { + version: 71, + }, + data: { + PreferencesController: { + knownMethodData: { + '0x095ea7b3': { + name: 'Approve', + params: [ + { + type: 'address', + }, + { + type: 'uint256', + }, + ], + }, + '0x1249c58b': { + name: 'Mint', + params: [], + }, + '0x1688f0b9': { + name: 'Create Proxy With Nonce', + params: [ + { + type: 'address', + }, + { + type: 'bytes', + }, + { + type: 'uint256', + }, + ], + }, + '0x18cbafe5': { + name: 'Swap Exact Tokens For E T H', + params: [ + { + type: 'uint256', + }, + { + type: 'uint256', + }, + { + type: 'address[]', + }, + { + type: 'address', + }, + { + type: 'uint256', + }, + ], + }, + '0x23b872dd': { + name: 'Transfer From', + params: [ + { + type: 'address', + }, + { + type: 'address', + }, + { + type: 'uint256', + }, + ], + }, + '0x2e1a7d4d': { + name: 'Withdraw', + params: [ + { + type: 'uint256', + }, + ], + }, + '0x2e7ba6ef': { + name: 'Claim', + params: [ + { + type: 'uint256', + }, + { + type: 'address', + }, + { + type: 'uint256', + }, + { + type: 'bytes32[]', + }, + ], + }, + '0x2eb2c2d6': { + name: 'Safe Batch Transfer From', + params: [ + { + type: 'address', + }, + { + type: 'address', + }, + { + type: 'uint256[]', + }, + { + type: 'uint256[]', + }, + { + type: 'bytes', + }, + ], + }, + '0x3671f8cf': {}, + '0x41441d3b': { + name: 'Enter Staking', + params: [ + { + type: 'uint256', + }, + ], + }, + '0x441a3e70': { + name: 'Withdraw', + params: [ + { + type: 'uint256', + }, + { + type: 'uint256', + }, + ], + }, + '0x6f652e1a': { + name: 'Create Order', + params: [ + { + type: 'address', + }, + { + type: 'uint256', + }, + { + type: 'uint256', + }, + { + type: 'uint256', + }, + ], + }, + '0x8dbdbe6d': { + name: 'Deposit', + params: [ + { + type: 'uint256', + }, + { + type: 'uint256', + }, + { + type: 'address', + }, + ], + }, + '0x8ed955b9': { + name: 'Harvest All', + params: [], + }, + '0xa22cb465': { + name: 'Set Approval For All', + params: [ + { + type: 'address', + }, + { + type: 'bool', + }, + ], + }, + '0xa9059cbb': { + name: 'Transfer', + params: [ + { + type: 'address', + }, + { + type: 'uint256', + }, + ], + }, + '0xab834bab': { + name: 'Atomic Match_', + params: [ + { + type: 'address[14]', + }, + { + type: 'uint256[18]', + }, + { + type: 'uint8[8]', + }, + { + type: 'bytes', + }, + { + type: 'bytes', + }, + { + type: 'bytes', + }, + { + type: 'bytes', + }, + { + type: 'bytes', + }, + { + type: 'bytes', + }, + { + type: 'uint8[2]', + }, + { + type: 'bytes32[5]', + }, + ], + }, + '0xd0e30db0': { + name: 'Deposit', + params: [], + }, + '0xddd81f82': { + name: 'Register Proxy', + params: [], + }, + '0xded9382a': { + name: 'Remove Liquidity E T H With Permit', + params: [ + { + type: 'address', + }, + { + type: 'uint256', + }, + { + type: 'uint256', + }, + { + type: 'uint256', + }, + { + type: 'address', + }, + { + type: 'uint256', + }, + { + type: 'bool', + }, + { + type: 'uint8', + }, + { + type: 'bytes32', + }, + { + type: 'bytes32', + }, + ], + }, + '0xe2bbb158': { + name: 'Deposit', + params: [ + { + type: 'uint256', + }, + { + type: 'uint256', + }, + ], + }, + '0xf305d719': { + name: 'Add Liquidity E T H', + params: [ + { + type: 'address', + }, + { + type: 'uint256', + }, + { + type: 'uint256', + }, + { + type: 'uint256', + }, + { + type: 'address', + }, + { + type: 'uint256', + }, + ], + }, + }, + }, + }, + }; + + const newStorage = await migration72.migrate(oldStorage); + expect(newStorage).toStrictEqual({ + meta: { + version: 72, + }, + data: { + PreferencesController: { + knownMethodData: {}, + }, + }, + }); + }); + + it('should preserve other PreferencesController state', async () => { + const oldStorage = { + meta: { + version: 71, + }, + data: { + PreferencesController: { + currentLocale: 'en', + dismissSeedBackUpReminder: false, + ipfsGateway: 'dweb.link', + knownMethodData: { + '0xd0e30db0': { + name: 'Deposit', + params: [], + }, + '0xddd81f82': { + name: 'Register Proxy', + params: [], + }, + }, + openSeaEnabled: false, + useTokenDetection: false, + }, + }, + }; + + const newStorage = await migration72.migrate(oldStorage); + expect(newStorage).toStrictEqual({ + meta: { + version: 72, + }, + data: { + PreferencesController: { + currentLocale: 'en', + dismissSeedBackUpReminder: false, + ipfsGateway: 'dweb.link', + knownMethodData: {}, + openSeaEnabled: false, + useTokenDetection: false, + }, + }, + }); + }); + + it('should not change state in controllers other than PreferencesController', async () => { + const oldStorage = { + meta: { + version: 71, + }, + data: { + PreferencesController: { + knownMethodData: { + '0xd0e30db0': { + name: 'Deposit', + params: [], + }, + '0xddd81f82': { + name: 'Register Proxy', + params: [], + }, + }, + }, + data: { + FooController: { a: 'b' }, + }, + }, + }; + + const newStorage = await migration72.migrate(oldStorage); + expect(newStorage).toStrictEqual({ + meta: { + version: 72, + }, + data: { + PreferencesController: { + knownMethodData: {}, + }, + data: { + FooController: { a: 'b' }, + }, + }, + }); + }); +}); diff --git a/app/scripts/migrations/index.js b/app/scripts/migrations/index.js index 321b91319..b2a52040d 100644 --- a/app/scripts/migrations/index.js +++ b/app/scripts/migrations/index.js @@ -75,6 +75,7 @@ import m068 from './068'; import m069 from './069'; import m070 from './070'; import m071 from './071'; +import m072 from './072'; const migrations = [ m002, @@ -147,6 +148,7 @@ const migrations = [ m069, m070, m071, + m072, ]; export default migrations; diff --git a/jest.config.js b/jest.config.js index d64ca0864..38a48471f 100644 --- a/jest.config.js +++ b/jest.config.js @@ -11,8 +11,8 @@ module.exports = { global: { branches: 44, functions: 42, - lines: 48, - statements: 48, + lines: 52, + statements: 52, }, './app/scripts/controllers/permissions/**/*.js': { branches: 100, diff --git a/ui/helpers/utils/transactions.util.js b/ui/helpers/utils/transactions.util.js index 25a38593f..d1380d8c0 100644 --- a/ui/helpers/utils/transactions.util.js +++ b/ui/helpers/utils/transactions.util.js @@ -35,12 +35,23 @@ async function getMethodFrom4Byte(fourBytePrefix) { mode: 'cors', }, ); - - if (fourByteResponse.count === 1) { - return fourByteResponse.results[0].text_signature; - } - return null; + fourByteResponse.results.sort((a, b) => { + return new Date(a.created_at).getTime() < new Date(b.created_at).getTime() + ? -1 + : 1; + }); + return fourByteResponse.results[0].text_signature; } + +function pickShortest(registrySig, fourByteSig) { + if (!registrySig) { + return fourByteSig; + } else if (!fourByteSig) { + return registrySig; + } + return fourByteSig.length < registrySig.length ? fourByteSig : registrySig; +} + let registry; /** @@ -51,7 +62,7 @@ let registry; */ export async function getMethodDataAsync(fourBytePrefix) { try { - const fourByteSig = getMethodFrom4Byte(fourBytePrefix).catch((e) => { + const fourByteSig = await getMethodFrom4Byte(fourBytePrefix).catch((e) => { log.error(e); return null; }); @@ -60,11 +71,12 @@ export async function getMethodDataAsync(fourBytePrefix) { registry = new MethodRegistry({ provider: global.ethereumProvider }); } - let sig = await registry.lookup(fourBytePrefix); + const registrySig = await registry.lookup(fourBytePrefix).catch((e) => { + log.error(e); + return null; + }); - if (!sig) { - sig = await fourByteSig; - } + const sig = pickShortest(registrySig, fourByteSig); if (!sig) { return {}; diff --git a/ui/helpers/utils/transactions.util.test.js b/ui/helpers/utils/transactions.util.test.js index 591e46d07..eaf0a963a 100644 --- a/ui/helpers/utils/transactions.util.test.js +++ b/ui/helpers/utils/transactions.util.test.js @@ -1,3 +1,5 @@ +import { HttpProvider } from 'ethjs'; +import nock from 'nock'; import { TRANSACTION_GROUP_STATUSES, TRANSACTION_STATUSES, @@ -55,4 +57,48 @@ describe('Transactions utils', () => { ).toStrictEqual(false); }); }); + + describe('getMethodDataAsync', () => { + global.ethereumProvider = new HttpProvider( + 'https://mainnet.infura.io/v3/341eacb578dd44a1a049cbc5f6fd4035', + ); + it('returns a valid signature for setApprovalForAll', async () => { + nock('https://www.4byte.directory:443', { encodedQueryParams: true }) + .get('/api/v1/signatures/') + .query({ hex_signature: '0xa22cb465' }) + .reply(200, { + count: 2, + next: null, + previous: null, + results: [ + { + id: 841519, + created_at: '2022-06-12T00:50:19.305588Z', + text_signature: 'niceFunctionHerePlzClick943230089(address,bool)', + hex_signature: '0xa22cb465', + bytes_signature: '¢,´e', + }, + { + id: 29659, + created_at: '2018-04-11T21:47:39.980645Z', + text_signature: 'setApprovalForAll(address,bool)', + hex_signature: '0xa22cb465', + bytes_signature: '¢,´e', + }, + ], + }); + nock('https://mainnet.infura.io:443', { encodedQueryParams: true }) + .post('/v3/341eacb578dd44a1a049cbc5f6fd4035') + .reply(200, (_, requestBody) => ({ + id: requestBody.id, + jsonrpc: '2.0', + result: + '0x0000000000000000000000000000000000000000000000000000000000000020000000000000000000000000000000000000000000000000000000000000002f6e69636546756e6374696f6e48657265506c7a436c69636b39343332333030383928616464726573732c626f6f6c290000000000000000000000000000000000', + })); + expect(await utils.getMethodDataAsync('0xa22cb465')).toStrictEqual({ + name: 'Set Approval For All', + params: [{ type: 'address' }, { type: 'bool' }], + }); + }); + }); });