From 32e6a94ed6a6a216041af93bb7c43c4c3bbcc9e0 Mon Sep 17 00:00:00 2001 From: Piotr Roslaniec Date: Fri, 16 Jun 2023 17:00:49 +0200 Subject: [PATCH 1/4] use ethers abi parser to validate function abi --- src/conditions/base/contract.ts | 49 +++++++++++++++------- test/unit/conditions/base/contract.test.ts | 24 +---------- test/unit/testVariables.ts | 2 +- 3 files changed, 38 insertions(+), 37 deletions(-) diff --git a/src/conditions/base/contract.ts b/src/conditions/base/contract.ts index 83a13b6ae..c7f153f31 100644 --- a/src/conditions/base/contract.ts +++ b/src/conditions/base/contract.ts @@ -1,3 +1,4 @@ +import { ethers } from 'ethers'; import Joi from 'joi'; import { ETH_ADDRESS_REGEXP } from '../const'; @@ -6,31 +7,51 @@ import { RpcCondition, rpcConditionRecord } from './rpc'; export const STANDARD_CONTRACT_TYPES = ['ERC20', 'ERC721']; -const functionAbiVariable = Joi.object({ - internalType: Joi.string(), // TODO is this needed? - name: Joi.string().required(), - type: Joi.string().required(), -}); - const functionAbiSchema = Joi.object({ name: Joi.string().required(), type: Joi.string().valid('function').required(), - inputs: Joi.array().items(functionAbiVariable), - outputs: Joi.array().items(functionAbiVariable), - // TODO: Should we restrict this to 'view'? - stateMutability: Joi.string(), + inputs: Joi.array(), + outputs: Joi.array(), + stateMutability: Joi.string().valid('view', 'pure').required(), }).custom((functionAbi, helper) => { + // Is `functionABI` a valid function fragment? + let asInterface; + try { + asInterface = new ethers.utils.Interface([functionAbi]); + } catch (e: unknown) { + const { message } = e as Error; + return helper.message({ + custom: message, + }); + } + + if (!asInterface.fragments) { + return helper.message({ + custom: '"functionAbi" is missing a function fragment', + }); + } + + if (asInterface.fragments.length > 1) { + return helper.message({ + custom: '"functionAbi" must contain exactly one function fragment', + }); + } + + // Now we just need to validate against the parent schema // Validate method name const method = helper.state.ancestors[0].method; - if (functionAbi.name !== method) { + const functionFragment = asInterface.fragments.filter( + (f) => f.name === method + )[0]; + if (!functionFragment) { return helper.message({ - custom: '"method" must be the same as "functionAbi.name"', + custom: '"functionAbi" does not contain the method specified as "method"', }); } // Validate nr of parameters const parameters = helper.state.ancestors[0].parameters; - if (functionAbi.inputs?.length !== parameters.length) { + if (functionFragment.inputs.length !== parameters.length) { return helper.message({ custom: '"parameters" must have the same length as "functionAbi.inputs"', }); @@ -39,7 +60,7 @@ const functionAbiSchema = Joi.object({ return functionAbi; }); -export const contractConditionRecord: Record = { +export const contractConditionRecord = { ...rpcConditionRecord, contractAddress: Joi.string().pattern(ETH_ADDRESS_REGEXP).required(), standardContractType: Joi.string() diff --git a/test/unit/conditions/base/contract.test.ts b/test/unit/conditions/base/contract.test.ts index 25fdddd8b..05074ddd1 100644 --- a/test/unit/conditions/base/contract.test.ts +++ b/test/unit/conditions/base/contract.test.ts @@ -7,7 +7,7 @@ import { import { ContractCondition } from '../../../../src/conditions/base'; import { USER_ADDRESS_PARAM } from '../../../../src/conditions/const'; import { fakeWeb3Provider } from '../../../utils'; -import { testContractConditionObj } from '../../testVariables'; +import { testContractConditionObj, testFunctionAbi } from '../../testVariables'; describe('validation', () => { it('accepts on a valid schema', () => { @@ -103,30 +103,10 @@ describe('accepts either standardContractType or functionAbi but not both or non }); describe('supports custom function abi', () => { - const fakeFunctionAbi = { - name: 'myFunction', - type: 'function', - inputs: [ - { - name: 'account', - type: 'address', - }, - { - name: 'myCustomParam', - type: 'uint256', - }, - ], - outputs: [ - { - name: 'someValue', - type: 'uint256', - }, - ], - }; const contractConditionObj = { ...testContractConditionObj, standardContractType: undefined, - functionAbi: fakeFunctionAbi, + functionAbi: testFunctionAbi, method: 'myFunction', parameters: [USER_ADDRESS_PARAM, ':customParam'], returnValueTest: { diff --git a/test/unit/testVariables.ts b/test/unit/testVariables.ts index ef943513a..36031e816 100644 --- a/test/unit/testVariables.ts +++ b/test/unit/testVariables.ts @@ -48,6 +48,7 @@ export const testContractConditionObj = { export const testFunctionAbi = { name: 'myFunction', type: 'function', + stateMutability: 'view', inputs: [ { internalType: 'address', @@ -67,5 +68,4 @@ export const testFunctionAbi = { type: 'uint256', }, ], - stateMutability: 'view', }; From 37a2b72590376baa2993bf525e605a4a666d782a Mon Sep 17 00:00:00 2001 From: Piotr Roslaniec Date: Wed, 28 Jun 2023 15:33:38 +0200 Subject: [PATCH 2/4] apply pr suggestions --- src/conditions/base/contract.ts | 15 +-- test/unit/conditions/base/contract.test.ts | 107 ++++++++++++++++++++- 2 files changed, 115 insertions(+), 7 deletions(-) diff --git a/src/conditions/base/contract.ts b/src/conditions/base/contract.ts index c7f153f31..522591b74 100644 --- a/src/conditions/base/contract.ts +++ b/src/conditions/base/contract.ts @@ -25,13 +25,13 @@ const functionAbiSchema = Joi.object({ }); } - if (!asInterface.fragments) { + if (!asInterface.functions) { return helper.message({ custom: '"functionAbi" is missing a function fragment', }); } - if (asInterface.fragments.length > 1) { + if (Object.values(asInterface.functions).length !== 1) { return helper.message({ custom: '"functionAbi" must contain exactly one function fragment', }); @@ -40,12 +40,15 @@ const functionAbiSchema = Joi.object({ // Now we just need to validate against the parent schema // Validate method name const method = helper.state.ancestors[0].method; - const functionFragment = asInterface.fragments.filter( - (f) => f.name === method - )[0]; + const abiMethodName = Object.keys(asInterface.functions).find((name) => + name.startsWith(`${method}(`) + ); + const functionFragment = abiMethodName + ? asInterface.functions[abiMethodName] + : null; if (!functionFragment) { return helper.message({ - custom: '"functionAbi" does not contain the method specified as "method"', + custom: `"functionAbi" not valid for method: "${method}"`, }); } diff --git a/test/unit/conditions/base/contract.test.ts b/test/unit/conditions/base/contract.test.ts index 05074ddd1..ee988bbb6 100644 --- a/test/unit/conditions/base/contract.test.ts +++ b/test/unit/conditions/base/contract.test.ts @@ -123,7 +123,7 @@ describe('supports custom function abi', () => { const customParams: Record = {}; customParams[myCustomParam] = 1234; - it('accepts custom function abi', async () => { + it('accepts custom function abi with a custom parameter', async () => { const asJson = await conditionContext .withCustomParams(customParams) .toJson(); @@ -131,4 +131,109 @@ describe('supports custom function abi', () => { expect(asJson).toContain(USER_ADDRESS_PARAM); expect(asJson).toContain(myCustomParam); }); + + it.each([ + { + method: 'balanceOf', + functionAbi: { + name: 'balanceOf', + type: 'function', + inputs: [{ name: '_owner', type: 'address' }], + outputs: [{ name: 'balance', type: 'uint256' }], + stateMutability: 'view', + }, + }, + { + method: 'get', + functionAbi: { + name: 'get', + type: 'function', + inputs: [], + outputs: [], + stateMutability: 'pure', + }, + }, + ])('accepts well-formed functionAbi', ({ method, functionAbi }) => { + expect(() => + new ContractCondition({ + ...contractConditionObj, + parameters: functionAbi.inputs.map( + (input) => `fake_parameter_${input}` + ), // + functionAbi, + method, + }).toObj() + ).not.toThrow(); + }); + + it.each([ + { + method: '1234', + expectedError: '"functionAbi.name" must be a string', + functionAbi: { + name: 1234, // invalid value + type: 'function', + inputs: [{ name: '_owner', type: 'address' }], + outputs: [{ name: 'balance', type: 'uint256' }], + stateMutability: 'view', + }, + }, + { + method: 'transfer', + expectedError: '"functionAbi.inputs" must be an array', + functionAbi: { + name: 'transfer', + type: 'function', + inputs: 'invalid value', // invalid value + outputs: [{ name: '_status', type: 'bool' }], + stateMutability: 'pure', + }, + }, + { + method: 'get', + expectedError: + '"functionAbi.stateMutability" must be one of [view, pure]', + functionAbi: { + name: 'get', + type: 'function', + inputs: [], + outputs: [], + stateMutability: 'invalid', // invalid value + }, + }, + { + method: 'test', + expectedError: '"functionAbi.outputs" must be an array', + functionAbi: { + name: 'test', + type: 'function', + inputs: [], + outputs: 'invalid value', // Invalid value + stateMutability: 'pure', + }, + }, + { + method: 'calculatePow', + expectedError: + 'Invalid condition: "parameters" must have the same length as "functionAbi.inputs"', + functionAbi: { + name: 'calculatePow', + type: 'function', + // 'inputs': [] // Missing inputs array + outputs: [{ name: 'result', type: 'uint256' }], + stateMutability: 'view', + }, + }, + ])( + 'rejects malformed functionAbi', + ({ method, expectedError, functionAbi }) => { + expect(() => + new ContractCondition({ + ...contractConditionObj, + functionAbi, + method, + }).toObj() + ).toThrow(expectedError); + } + ); }); From 162e2baf2d9cfadc0108c3b6bf9cde45e6f5a0b8 Mon Sep 17 00:00:00 2001 From: Piotr Roslaniec Date: Wed, 28 Jun 2023 18:14:24 +0200 Subject: [PATCH 3/4] apply pr suggestions --- src/conditions/base/contract.ts | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/src/conditions/base/contract.ts b/src/conditions/base/contract.ts index 522591b74..7f67d9be3 100644 --- a/src/conditions/base/contract.ts +++ b/src/conditions/base/contract.ts @@ -40,12 +40,16 @@ const functionAbiSchema = Joi.object({ // Now we just need to validate against the parent schema // Validate method name const method = helper.state.ancestors[0].method; - const abiMethodName = Object.keys(asInterface.functions).find((name) => - name.startsWith(`${method}(`) - ); - const functionFragment = abiMethodName - ? asInterface.functions[abiMethodName] - : null; + + let functionFragment; + try { + functionFragment = asInterface.getFunction(method); + } catch (e) { + return helper.message({ + custom: `"functionAbi" contains ambiguous "${method}"`, + }); + } + if (!functionFragment) { return helper.message({ custom: `"functionAbi" not valid for method: "${method}"`, From 5bd27b44e30d942daa9b7430a646133f8a6db4d4 Mon Sep 17 00:00:00 2001 From: piotr-roslaniec <39299780+piotr-roslaniec@users.noreply.github.com> Date: Wed, 28 Jun 2023 18:27:04 +0200 Subject: [PATCH 4/4] Update src/conditions/base/contract.ts Co-authored-by: Derek Pierre --- src/conditions/base/contract.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/conditions/base/contract.ts b/src/conditions/base/contract.ts index 7f67d9be3..c4221e1da 100644 --- a/src/conditions/base/contract.ts +++ b/src/conditions/base/contract.ts @@ -46,7 +46,7 @@ const functionAbiSchema = Joi.object({ functionFragment = asInterface.getFunction(method); } catch (e) { return helper.message({ - custom: `"functionAbi" contains ambiguous "${method}"`, + custom: `"functionAbi" has no matching function for "${method}"`, }); }