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

feat(rpc): adds revert data to eth_call #2281

Merged
merged 5 commits into from
May 17, 2024
Merged

Conversation

jurajpiar
Copy link
Member

Adds revert data to the eth_call response if failed.

Description

  • Created a new ProgramRevert class to combine revert reason and revert data.
  • Changed decodeProgramRevert (formerly decodeProgramRevertMessage) to return ProgramRevert
  • Modified RskJsonRpcRequestException to accept ProgramRevert
  • Modified basic unit test test_revertedTransaction to check for this revert data
  • ...TBC

Motivation and Context

How Has This Been Tested?

  • [WIP] Unit tests
  • Integration test TBD

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • Tests for the changes have been added (for bug fixes / features)
  • Requires Activation Code (Hard Fork)
  • Other information:

Copy link
Contributor

@Vovchyk Vovchyk left a comment

Choose a reason for hiding this comment

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

don't forget to pass this data value to JsonError in RskErrorResolver

@jurajpiar jurajpiar force-pushed the juraj/rpc_w_revert_data branch from dbb850f to 59e2713 Compare April 5, 2024 17:41
@jurajpiar jurajpiar force-pushed the juraj/rpc_w_revert_data branch 2 times, most recently from f7cbf04 to d93b78a Compare April 9, 2024 13:38
@jurajpiar jurajpiar force-pushed the juraj/rpc_w_revert_data branch from d93b78a to ac06f54 Compare April 9, 2024 14:03
@jurajpiar jurajpiar force-pushed the juraj/rpc_w_revert_data branch 5 times, most recently from f65d4b8 to 6581123 Compare April 10, 2024 16:17
@jurajpiar jurajpiar marked this pull request as ready for review April 10, 2024 16:24
@jurajpiar jurajpiar requested review from Vovchyk and asoto-iov April 10, 2024 16:24
@jurajpiar jurajpiar force-pushed the juraj/rpc_w_revert_data branch 6 times, most recently from 0765896 to 6f801e1 Compare April 11, 2024 01:39
@jurajpiar
Copy link
Member Author

pipeline:run

Copy link
Contributor

@fmacleal fmacleal left a comment

Choose a reason for hiding this comment

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

Good job!

I have few comments, if you have any doubt, let me know and I we disccuss it.

@jurajpiar jurajpiar force-pushed the juraj/rpc_w_revert_data branch 4 times, most recently from e3de0a8 to 9ec43f0 Compare April 18, 2024 09:34
@jurajpiar jurajpiar requested review from Vovchyk and fmacleal April 18, 2024 09:35
@jurajpiar
Copy link
Member Author

pipeline:run

fmacleal
fmacleal previously approved these changes Apr 18, 2024
Copy link
Contributor

@fmacleal fmacleal left a comment

Choose a reason for hiding this comment

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

Nice, good job!

Copy link
Contributor

@nagarev nagarev left a comment

Choose a reason for hiding this comment

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

LGTM

@0xM3R
Copy link

0xM3R commented May 15, 2024

Here are some further improvement proposal RskJsonRpcRequestException.java although they aren't direct security risk items:

  • The byte[] revertData field should rather be declared as immutable to prevent unexpected modification if any unexpected Java runtime exception occurs. Therefore e defensive copies of revertData byte can be used in the constructor and getter.
    i.e.
   protected RskJsonRpcRequestException(Integer code, @Nullable byte[] revertData, String message, Exception e) {
        super(message, e);
        this.code = code;
        this.revertData = (revertData != null) ? revertData.clone() : null; // Defensive copy
    }
  • @Nullable fields handling mechanism can be improved in a way to ensure more robust handling of nulls correctly to prevent NullPointerException
    i.e.
public static RskJsonRpcRequestException transactionRevertedExecutionError(
            @Nonnull String revertReason,
            @Nonnull byte[] revertData

@jurajpiar jurajpiar dismissed Vovchyk’s stale review May 15, 2024 10:10

fixed by the requester

@jurajpiar jurajpiar force-pushed the juraj/rpc_w_revert_data branch from 4055564 to 58fa81b Compare May 17, 2024 10:25
@jurajpiar jurajpiar force-pushed the juraj/rpc_w_revert_data branch from 58fa81b to fad6deb Compare May 17, 2024 10:26
Copy link

@Vovchyk Vovchyk merged commit a03fefd into master May 17, 2024
10 checks passed
@Vovchyk Vovchyk deleted the juraj/rpc_w_revert_data branch May 17, 2024 11:34
@jurajpiar jurajpiar restored the juraj/rpc_w_revert_data branch May 27, 2024 09:27
@jurajpiar jurajpiar deleted the juraj/rpc_w_revert_data branch May 27, 2024 09:47
@aeidelman aeidelman added this to the Arrowhead 6.2.0 milestone May 27, 2024
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.

7 participants