feat: replace ethers abi coder with ours #6385
Conversation
Signed-off-by: Marin Petrunic <[email protected]>
Signed-off-by: Marin Petrunic <[email protected]>
mpetrunic
requested review from
avkos,
jdevcs,
luu-alex and
Muhammad-Altabba
as code owners
Deploying with
|
| Latest commit: |
305391c
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://e9ee390d.web3-js-docs.pages.dev |
| Branch Preview URL: | https://feat-web3-abi-coder.web3-js-docs.pages.dev |
Signed-off-by: Marin Petrunic <[email protected]>
Bundle StatsHey there, this message comes from a github action that helps you and reviewers to understand how these changes affect the size of this project's bundle. As this PR is updated, I'll keep you updated on how the bundle size is impacted. Total
View detailed bundle breakdownAdded No assets were added Removed No assets were removed Bigger No assets were bigger Smaller
Unchanged
|
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## 4.x #6385 +/- ##
==========================================
- Coverage 89.88% 89.39% -0.50%
==========================================
Files 201 212 +11
Lines 7780 8135 +355
Branches 2128 2212 +84
==========================================
+ Hits 6993 7272 +279
- Misses 787 863 +76
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Signed-off-by: Marin Petrunic <[email protected]>
Signed-off-by: Marin Petrunic <[email protected]>
Signed-off-by: Marin Petrunic <[email protected]>
| } | ||
| return { | ||
| result: numberResult, | ||
| encoded: bytes.subarray(WORD_SIZE), |
There was a problem hiding this comment.
subarray creates a new view on existing buffer, and changes on new obj will impact original and vice versa. so I think a copy should be made of bytes with required data and sent back from this function.
There was a problem hiding this comment.
same for other places of code in this PR
There was a problem hiding this comment.
That was the goal as it will reduce memory and performance impact. And there is no fear here since decoders never modify any bytes. Better option would be to find a way to freeze internal array buffer but I do not know a way to do it.
There was a problem hiding this comment.
ok, Its tradeoff between memory+performance vs safety( so data is not corrupted by maintainers misuse of it).
I'll also lean towards UX as compared to maintainers. btw I like this approach of using uint8arry / buffers instead of padding using high level functions, its great.
Signed-off-by: Marin Petrunic <[email protected]>
Signed-off-by: Marin Petrunic <[email protected]>
Signed-off-by: Marin Petrunic <[email protected]>
Diff coverage is actually >90%. I assume total coverage decreased as we used a library before so that code had "100%" coverage^^
I mentioned that in PR description as todo. That said, I noticed ethers test cases uses human readable abi and I do think we will hit a lot of cases where abitype is not really parsing them well (afaik they are not standardized). We could use https://github.com/wagmi-dev/viem/blob/main/src/_test/abis.ts instead^^ |
There was a problem hiding this comment.
Thanks for your great contributions!
However, I have a special concern regarding the breaking change that would be introduced with the new errors (mentioned here). Or, what do you think?
| const dynamic = size === -1; | ||
| const dynamicItems = encodedParams.length > 0 && encodedParams[0].dynamic; | ||
| if (!dynamic && values.length !== size) { | ||
| throw new AbiError("Given arguments count doesn't match array length", { |
There was a problem hiding this comment.
Errors here and everywhere else in the MR seems to inturduce breaking changes, right?
Are we planning on upgreading the major version after this MR? Or else we need to keep the errors as they where, right?
There was a problem hiding this comment.
What do you mean? its the same error instance as before. Adding new error messages is not a breaking chnage
There was a problem hiding this comment.
I think this is not just a new error message. The code used to throw an error for this case, and for others, but with a different message and a different structure, right?
The old message was according to the old deleted code:
throw new AbiError(`Parameter encoding error`, err as Error); And the innerError was the error object we get from the encoder.
There was a problem hiding this comment.
Code still throws AbiError error and our api doesn't guarantee inner error type will stay the same (It will be of type Error | Error[] if present). It's user's fault if he expected something that typescript types did not guarantee.
There was a problem hiding this comment.
Thanks @mpetrunic,
Yes, it is not guaranteed by typescript types here. But still the user can even use JavaScript which has no types. I am thinking here about the actual strings and values that the user used to get and might already has ifs checks on. And if we think it is the user's fault to depend on types we did not specify, changing them is still considered a breaking change, as far as I know.
So, as far as I know, if we do not maintain to provide the exact message string and the exact internal properties for every error, as we used to, then we are introducing a breaking change. And yes we can add more properties to the error object. But we are not supposed to remove or update the string or a property value, without pumping the major version.
And yes, our API has been used to guarantee the same message and properties's values for the innerError message because the early used library is following semver.
And I think keeping the error messages and properties, as they were, is possible even though it might be a bit tricky.
Please correct me if changing an error message, or a value of an internal property inside an error object, is not considered a breaking change.
Thanks,
There was a problem hiding this comment.
Afaik, error messages are implementation details intended for the developer's eyes. No one should rely on their exact content or even worse make an if/else statement based on their content (fixing a typo in an error message would then be a breaking change). If the developer intended for you to handle those errors, errors would either be different instances or contain error codes along with the list of error codes the function might throw.
Signed-off-by: Marin Petrunic <[email protected]>
There was a problem hiding this comment.
As discussed, if loosedecoding is enabled it skips checking strict padding rules, due to some old solidity versions bug for event data emitted from external functions.
we are not using loosedecoding feature by passing false, so we can skip this feature in this PR.
Description
Fresh abi code implementation to replace ethers implementation. Should be checked but afaik it adds abilities to encode/decode dynamic array and tuples with dynamic children.
There are probably bugs lurking so I would suggest to add more tests.
P.S. Improved build with concurrently because using '&' means previous scripts are run in the background so there can be concurrency issues with packages that depend on build causing weird type errors from time to time.
resolves #6270 #6211 #6269
Type of change
TODO
Checklist:
npm run lintwith success and extended the tests and types if necessary.npm run test:unitwith success.npm run test:coverageand my test cases cover all the lines and branches of the added code.npm run buildand testeddist/web3.min.jsin a browser.CHANGELOG.mdfile in the root folder.