Skip to content

Conversation

mmsqe
Copy link
Contributor

@mmsqe mmsqe commented Jul 15, 2025

Description

  • this change shouldn't have been included since revert reason was aligned
  • message field already contains error detail, data get duplicated
  • for test info

Closes: #XXXX


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

@aljo242
Copy link
Contributor

aljo242 commented Jul 15, 2025

Hey @mmsqe could you provide more context on this - why is this being reverted?

@mmsqe
Copy link
Contributor Author

mmsqe commented Jul 16, 2025

Hey @mmsqe could you provide more context on this - why is this being reverted?

Seems like behavior change after #224, but we could always decode for precompile tests right?

@cloudgray
Copy link
Contributor

cloudgray commented Jul 17, 2025

Hey @mmsqe could you provide more context on this - why is this being reverted?

Seems like behavior change after #224, but we could always decode for precompile tests right?

Hi @mmsqe!
Thank you very much for correcting the part I had mistakenly modified.

To add some context, I do believe that PR#224 was necessary. The part I incorrectly modified appears to be the handling of the return data in the JSON-RPC layer, where the return data became not hex-encoded.

The issue that PR#224 aimed to address was, more precisely, that when a precompile is invoked via a caller contract, the revert reason was not included in the return data, making it impossible to decode. [ref: #223 ]

The root cause was that the precompile, upon encountering an error, was returning it in a way that differed from how Geth’s opCall handles errors.

  • In Geth, when a revert occurs, opCall returns nil for the error and the hex-encoded revert reason in the return data.
  • On the other hand, prior to PR#224, the precompile would return a custom error type containing the revert reason in the error itself, and nil as the return data—the opposite behavior.

Although this diverged from the Geth interpreter’s behavior, it didn’t cause any issues when the precompile was invoked directly.

However, when the precompile is called from within a contract, the revert reason does not get correctly passed along to opRevert, resulting in its loss.

The key changes I made were as follows:

  1. Changed the precompile’s return values on error from (nil, Error("reason")) to (ExecRevertError, abi-encoded("reason")). [ref]
  2. Set the revert reason to EVM.Interpreter.returnData during errors, as is done by other opcodes, so that it can be accessed by operations like opReturnDataCopy. [ref]

From what I can see in your modifications, the issue doesn't seem to stem from these parts. That said, if I’ve misunderstood anything, I’d greatly appreciate your clarification!

cc. @zsystm @vladjdk @aljo242

@cloudgray
Copy link
Contributor

@mmsqe

Upon reviewing PR#224 again at this point, it seems that setting the EVM interpreter's returndata to the response in ApplyMessageWithConfig() is unnecessary. This part is already handled automatically within the EVM itself. [ref]

Would you be able to remove that section in this PR?

Copy link
Contributor

@cloudgray cloudgray left a comment

Choose a reason for hiding this comment

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

LGTM! Really appreciate your contribution!

Copy link
Contributor

@aljo242 aljo242 left a comment

Choose a reason for hiding this comment

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

LGTM with a small nit

@aljo242 aljo242 merged commit 8d21baf into cosmos:main Jul 17, 2025
15 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