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

Better ABI Validation: check type of constructor/call args against abi #278

Open
2 tasks
kanej opened this issue Jun 20, 2023 · 2 comments · May be fixed by #690
Open
2 tasks

Better ABI Validation: check type of constructor/call args against abi #278

kanej opened this issue Jun 20, 2023 · 2 comments · May be fixed by #690
Assignees
Labels
status:ready This issue is ready to be worked on
Milestone

Comments

@kanej
Copy link
Member

kanej commented Jun 20, 2023

Add to the general validation phase a check that each argument in the call/constructor args of the relevant futures matches the type expected by the abi.

TODO

  • Draw up a test plan that will effectively act as a specification
  • Spike type validation for single future (i.e. named contract deploy)
@kanej kanej added the status:ready This issue is ready to be worked on label Jun 20, 2023
@kanej kanej added this to the Public Beta milestone Jun 20, 2023
@kanej kanej moved this to Todo in Hardhat Ignition Jun 20, 2023
@kanej kanej modified the milestones: Beta v0.4.0, Beta v0.5.0 Sep 1, 2023
@kanej kanej mentioned this issue Sep 18, 2023
20 tasks
@kanej kanej removed this from the Public beta followup milestone Oct 24, 2023
@kanej kanej changed the title Validation: check type of constructor/call args against abi Better ABI Validation: check type of constructor/call args against abi Oct 26, 2023
@kanej kanej added this to the v1.0.0 milestone Oct 26, 2023
@zoeyTM zoeyTM moved this from Todo to In Progress in Hardhat Ignition Feb 5, 2024
@zoeyTM zoeyTM self-assigned this Feb 5, 2024
@zoeyTM
Copy link
Contributor

zoeyTM commented Feb 5, 2024

The types of futures that accept arguments and need validation are:

  • contract deploys (not libraries)
  • function calls
  • static calls

For arguments, these are the types of inputs we need to be able to validate:

  • raw primitives (number, bigint, string, boolean)
  • contract futures (resolve to address type)
  • static call futures (resolves to any solidity param type)
  • read argument event futures (resolves to any solidity param type)
  • runtime value
    • account (resolves to address type)
    • module parameter (resolves to number, bigint, string, boolean)
  • a record of any of the above
  • an array of any of the above

For raw primitives, we can let ethers do the heavy lifting for us with something like:

const iface = new ethers.Interface(artifact.abi);

try {
  iface.encodeDeploy([future.constructorArgs])
} catch {
  throw new Error('failed validation')
}

however, this approach would only work for primitive values, so I think primitives should instead just be accounted for within our custom loop that could look something like this (pseudocode):

function validateArgs(artifact: Artifact, functionName: string, args: ArgumentType[]): IgnitionError[] {
  // loop through, probably recursively, and resolve to primitive type strings
  const argTypes = args.map((arg) => resolveTypeForArg(artifact, arg)); 

  // get an array of primitive type strings representing the valid type for each arg, derived from the ABI and likely with help from ethers
  const validTypes = resolveValidTypesForFunctionArgs(artifact, functionName);

  // compare each argument against the valid type
  let errors = [];
  for (let i = 0; i < argTypes.length; i++) {
    if (argTypes[i] !== validTypes[i]) {
      errors.push(new IgnitionError("invalid arg type"));
    }
  }

  return errors;
}

@kanej
Copy link
Member Author

kanej commented Feb 5, 2024

@zoeyTM if I am following. During validation we add additional checks to args (either call or constructor) that recursively cross checks the types of the passed values with the ABI's type.

Does ethers v6 give us any util functions to help with this?
It seems viem has parseAbi, but I wouldn't want to switch to viem in just his case (and a full viem switch is likely out of scope).

Do you think cross checking the types is extensive custom code? Or is leveraging either ethers/viem the way to go?

@zoeyTM zoeyTM linked a pull request Feb 7, 2024 that will close this issue
@zoeyTM zoeyTM moved this from In Progress to In Review in Hardhat Ignition Feb 7, 2024
@kanej kanej moved this from In Review to Blocked in Hardhat Ignition Apr 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status:ready This issue is ready to be worked on
Projects
Status: Blocked
Development

Successfully merging a pull request may close this issue.

2 participants