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

JSON RPC methods converting args more consistently #1659

Merged

Conversation

rmoreliovlabs
Copy link
Contributor

There are some JSON RPC methods that process transactions with data provided as JSON, like eth_sendTransaction, eth_call, eth_estimateGas (the method eth_sendRawTransaction receives the encoded transaction in an hexadecimal string)

Description

They are some JSON RPC methods that process transactions with data provided as JSON, like eth_sendTransaction, eth_call, eth_estimateGas (the method eth_sendRawTransaction receives the encoded transaction in an hexadecimal string)

Let's take one of this arguments: gas

It comes as a string, like "1000000", "0xa000", but also "a000" is accepted in SOME methods (not all, others reject this value without 0x prefix). This is the inconsistency.

In the method EthModule.call

public String call(Web3.CallArguments args, String bnOrId)

an internal method is used: callConstant:

    private ProgramResult callConstant(Web3.CallArguments args, Block executionBlock) {
        CallArgumentsToByteArray hexArgs = new CallArgumentsToByteArray(args);
        return reversibleTransactionExecutor.executeTransaction(
                executionBlock,
                executionBlock.getCoinbase(),
                hexArgs.getGasPrice(),
                hexArgs.getGasLimit(),
                hexArgs.getToAddress(),
                hexArgs.getValue(),
                hexArgs.getData(),
                hexArgs.getFromAddress()
        );
    }

The args.gas field is string. The hexArgs.getGasLimit() code is:

    public byte[] getGasLimit() {
        // maxGasLimit value is 100000000000000
        String maxGasLimit = "0x5AF3107A4000";
        byte[] gasLimit = stringHexToByteArray(maxGasLimit);
        if (args.gas != null && args.gas.length() != 0) {
            gasLimit = stringHexToByteArray(args.gas);
        }

        return gasLimit;
    }

But NOTABLY, the stringHexToByteArray method converts the numeric string (ie "1000") AS HEXADECIMAL:

    public static byte[] stringHexToByteArray(String x) {
        String result = x;
        if (x.startsWith("0x")) {
            result = x.substring(2);
        }
        if (result.length() % 2 != 0) {
            result = "0" + result;
        }
        return Hex.decode(result);
    }

So, the result is a byte array with { 0x10, 0x00 }

This contrasts with the send transaction behavior. eth_sendTransaction receives the same args but it processes them differently. Internally, it executes EthModuleTransactionBase.sendTransaction, that use TypeConverter.stringNumberAsBigInt(args.gas) to convert the gas string.

This is the code of the conversion method:

    public static BigInteger stringNumberAsBigInt(String input) {
        if (input.startsWith("0x")) {
            return TypeConverter.stringHexToBigInteger(input);
        } else {
            return TypeConverter.stringDecimalToBigInteger(input);
        }
    }

IT TAKES INTO ACCOUNT THE PREFIX 0x, so, "1000" will be parse AS DECIMAL, not hexadecimal.

This inconsistency does not affect the consensus, and apparently only affects non-critical methods (query methods).

TO CHECK: other fields conversion, like args.gasPrice

Extracted from: #1458

Motivation and Context

It will add more consistency at the moment of parsing args from our RPC methods

How Has This Been Tested?

Yes added some tests and also tested on postman

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:

@rmoreliovlabs
Copy link
Contributor Author

pipeline:run

@rmoreliovlabs rmoreliovlabs force-pushed the more-consistent-parsing-for-hex-or-numbers-args-in-endpoints branch from bea7685 to 5031e8b Compare November 22, 2021 13:59
Vovchyk
Vovchyk previously approved these changes Nov 23, 2021
@rmoreliovlabs rmoreliovlabs force-pushed the more-consistent-parsing-for-hex-or-numbers-args-in-endpoints branch 8 times, most recently from 910cd05 to 98f847c Compare November 27, 2021 03:20
@rmoreliovlabs rmoreliovlabs force-pushed the more-consistent-parsing-for-hex-or-numbers-args-in-endpoints branch from 9fea3c0 to 5fe2113 Compare November 29, 2021 17:06
@rmoreliovlabs rmoreliovlabs force-pushed the more-consistent-parsing-for-hex-or-numbers-args-in-endpoints branch 2 times, most recently from f132b52 to 94e1819 Compare December 2, 2021 14:12
@rmoreliovlabs rmoreliovlabs requested a review from a team as a code owner December 2, 2021 14:12
@Vovchyk
Copy link
Contributor

Vovchyk commented Dec 2, 2021

pipeline:run

1 similar comment
@Vovchyk
Copy link
Contributor

Vovchyk commented Dec 2, 2021

pipeline:run

@rmoreliovlabs rmoreliovlabs force-pushed the more-consistent-parsing-for-hex-or-numbers-args-in-endpoints branch from 94e1819 to f2638a4 Compare December 2, 2021 16:03
@rmoreliovlabs rmoreliovlabs force-pushed the more-consistent-parsing-for-hex-or-numbers-args-in-endpoints branch from f2638a4 to 7e14047 Compare December 13, 2021 17:11
@rmoreliovlabs
Copy link
Contributor Author

pipeline:run

@rmoreliovlabs rmoreliovlabs force-pushed the more-consistent-parsing-for-hex-or-numbers-args-in-endpoints branch from 7e14047 to 71cd4f0 Compare December 14, 2021 13:46
Validating hexadecimal number
@rmoreliovlabs rmoreliovlabs force-pushed the more-consistent-parsing-for-hex-or-numbers-args-in-endpoints branch from 71cd4f0 to 60f9f4f Compare December 14, 2021 14:10
@sonarcloud
Copy link

sonarcloud bot commented Dec 14, 2021

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 5 Code Smells

95.2% 95.2% Coverage
0.0% 0.0% Duplication

@Vovchyk Vovchyk merged commit 428d8ad into master Dec 14, 2021
@Vovchyk Vovchyk deleted the more-consistent-parsing-for-hex-or-numbers-args-in-endpoints branch December 14, 2021 14:45
@aeidelman aeidelman added this to the Iris v3.3.0 milestone Mar 23, 2022
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.

5 participants