-
Notifications
You must be signed in to change notification settings - Fork 400
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
Extract revert messages #6226
Extract revert messages #6226
Conversation
@@ -104,4 +106,36 @@ private string DefaultErrorMessage(ReadOnlySpan<byte> span) | |||
return null; | |||
} | |||
} | |||
|
|||
private unsafe string? TryUnpackRevertMessage(ReadOnlySpan<byte> span) |
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.
Shouldn't this be unified with TryGetErrorMessage
? Looks similar.
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.
They are similar but not the same:
- A specific prefix is required.
- The span has a required minimum length, not a specific length.
- The returned error message is decoded as UTF8.
Still, maybe there's something to be done to reduce the amount of code. What do you think @benaadams?
We could and maybe should do that with ABI, but this is bigger refactor, so lets go with this for now. |
@@ -18,8 +19,25 @@ public class TransactionSubstate | |||
private const string Revert = "revert"; | |||
|
|||
private const int RevertPrefix = 4; | |||
private const int WordSize = EvmPooledMemory.WordSize; | |||
|
|||
private const string RevertedErrorMessagePrefix = "Reverted "; |
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.
If there was an extra colon in the prefix, i.e. Reverted: {message}
this would become compatible with Geth. I want to point it out since you're already making a breaking change.
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.
There's an ongoing discussion on standarization of error messages: ethereum/execution-apis#463.
This reverts commit 594ab98.
# Conflicts: # src/Nethermind/Nethermind.Evm/TransactionSubstate.cs
Fixes #6224
Changes
revert
error messages.Types of changes
What types of changes does your code introduce?
Testing
Requires testing
If yes, did you write tests?
Documentation
Requires documentation update
Requires explanation in Release Notes
Remarks
Some clients like
etherjs
try to decode error messages on a per client basis in an ad-hoc fashion given that there is currently no standarized way to return errors foreth_estimateGas
(see ethereum/execution-apis#463). This PR might affect clients that perform such decoding.