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

fix 2d array bug of abiSchemaToJsonSchema in web3-validator #6836

Merged
merged 9 commits into from
Mar 20, 2024

Conversation

EtlesL
Copy link
Contributor

@EtlesL EtlesL commented Feb 27, 2024

Description

We encountered a bug in the web3-validator code where the handling of multi-dimensional arrays was not implemented correctly.
In the original code snippet, the arraySize vatiable was only assigned the value of the first element in the arraySizes array. This prevented proper handling of multi-dimensional arrays.

const arraySize = arraySizes[0];
const item: JsonSchema = {
    type: 'array',
    $id: abiName,
    items: convertEthType(String(baseType)),
    minItems: arraySize,
    maxItems: arraySize,
};

To fix the bug, I added an additional loop to iterate over the arraySizes array to handle each dimension:

for (let i = 0; i < arraySizes.length; i++) {
  const arraySize = arraySizes[i];
  const item: JsonSchema = {
      type: 'array',
      $id: abiName,
      items: convertEthType(String(baseType)),
      minItems: arraySize,
      maxItems: arraySize,
  };

  if (arraySize < 0) {
      delete item.maxItems;
      delete item.minItems;
  }

  (lastSchema.items as JsonSchema[]).push(item);
}

Besides, different from Issue #6486, if the multi-dimensional arrays has a fixed length, it will go to different code snippet.

Fixes #6798

Type of change

  • Bug fix (non-breaking change which fixes an issue)

Checklist:

  • I have selected the correct base branch.
  • I have performed a self-review of my own code.
  • I have commented my code, particularly in hard-to-understand areas.
  • I have made corresponding changes to the documentation.
  • My changes generate no new warnings.
  • Any dependent changes have been merged and published in downstream modules.
  • I ran npm run lint with success and extended the tests and types if necessary.
  • I ran npm run test:unit with success.
  • I ran npm run test:coverage and my test cases cover all the lines and branches of the added code.
  • I ran npm run build and tested dist/web3.min.js in a browser.
  • I have tested my code on the live network.
  • I have checked the Deploy Preview and it looks correct.
  • I have updated the CHANGELOG.md file in the root folder.
  • I have linked Issue(s) with this PR in "Linked Issues" menu.

Yi Zhang added 3 commits March 1, 2024 14:00
- 123 cannot be uint
- test case("nested tuple object in nested array") has a wrong data structure of tuple[][3]
1. fix function convertToZod to distinguish tuple and array, add checks in maxItems & minItems
2. revert abiSchemaToJsonSchema of utils and fix
3. add testcases in validator.validate
@EtlesL
Copy link
Contributor Author

EtlesL commented Mar 1, 2024

@jdevcs
I'm sorry for the misunderstanding about the type conversion earlier.
At first, I thought the multi-dimensional array was not converted correctly, so I added a loop. However, after carefully reading the code, I realized that this was not correct. Regarding this bug,

public validate(schema: JsonSchema, data: Json, options?: { silent?: boolean }) {
    const zod = convertToZod(schema);
    const result = zod.safeParse(data);
    if (!result.success) {
	    const errors = this.convertErrors(result.error?.issues ?? []);
	    if (errors) {
		    if (options?.silent) {
			    return errors;
		    }
		    throw new Web3ValidatorError(errors);
	    }
    }
    return undefined;
}

The core issue lies in the first line, where the schema is converted to Zod. In convertToZod function, multi-dimensional arrays and tuple types are confused.

if (schema?.type === 'array' && schema?.items) {
    if (Array.isArray(schema.items) && schema.items.length > 0) {
        // do somsthing
        return z.tuple(arr as [ZodTypeAny, ...ZodTypeAny[]]);
    }
    return z.array(convertToZod(schema.items));
}

So, I added additional conditions to distinguish between them. If it's a tuple

  1. schema.items MUST be an array
  2. the length schema.items MUST have more than 1 items
  3. maxItems MUST NOT be undefined
  4. the ids of schema.items MUST be unique
