Skip to content
This repository was archived by the owner on Mar 5, 2025. It is now read-only.

Fix validation uint int sizes - #6434

Merged
luu-alex merged 16 commits into
web3:4.xfrom
fullkomnun:fix_validation_uint_int_sizes
Oct 17, 2023
Merged

Fix validation uint int sizes#6434
luu-alex merged 16 commits into
web3:4.xfrom
fullkomnun:fix_validation_uint_int_sizes

Conversation

@fullkomnun

@fullkomnun fullkomnun commented Sep 17, 2023

Copy link
Copy Markdown
Contributor

Description

Previously an ABI that contained a numeric type (int/uint) of size that is not a power of 2 (such as uint40, int24) would result in the validator / check function being set on Zod as undefined and lead to a cryptic TypeError during validation.
I have setup validators for all valid numeric type sizes ([u]int8, [u]int 16, ... , [u]int256) and throw an error right away if during conversion of JSON scheme to Zod an unsupported type is encountered.
Generated 'single parameter' ABI test cases for all valid sizes.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

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.

@codecov

codecov Bot commented Sep 18, 2023 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

Merging #6434 (30ab555) into 4.x (226b3ba) will increase coverage by 0.00%.
The diff coverage is 92.85%.

Additional details and impacted files 
@@           Coverage Diff           @@
##              4.x    #6434   +/-   ##
=======================================
  Coverage   89.67%   89.67%           
=======================================
  Files         214      215    +1     
  Lines        8222     8234   +12     
  Branches     2223     2224    +1     
=======================================
+ Hits         7373     7384   +11     
- Misses        849      850    +1
Flag Coverage Δ
UnitTests 89.67% <92.85%> (+<0.01%) ⬆️

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 ∅ <ø> (∅)

avkos
avkos approved these changes Sep 18, 2023
View reviewed changes

@avkos avkos left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks for your contribution

luu-alex
luu-alex reviewed Sep 18, 2023
View reviewed changes
Comment thread packages/web3-validator/coverage/coverage-final.json Outdated
jdevcs
jdevcs approved these changes Sep 18, 2023
View reviewed changes

@jdevcs jdevcs left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for contribution

@jdevcs

jdevcs commented Sep 19, 2023

Copy link
Copy Markdown
Contributor

Same in this PR , update is required to fix Conflicting file, I'll merge PR after it, Thanks

@fullkomnun
fullkomnun force-pushed the fix_validation_uint_int_sizes branch from 584d31d to 0e76199 Compare September 21, 2023 22:45
@fullkomnun fullkomnun mentioned this pull request Sep 22, 2023
Closed
@fullkomnun
fullkomnun force-pushed the fix_validation_uint_int_sizes branch from 0e76199 to 47004c7 Compare September 22, 2023 17:45
@fullkomnun
fullkomnun force-pushed the fix_validation_uint_int_sizes branch from 47004c7 to d408325 Compare September 23, 2023 15:26
jdevcs
jdevcs reviewed Sep 26, 2023
View reviewed changes

@jdevcs jdevcs left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unit test not passing

@fullkomnun

fullkomnun commented Sep 26, 2023

Copy link
Copy Markdown
Contributor Author

unit test not passing

Checking

@CodeLikeLisa

CodeLikeLisa commented Sep 27, 2023

Copy link
Copy Markdown

is the solution to use an older version of web3js?

jdevcs
jdevcs reviewed Sep 28, 2023
View reviewed changes
Comment thread packages/web3-validator/src/validator.ts Outdated
@jdevcs
jdevcs self-requested a review September 28, 2023 11:39
@luu-alex

luu-alex commented Sep 29, 2023

Copy link
Copy Markdown
Contributor

Hey there im looking into an issue. @fullkomnun theres an inconsistent integration test failing in the pipelines and i think it stems from the last PR you pushed. The abi and the testcase that is failing. any ideas?

@luu-alex
luu-alex self-requested a review September 29, 2023 13:39
jdevcs
jdevcs reviewed Oct 2, 2023
View reviewed changes

@jdevcs jdevcs left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

codecov/patch Failing after 1s — 66.66% of diff hit (target 89.88%)

@luu-alex

luu-alex commented Oct 3, 2023

Copy link
Copy Markdown
Contributor

@luu-alex update, it seems to be an eventslog issue. not related to PR

@luu-alex luu-alex mentioned this pull request Oct 4, 2023
Merged
17 tasks
@jdevcs

jdevcs commented Oct 9, 2023

Copy link
Copy Markdown
Contributor

codecov/patch Failing after 1s — 66.66% of diff hit (target 89.88%)

@fullkomnun

@jdevcs
jdevcs self-requested a review October 10, 2023 13:43
Muhammad-Altabba
Muhammad-Altabba reviewed Oct 11, 2023
View reviewed changes
Comment thread packages/web3-validator/src/validator.ts Outdated
@luu-alex luu-alex mentioned this pull request Oct 12, 2023
Closed
Muhammad-Altabba
Muhammad-Altabba approved these changes Oct 16, 2023
View reviewed changes

@Muhammad-Altabba Muhammad-Altabba left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks,
I just have suggestions for the error string...

Comment thread packages/web3-validator/CHANGELOG.md Outdated
Comment thread packages/web3-errors/src/errors/schema_errors.ts Outdated
Comment thread packages/web3-errors/test/unit/__snapshots__/errors.test.ts.snap Outdated
@ngmachado ngmachado mentioned this pull request Oct 17, 2023
Closed
@ngmachado

ngmachado commented Oct 17, 2023

Copy link
Copy Markdown

Any news about this PR?

@luu-alex

luu-alex commented Oct 17, 2023

Copy link
Copy Markdown
Contributor

@ngmachado will be merged in today

@luu-alex
luu-alex merged commit 09f4c8b into web3:4.x Oct 17, 2023
@jdevcs jdevcs mentioned this pull request Oct 18, 2023
Merged
@mconnelly8 mconnelly8 mentioned this pull request Jan 11, 2024
Closed
This was referenced Sep 18, 2026
Closed
Draft
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@luu-alex luu-alex Awaiting requested review from luu-alex
3 more reviewers
@avkos avkos avkos approved these changes
@Muhammad-Altabba Muhammad-Altabba Muhammad-Altabba approved these changes
@jdevcs jdevcs jdevcs approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@fullkomnun @jdevcs @CodeLikeLisa @luu-alex @ngmachado @avkos @Muhammad-Altabba