Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
feat: replace ethers abi coder with ours #6385
feat: replace ethers abi coder with ours #6385
Changes from all commits
cc2f8f7
3badf8a
7a7ebdc
e21cb70
8204a19
c2e034a
d48f099
56c7dc6
27c33c0
7b83965
a7796ce
da0a41d
9cb6bde
0db1dd9
88238b7
305391c
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
if
s 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.