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

Polkadot: Reverts return encoded error data #1449

Merged
merged 62 commits into from
Aug 10, 2023

Conversation

xermicus
Copy link
Contributor

@xermicus xermicus commented Jul 19, 2023

This is a continuation of #1415. require(), assert() and revert() now return error data, according to the Ethereum Solidity documentation. Additionally, many reverts inserted by the compiler now return the corresponding Panic(uint256) error data, to align Solang closer with solc.

The error types known to the contract are added in the metadata lang_error field. At the moment there are only Error and Panic because we don't support custom errors yet.

Refactored revert-related code into a dedicated codegen module to. Refactored the polkadot::errors into distinct tests, made them less brittle and added assertions for the execution output.

I'm now working on a follow-up PR for bubbling up uncaught exceptions (this is why it's already included that in the documentation).

xermicus and others added 29 commits July 6, 2023 23:03
Signed-off-by: xermicus <[email protected]>
Signed-off-by: Cyrill Leutwiler <[email protected]>
Signed-off-by: Cyrill Leutwiler <[email protected]>
Signed-off-by: xermicus <[email protected]>
Signed-off-by: xermicus <[email protected]>
Signed-off-by: xermicus <[email protected]>
Signed-off-by: xermicus <[email protected]>
Signed-off-by: xermicus <[email protected]>
Signed-off-by: Cyrill Leutwiler <[email protected]>
Signed-off-by: Cyrill Leutwiler <[email protected]>
Signed-off-by: Cyrill Leutwiler <[email protected]>
Signed-off-by: Cyrill Leutwiler <[email protected]>
Signed-off-by: xermicus <[email protected]>
Signed-off-by: Cyrill Leutwiler <[email protected]>
@xermicus xermicus requested a review from seanyoung as a code owner July 19, 2023 17:24
Signed-off-by: xermicus <[email protected]>
Copy link
Contributor

@seanyoung seanyoung left a comment

Choose a reason for hiding this comment

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

Great stuff just some minor things

docs/targets/polkadot.rst Outdated Show resolved Hide resolved
docs/targets/polkadot.rst Outdated Show resolved Hide resolved
src/codegen/encoding/mod.rs Show resolved Hide resolved
src/codegen/encoding/scale_encoding.rs Outdated Show resolved Hide resolved
src/codegen/encoding/scale_encoding.rs Outdated Show resolved Hide resolved
src/emit/instructions.rs Outdated Show resolved Hide resolved
xermicus and others added 3 commits July 26, 2023 12:33
Copy link
Contributor

@seanyoung seanyoung left a comment

Choose a reason for hiding this comment

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

There are still comments open when you requested a review from me

src/codegen/encoding/scale_encoding.rs Outdated Show resolved Hide resolved
docs/targets/polkadot.rst Outdated Show resolved Hide resolved
src/codegen/encoding/scale_encoding.rs Outdated Show resolved Hide resolved
src/codegen/encoding/scale_encoding.rs Outdated Show resolved Hide resolved
src/emit/binary.rs Show resolved Hide resolved
xermicus and others added 4 commits July 26, 2023 21:15
Signed-off-by: Cyrill Leutwiler <[email protected]>
docs/targets/polkadot.rst Outdated Show resolved Hide resolved
src/emit/instructions.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@seanyoung seanyoung left a comment

Choose a reason for hiding this comment

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

Please

src/codegen/encoding/scale_encoding.rs Outdated Show resolved Hide resolved
docs/targets/polkadot.rst Outdated Show resolved Hide resolved
docs/targets/polkadot.rst Outdated Show resolved Hide resolved
Comment on lines 80 to 82
SCALE encoding does not allow discriminators larger than 1 byte, the hex-encoded error selector
is provided as the enum variant name in the metadata (the selector could also be calculated by
reconstructing and hashing the error signature based on the enum variant types).
Copy link
Contributor

Choose a reason for hiding this comment

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

So where does the name go? E.g. Panic, Error, or CustomError5?

I do think this this paragraph is inscrutable. I still don't have a good idea of what is going on here.

Copy link
Contributor Author

@xermicus xermicus Aug 8, 2023

Choose a reason for hiding this comment

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

Makes me think, do you think it'd be better if the Selector bytes goes into the path of the Variant Type?

Copy link
Contributor Author

@xermicus xermicus Aug 9, 2023

Choose a reason for hiding this comment

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

I've rewritten it and added an example.

I also removed that the selector can be reconstructed. We don't guarantee this for messages selectors, so I doubt whether we can guarantee this for errors once we introduce custom errors. And including the selector directly is better anyways and makes this unnecessary anyways.

docs/targets/polkadot.rst Outdated Show resolved Hide resolved
xermicus and others added 7 commits August 8, 2023 11:33
Co-authored-by: Lucas Steuernagel <[email protected]>
Signed-off-by: Cyrill Leutwiler <[email protected]>
Signed-off-by: Cyrill Leutwiler <[email protected]>
Signed-off-by: Cyrill Leutwiler <[email protected]>
Signed-off-by: Cyrill Leutwiler <[email protected]>
Signed-off-by: xermicus <[email protected]>
@xermicus xermicus requested a review from seanyoung August 9, 2023 16:30
@xermicus xermicus merged commit bac5707 into hyperledger-solang:main Aug 10, 2023
17 checks passed
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.

3 participants