-
Notifications
You must be signed in to change notification settings - Fork 695
feat: include PC abort reason in tx receipt #6018
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
base: develop
Are you sure you want to change the base?
Conversation
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.
This LGTM, just a comment on a refactor that should improve readability.
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.
Overall LGTM, just requesting the comment to be updated
Previously, this callback returned a boolean indicating whether the transaction should be rolled back. Now it returns an optional string, where the string indicates a reason for the abort.
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.
LGTM! I can't actually resolve my comments, but feel free to do so.
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.
LGTM, just a rustdocs request.
Merge conflict resolved and ready for re-review. |
Including this message (which currently only shows in the logs) should be very helpful for users to understand what went wrong in their transaction. I think another change is needed to pass this
vm_error
from the transaction receipt to event listeners, which would then enable the API to consume this information and show it in places like the Explorer. cc @zone117x