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

Add input to CallArguments class #2075

Merged
merged 3 commits into from
Aug 29, 2023
Merged

Add input to CallArguments class #2075

merged 3 commits into from
Aug 29, 2023

Conversation

rmoreliovlabs
Copy link
Contributor

Description

  • Added input field to CallArguments
  • Added new test for RPC methods that acccept a CallArguments object as argument.

Motivation and Context

Following the investigation done in CORE-2939, regarding an issue with some of the RPC methods we use in rskj, we found that both Web3js and Geth now have a parameter called input which currently is not supported by our code, this parameter seems to behave similar to the data parameter given that Web3js automatically add input in the payload if it is missing add sets its value to a copy from data's.

How Has This Been Tested?

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:

@@ -39,6 +39,7 @@ public class Constants {
public static final byte TESTNET_CHAIN_ID = (byte) 31;
public static final byte DEVNET_CHAIN_ID = (byte) 32;
public static final byte REGTEST_CHAIN_ID = (byte) 33;
public static final String TEST_DATA = "0x603d80600c6000396000f3007c01000000000000000000000000000000000000000000000000000000006000350463c6888fa18114602d57005b6007600435028060005260206000f3";
Copy link
Contributor

Choose a reason for hiding this comment

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

it looks like this is tests specific field and only used in EthModuleTest. I'd rather move it to tests

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, I moved it to the test file.

@@ -36,6 +38,7 @@ public class CallArguments {
private String nonce;
private String chainId;
private String type; // ignore, see https://github.com/rsksmart/rskj/pull/1601
private String input;
Copy link
Contributor

Choose a reason for hiding this comment

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

do we really need this extra field? Can we reuse data and have something like this?:

    public void setInput(String input) {
        this.data = input;
    }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True, I've removed it and implemented your suggested approach.

@@ -353,4 +355,359 @@ void chainId() {
);
assertThat(eth.chainId(), is("0x21"));
}

@Test
void whenExecuteCallWithDataParameter_callExecutorWithData() {
Copy link
Contributor

Choose a reason for hiding this comment

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

nice coverage 👍 could we also test CallArguments as an unit? Eg as in here? - web3_CallArguments_toString()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Vovchyk
Vovchyk previously approved these changes Jul 14, 2023
@sonarcloud
Copy link

sonarcloud bot commented Jul 14, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

100.0% 100.0% Coverage
0.0% 0.0% Duplication

@sonarcloud
Copy link

sonarcloud bot commented Aug 29, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

100.0% 100.0% Coverage
0.0% 0.0% Duplication

warning The version of Java (11.0.16) you have used to run this analysis is deprecated and we will stop accepting it soon. Please update to at least Java 17.
Read more here

@Vovchyk Vovchyk merged commit e5b5f18 into master Aug 29, 2023
5 checks passed
@Vovchyk Vovchyk deleted the add-input-support branch August 29, 2023 15:41
@aeidelman aeidelman added this to the Fingerroot 5.3.0 milestone Oct 5, 2023
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