Validate `functionAbi` using `ethers` ABI parser #228

piotr-roslaniec · 2023-06-16T15:21:49Z

Type of PR:

Feature

Required reviews:

1

What this does:

Uses ethers to validate functionABI

Issues fixed/closed:

Fixes #...

Why it's needed:

We've implemented somewhat basic validation for functionABI, but it's still possible to pass mangled/illegal JSON ABI into your conditions

Notes for reviewers:

I need some input on this - It looks like there is going to be some overlap between Joi and ethers, and we have to implement some checks and balances ourselves on top of that.

codecov-commenter · 2023-06-28T08:24:40Z

Codecov Report

Merging #228 (5bd27b4) into tdec-epic (50dd01c) will decrease coverage by 0.23%.
The diff coverage is 66.66%.

@@              Coverage Diff              @@
##           tdec-epic     #228      +/-   ##
=============================================
- Coverage      83.47%   83.24%   -0.23%     
=============================================
  Files             37       37              
  Lines            968      979      +11     
  Branches         121      123       +2     
=============================================
+ Hits             808      815       +7     
- Misses           154      158       +4     
  Partials           6        6

Impacted Files	Coverage Δ
src/conditions/base/contract.ts	`79.31% <66.66%> (-9.58%)`	⬇️

github-actions · 2023-06-28T08:25:28Z

Bundled size for the package is listed below:

build/module/types/ethers-contracts/factories: 82.03 KB
build/module/types/ethers-contracts: 152.34 KB
build/module/types: 156.25 KB
build/module/test: 42.97 KB
build/module/src/sdk/strategy: 31.25 KB
build/module/src/sdk: 46.88 KB
build/module/src/characters: 89.84 KB
build/module/src/conditions/context: 42.97 KB
build/module/src/conditions/predefined: 19.53 KB
build/module/src/conditions/base: 54.69 KB
build/module/src/conditions: 156.25 KB
build/module/src/agents: 35.16 KB
build/module/src/policies: 19.53 KB
build/module/src/kits: 19.53 KB
build/module/src: 433.59 KB
build/module: 687.50 KB
build/main/types/ethers-contracts/factories: 82.03 KB
build/main/types/ethers-contracts: 152.34 KB
build/main/types: 156.25 KB
build/main/test: 46.88 KB
build/main/src/sdk/strategy: 31.25 KB
build/main/src/sdk: 46.88 KB
build/main/src/characters: 89.84 KB
build/main/src/conditions/context: 42.97 KB
build/main/src/conditions/predefined: 19.53 KB
build/main/src/conditions/base: 54.69 KB
build/main/src/conditions: 156.25 KB
build/main/src/agents: 35.16 KB
build/main/src/policies: 19.53 KB
build/main/src/kits: 19.53 KB
build/main/src: 437.50 KB
build/main: 695.31 KB
build: 1.35 MB

theref

Great change

derekpierre · 2023-06-28T12:13:27Z

src/conditions/base/contract.ts

+    });
+  }
+
+  if (!asInterface.fragments) {


Should we be using .functions instead of .fragments? Just reading from here - https://docs.ethers.org/v5/api/utils/abi/interface/#Interface--properties.

Same for various logic below.

I initially avoided that since the keys in .functions are function signatures, not function names.

Where are the key values used? L44?

Yes, we use them to fetch the FunctionFragments

derekpierre · 2023-06-28T12:16:06Z

src/conditions/base/contract.ts

    return helper.message({
-      custom: '"method" must be the same as "functionAbi.name"',
+      custom: '"functionAbi" does not contain the method specified as "method"',


Can we output the actual method name. So something like (?):

Suggested change

custom: '"functionAbi" does not contain the method specified as "method"',

custom: `"functionAbi" not valid for method, "${method}"`,

derekpierre · 2023-06-28T12:17:28Z

src/conditions/base/contract.ts

    });
  }

  // Validate nr of parameters
  const parameters = helper.state.ancestors[0].parameters;
