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

[NIT-3084] Check if the JSON-RPC error code is 3 for confirming execution reverted #2924

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

Conversation

amsanghi
Copy link
Contributor

@amsanghi amsanghi commented Feb 4, 2025

Fixes: NIT-3084

@amsanghi amsanghi marked this pull request as ready for review February 4, 2025 15:27
@amsanghi amsanghi requested a review from eljobe February 13, 2025 16:33
}
var rpcError rpc.Error
ok := errors.As(err, &rpcError)
if ok && rpcError.ErrorCode() == 3 {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I know that this was the task, and remember Lee finding that error code, but so far I found no confirmation that error code 3 does mean "execution reverted".
I think remove this part or find other confirmation for it.
I do like the creation of IsExecutionReverted error function, and would be happy to merge it. Not critical.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, so i looked around and it looks like the code is 6 based on https://github.com/OffchainLabs/go-ethereum/blob/master/core/vm/errors.go#L131, let me know if I should change it to 6 or it still makes sense to just just remove the code part.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually nvm , here's the part where code 3 comes from - https://github.com/OffchainLabs/go-ethereum/blob/master/internal/ethapi/errors.go#L37

Copy link
Member

Choose a reason for hiding this comment

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

Yeah. I think this looks right.

Copy link
Member

@eljobe eljobe left a comment

Choose a reason for hiding this comment

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

LGTM

}
var rpcError rpc.Error
ok := errors.As(err, &rpcError)
if ok && rpcError.ErrorCode() == 3 {
Copy link
Member

Choose a reason for hiding this comment

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

Yeah. I think this looks right.

Copy link
Collaborator

@tsahee tsahee left a comment

Choose a reason for hiding this comment

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

LGTM

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