Skip to content

fix(precompiles): return data for revert #224

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

Draft
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

cloudgray
Copy link
Contributor

Description

Problem

  • When the caller contract calls the precompile, the error message isn’t propagated back to it.
  • Unlike the geth opcode opRevert, which returns the error message in its return data and ErrExecutionReverted as the error, the Cosmos/EVM precompile currently returns nil as return data and a custom error type as the error.
  • We need to make precompile errors behave like opRevert: return the error message in the return data and use ErrExecutionReverted as the error.

Solution

  1. Modify return values on precompile error
    • Always return ErrExecutionReverted as the error.
    • Encode the original error message with the Revert selector ("Error(string)") as the return data.
    • Use evm.Interpreter.SetReturnData() to set that encoded data into EVMInterpreter.returnData.
  2. Modify return values in ApplyMessageWithConfig
    • If err is ErrExecutionReverted, set res.Ret to evm.Interpreter.ReturnData().
  3. Update the test util function
    • Previously, on error the code logged the revert reason via emitted events (which never occur when an error is thrown).
    • Now, pull the EthereumTxResponse.Ret returned in step 2, decode it with abi.UnpackRevert, and print that as the reason.

Result

As-Is

err_msg_empty In the original code, errors thrown by the precompile were not propagated to the caller contract.

To-Be

revert_with_reason
Now, errors returned by the precompile are returned in the caller contract’s return data, and the test utilities have been updated so that the test code can verify them.

Closes: #223


Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

  • tackled an existing issue or discussed with a team member
  • left instructions on how to review the changes
  • targeted the main branch

Reviewers Checklist

All items are required.
Please add a note if the item is not applicable
and please add your handle next to the items reviewed
if you only reviewed selected items.

I have...

  • added a relevant changelog entry to the Unreleased section in CHANGELOG.md
  • confirmed all author checklist items have been addressed
  • confirmed that this PR does not change production code
  • reviewed content
  • tested instructions (if applicable)
  • confirmed all CI checks have 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.

Precompile Errors Not Propagated to the Caller Contract
1 participant