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

Include human-friendly error messages on EVM transaction events #6836

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

m-Peter
Copy link
Collaborator

@m-Peter m-Peter commented Dec 24, 2024

Closes #6770

@codecov-commenter
Copy link

codecov-commenter commented Dec 24, 2024

Codecov Report

Attention: Patch coverage is 10.00000% with 9 lines in your changes missing coverage. Please review.

Project coverage is 41.17%. Comparing base (72adf9e) to head (e682cd2).

Files with missing lines Patch % Lines
fvm/evm/types/result.go 0.00% 9 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #6836      +/-   ##
==========================================
- Coverage   41.18%   41.17%   -0.01%     
==========================================
  Files        2109     2109              
  Lines      185660   185669       +9     
==========================================
- Hits        76460    76450      -10     
- Misses     102788   102805      +17     
- Partials     6412     6414       +2     
Flag Coverage Δ
unittests 41.17% <10.00%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@m-Peter m-Peter force-pushed the mpeter/evm-events-abi-decode-assert-errors branch 2 times, most recently from 2d8eb89 to 7fb6ff4 Compare December 29, 2024 11:46
Comment on lines 67 to 73
errorMessage := p.Result.ErrorMsg()
if p.Result.ResultSummary().ErrorCode == types.ExecutionErrCodeExecutionReverted {
reason, errUnpack := abi.UnpackRevert(p.Result.ReturnedData)
if errUnpack == nil {
errorMessage = fmt.Sprintf("%v: %v", gethVM.ErrExecutionReverted.Error(), reason)
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you make this a method on types.Result. Maybe call it ErrorMessageWithRevertReason or something.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Nice idea 👌 Updated in e682cd2 .

@m-Peter m-Peter force-pushed the mpeter/evm-events-abi-decode-assert-errors branch from 7fb6ff4 to e682cd2 Compare January 6, 2025 10:54
Copy link
Contributor

@janezpodhostnik janezpodhostnik left a comment

Choose a reason for hiding this comment

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

LGTM!
Needs an HCU to be deployed, since error bodies are part of the proofs as far as I'm aware.

Copy link
Member

@zhangchiqing zhangchiqing left a comment

Choose a reason for hiding this comment

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

would this require HCU?

@m-Peter
Copy link
Collaborator Author

m-Peter commented Jan 6, 2025

@janezpodhostnik @zhangchiqing This change affects only the errorMessage field on EVM.TransactionExecuted event (https://github.com/onflow/flow-go/blob/master/fvm/evm/stdlib/contract.cdc#L55-L56).
It doesn't change the error message in any Cadence EVM APIs (such as EVM.run / EVM.dryRun / coa.call etc).
I don't know if this means that an HCU is required or not though 😅 But I hope the clarification helps somehow 🙏

Copy link
Member

@zhangchiqing zhangchiqing left a comment

Choose a reason for hiding this comment

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

Yeah, the error message is not part of the execution result. so I think HCU is not necessary.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Flow EVM] Consider including RevertReason string in EVM.TransactionExecuted event payload
4 participants