-  if (functionAbi.inputs?.length !== parameters.length) {
+  if (functionFragment.inputs.length !== parameters.length) {


derekpierre · 2023-06-28T12:20:28Z

test/unit/conditions/base/contract.test.ts

@@ -7,7 +7,7 @@ import {
 import { ContractCondition } from '../../../../src/conditions/base';


I'd love to see some comprehensive testing around validation of many different functionAbis, both valid and invalid, and correct processing/failure as part of this PR.

derekpierre

See comment about testing.

github-actions · 2023-06-28T13:35:34Z

Bundled size for the package is listed below:

build/module/types/ethers-contracts/factories: 82.03 KB
build/module/types/ethers-contracts: 152.34 KB
build/module/types: 156.25 KB
build/module/test: 42.97 KB
build/module/src/sdk/strategy: 31.25 KB
build/module/src/sdk: 46.88 KB
build/module/src/characters: 89.84 KB
build/module/src/conditions/context: 42.97 KB
build/module/src/conditions/predefined: 19.53 KB
build/module/src/conditions/base: 54.69 KB
build/module/src/conditions: 156.25 KB
build/module/src/agents: 35.16 KB
build/module/src/policies: 19.53 KB
build/module/src/kits: 19.53 KB
build/module/src: 433.59 KB
build/module: 687.50 KB
build/main/types/ethers-contracts/factories: 82.03 KB
build/main/types/ethers-contracts: 152.34 KB
build/main/types: 156.25 KB
build/main/test: 46.88 KB
build/main/src/sdk/strategy: 31.25 KB
build/main/src/sdk: 46.88 KB
build/main/src/characters: 89.84 KB
build/main/src/conditions/context: 42.97 KB
build/main/src/conditions/predefined: 19.53 KB
build/main/src/conditions/base: 54.69 KB
build/main/src/conditions: 156.25 KB
build/main/src/agents: 35.16 KB
build/main/src/policies: 19.53 KB
build/main/src/kits: 19.53 KB
build/main/src: 437.50 KB
build/main: 695.31 KB
build: 1.35 MB

github-actions · 2023-06-28T13:36:57Z

Bundled size for the package is listed below:

build/module/types/ethers-contracts/factories: 82.03 KB
build/module/types/ethers-contracts: 152.34 KB
build/module/types: 156.25 KB
build/module/test: 42.97 KB
build/module/src/sdk/strategy: 31.25 KB
build/module/src/sdk: 46.88 KB
build/module/src/characters: 89.84 KB
build/module/src/conditions/context: 42.97 KB
build/module/src/conditions/predefined: 19.53 KB
build/module/src/conditions/base: 54.69 KB
build/module/src/conditions: 156.25 KB
build/module/src/agents: 35.16 KB
build/module/src/policies: 19.53 KB
build/module/src/kits: 19.53 KB
build/module/src: 433.59 KB
build/module: 687.50 KB
build/main/types/ethers-contracts/factories: 82.03 KB
build/main/types/ethers-contracts: 152.34 KB
build/main/types: 156.25 KB
build/main/test: 46.88 KB
build/main/src/sdk/strategy: 31.25 KB
build/main/src/sdk: 46.88 KB
build/main/src/characters: 89.84 KB
build/main/src/conditions/context: 42.97 KB
build/main/src/conditions/predefined: 19.53 KB
build/main/src/conditions/base: 54.69 KB
build/main/src/conditions: 156.25 KB
build/main/src/agents: 35.16 KB
build/main/src/policies: 19.53 KB
build/main/src/kits: 19.53 KB
build/main/src: 437.50 KB
build/main: 695.31 KB
build: 1.35 MB

derekpierre · 2023-06-28T14:02:24Z

test/unit/conditions/base/contract.test.ts

+  ])(
+    'rejects malformed functionAbi',
+    ({ method, expectedError, functionAbi }) => {
+      expect(() =>


Love this! 😍

derekpierre · 2023-06-28T14:15:31Z

src/conditions/base/contract.ts

+  const abiMethodName = Object.keys(asInterface.functions).find((name) =>
+    name.startsWith(`${method}(`)
+  );
+  const functionFragment = abiMethodName
+    ? asInterface.functions[abiMethodName]
+    : null;


Could this be reduced to the following (https://docs.ethers.org/v5/api/utils/abi/interface/#Interface--fragments)?

Suggested change

const abiMethodName = Object.keys(asInterface.functions).find((name) =>

name.startsWith(`${method}(`)

);

const functionFragment = abiMethodName

? asInterface.functions[abiMethodName]

: null;

const functionFragment = asInterface.getFunction(method);

It seems to possibly throw an ArgumentError, https://github.com/ethers-io/ethers.js/blob/master/packages/abi/src.ts/interface.ts#L193, but do we care?

I think it makes sense given our conversation about the ambiguity of functions. Fixed.

src/conditions/base/contract.ts

derekpierre

🎸 - good stuff. Minor nitpick suggestion.

Co-authored-by: Derek Pierre <derek.pierre@gmail.com>

github-actions · 2023-06-28T16:29:18Z

Bundled size for the package is listed below:

build/module/types/ethers-contracts/factories: 82.03 KB
build/module/types/ethers-contracts: 152.34 KB
build/module/types: 156.25 KB
build/module/test: 42.97 KB
build/module/src/sdk/strategy: 31.25 KB
build/module/src/sdk: 46.88 KB
build/module/src/characters: 89.84 KB
build/module/src/conditions/context: 42.97 KB
build/module/src/conditions/predefined: 19.53 KB
build/module/src/conditions/base: 54.69 KB
build/module/src/conditions: 156.25 KB
build/module/src/agents: 35.16 KB
build/module/src/policies: 19.53 KB
build/module/src/kits: 19.53 KB
build/module/src: 433.59 KB
build/module: 687.50 KB
build/main/types/ethers-contracts/factories: 82.03 KB
build/main/types/ethers-contracts: 152.34 KB
build/main/types: 156.25 KB
build/main/test: 46.88 KB
build/main/src/sdk/strategy: 31.25 KB
build/main/src/sdk: 46.88 KB
build/main/src/characters: 89.84 KB
build/main/src/conditions/context: 42.97 KB
build/main/src/conditions/predefined: 19.53 KB
build/main/src/conditions/base: 54.69 KB
build/main/src/conditions: 156.25 KB
build/main/src/agents: 35.16 KB
build/main/src/policies: 19.53 KB
build/main/src/kits: 19.53 KB
build/main/src: 437.50 KB
build/main: 695.31 KB
build: 1.35 MB

piotr-roslaniec mentioned this pull request Jun 16, 2023

Support prefix notation for compound conditions #226

Merged

8 tasks

piotr-roslaniec force-pushed the validate-abi-w-ethers branch from b0e00fe to a0bd371 Compare June 28, 2023 08:07

use ethers abi parser to validate function abi

32e6a94

piotr-roslaniec force-pushed the validate-abi-w-ethers branch from a0bd371 to 32e6a94 Compare June 28, 2023 08:17

piotr-roslaniec marked this pull request as ready for review June 28, 2023 08:24

piotr-roslaniec requested review from derekpierre, KPrasch and theref June 28, 2023 08:25

theref approved these changes Jun 28, 2023

View reviewed changes

piotr-roslaniec mentioned this pull request Jun 28, 2023

Refactor CBD API #231

Merged

derekpierre requested review from cygnusv and vzotova June 28, 2023 12:08

derekpierre reviewed Jun 28, 2023

View reviewed changes

derekpierre requested changes Jun 28, 2023

View reviewed changes

apply pr suggestions

37a2b72

piotr-roslaniec force-pushed the validate-abi-w-ethers branch from 6b16a1b to 37a2b72 Compare June 28, 2023 13:35

derekpierre reviewed Jun 28, 2023

View reviewed changes

apply pr suggestions

162e2ba

derekpierre reviewed Jun 28, 2023

View reviewed changes

src/conditions/base/contract.ts Outdated Show resolved Hide resolved

derekpierre approved these changes Jun 28, 2023

View reviewed changes

Update src/conditions/base/contract.ts

5bd27b4

Co-authored-by: Derek Pierre <derek.pierre@gmail.com>

piotr-roslaniec merged commit d9d957e into tdec-epic Jun 28, 2023

piotr-roslaniec deleted the validate-abi-w-ethers branch June 28, 2023 17:07

piotr-roslaniec mentioned this pull request Jul 8, 2023

tDec epic - alpha release tracker #197

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Validate `functionAbi` using `ethers` ABI parser #228

Validate `functionAbi` using `ethers` ABI parser #228

piotr-roslaniec commented Jun 16, 2023 •

edited

Loading

codecov-commenter commented Jun 28, 2023 •

edited

Loading

github-actions bot commented Jun 28, 2023

theref left a comment

derekpierre Jun 28, 2023

piotr-roslaniec Jun 28, 2023

piotr-roslaniec Jun 28, 2023

derekpierre Jun 28, 2023

piotr-roslaniec Jun 28, 2023

derekpierre Jun 28, 2023

piotr-roslaniec Jun 28, 2023

derekpierre Jun 28, 2023

derekpierre Jun 28, 2023 •

edited

Loading

derekpierre left a comment

github-actions bot commented Jun 28, 2023

github-actions bot commented Jun 28, 2023

derekpierre Jun 28, 2023

derekpierre Jun 28, 2023 •

edited

Loading

piotr-roslaniec Jun 28, 2023

derekpierre left a comment

github-actions bot commented Jun 28, 2023

	custom: '"functionAbi" does not contain the method specified as "method"',
	custom: `"functionAbi" not valid for method, "${method}"`,

		@@ -7,7 +7,7 @@ import {
		import { ContractCondition } from '../../../../src/conditions/base';

Validate functionAbi using ethers ABI parser #228

Validate functionAbi using ethers ABI parser #228

Conversation

piotr-roslaniec commented Jun 16, 2023 • edited Loading

codecov-commenter commented Jun 28, 2023 • edited Loading

Codecov Report

github-actions bot commented Jun 28, 2023

theref left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

derekpierre Jun 28, 2023 • edited Loading

Choose a reason for hiding this comment

derekpierre left a comment

Choose a reason for hiding this comment

github-actions bot commented Jun 28, 2023

github-actions bot commented Jun 28, 2023

Choose a reason for hiding this comment

derekpierre Jun 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

derekpierre left a comment

Choose a reason for hiding this comment

github-actions bot commented Jun 28, 2023

Validate `functionAbi` using `ethers` ABI parser #228

Validate `functionAbi` using `ethers` ABI parser #228

piotr-roslaniec commented Jun 16, 2023 •

edited

Loading

codecov-commenter commented Jun 28, 2023 •

edited

Loading

derekpierre Jun 28, 2023 •

edited

Loading

derekpierre Jun 28, 2023 •

edited

Loading