if (Array.isArray(schema.items) 
    && schema.items.length > 1
    && schema.maxItems !== undefined
    && new Set(schema.items.map((item: JsonSchema) => item.$id)).size === schema.items.length) {
        //do something
        return z.tuple(arr as [ZodTypeAny, ...ZodTypeAny[]]);
}

Also, in this method, the maxItems and minItems were lost during the conversion, resulting in the length judgment always being "must NOT have more than 1 item."

const nextSchema = Array.isArray(schema.items) ? schema.items[0] : schema.items;
let zodArraySchema = z.array(convertToZod(nextSchema));

zodArraySchema = schema.minItems !== undefined ? zodArraySchema.min(schema.minItems) : zodArraySchema;
zodArraySchema = schema.maxItems !== undefined ? zodArraySchema.max(schema.maxItems) : zodArraySchema;
return zodArraySchema;

Besides that, the previous unit tests did not cover all test cases. After adding the abiToJsonSchemaCases in the web_validator.test, I found that:

  1. The test case "nested tuple object in nested array" has a type error. The tuple[][3] should have 3 elements in the outer layer and an unspecified number in the inner layer, but the original code was written in reverse.
  2. The test case "nested tuple object in nested array" also has a small issue with the utils abi to jsonSchema, which I fixed as well.

Please review again, thx.

@EtlesL EtlesL requested a review from jdevcs March 1, 2024 16:26
@EtlesL
Copy link
Contributor Author

EtlesL commented Mar 2, 2024

I can't run tests of web-eth-contracts on my local environment, I saw the error in the auto pipeline and fixed. Not sure why, because I didn't change the codes of contracts packages. Please review.

@EtlesL
Copy link
Contributor Author

EtlesL commented Mar 5, 2024

Hi, are there any updates? I noticed the pipeline failed, but there are no error logs provided.

@jdevcs
Copy link
Contributor

jdevcs commented Mar 5, 2024

Thanks for detailed explanation of changes, there are some unstable tests ( not related with this PR ) and we are working in current sprint for fixing these. #6857, could you rebase your branch when 6857 is merged,

@EtlesL
Copy link
Contributor Author

EtlesL commented Mar 10, 2024

Thanks for detailed explanation of changes, there are some unstable tests ( not related with this PR ) and we are working in current sprint for fixing these. #6857, could you rebase your branch when 6857 is merged,

@jdevcs Hi, I saw it merged, and I've updated the branch.

Copy link

codecov bot commented Mar 13, 2024

Codecov Report

Merging #6836 (26610c6) into 4.x (383af39) will decrease coverage by 0.02%.
The diff coverage is 94.44%.

Additional details and impacted files
@@            Coverage Diff             @@
##              4.x    #6836      +/-   ##
==========================================
- Coverage   91.99%   91.98%   -0.02%     
==========================================
  Files         215      215              
  Lines        8172     8174       +2     
  Branches     2202     2207       +5     
==========================================
+ Hits         7518     7519       +1     
- Misses        654      655       +1     
Flag Coverage Δ
UnitTests 91.98% <94.44%> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components Coverage Δ
web3 ∅ <ø> (∅)
web3-core ∅ <ø> (∅)
web3-errors ∅ <ø> (∅)
web3-eth ∅ <ø> (∅)
web3-eth-abi ∅ <ø> (∅)
web3-eth-accounts ∅ <ø> (∅)
web3-eth-contract ∅ <ø> (∅)
web3-eth-ens ∅ <ø> (∅)
web3-eth-iban ∅ <ø> (∅)
web3-eth-personal ∅ <ø> (∅)
web3-net ∅ <ø> (∅)
web3-providers-http ∅ <ø> (∅)
web3-providers-ipc ∅ <ø> (∅)
web3-providers-ws ∅ <ø> (∅)
web3-rpc-methods ∅ <ø> (∅)
web3-utils ∅ <ø> (∅)
web3-validator ∅ <ø> (∅)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2d array with web3 validator
3 participants