Skip to content

Commit

Permalink
fix: fixed requests throw error when both data and input fields are p…
Browse files Browse the repository at this point in the history 
…resent in transaction object (hashgraph#2551)

* fix: fixed callData mistakenly accepts call.value

Signed-off-by: Logan Nguyen <logan.nguyen@swirldslabs.com>

* fix: fixed requests throw error when both data and input fields are present in transaction object

Signed-off-by: Logan Nguyen <logan.nguyen@swirldslabs.com>

---------

Signed-off-by: Logan Nguyen <logan.nguyen@swirldslabs.com>
  • Loading branch information
@quiet-node
quiet-node authored Jun 3, 2024
1 parent d0a1e25 commit 084547f
Show file tree
Hide file tree
Showing 5 changed files with 78 additions and 69 deletions.
31 changes: 16 additions & 15 deletions packages/relay/src/lib/eth.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -561,16 +561,19 @@ export class EthImpl implements Eth {
_blockParam: string | null,
requestIdPrefix?: string,
): Promise<string | JsonRpcError> {
this.logger.trace(
`${requestIdPrefix} estimateGas(transaction=${JSON.stringify(transaction)}, _blockParam=${_blockParam})`,
);
const callData = transaction.data ? transaction.data : transaction.input;
const callDataSize = callData ? callData.length : 0;

if (transaction?.data?.length >= constants.FUNCTION_SELECTOR_CHAR_LENGTH) {
if (callDataSize >= constants.FUNCTION_SELECTOR_CHAR_LENGTH) {
this.ethExecutionsCounter
.labels(EthImpl.ethEstimateGas, transaction.data.substring(0, constants.FUNCTION_SELECTOR_CHAR_LENGTH))
.labels(EthImpl.ethEstimateGas, callData.substring(0, constants.FUNCTION_SELECTOR_CHAR_LENGTH))
.inc();
}

this.logger.trace(
`${requestIdPrefix} estimateGas(transaction=${JSON.stringify(transaction)}, _blockParam=${_blockParam})`,
);

this.contractCallFormat(transaction);
let gas = EthImpl.gasTxBaseCost;
try {
Expand Down Expand Up @@ -653,16 +656,14 @@ export class EthImpl implements Eth {
transaction.gas = parseInt(transaction.gas);
}

if (transaction.data && transaction.input) {
throw predefined.INVALID_ARGUMENTS('Cannot accept both input and data fields. Use only one.');
}

// Support either data or input. https://ethereum.github.io/execution-apis/api-documentation/ lists input but many EVM tools still use data.
// We chose in the mirror node to use data field as the correct one, however for us to be able to support all tools,
// we have to modify transaction object, so that it complies with the mirror node.
// That means that, if input field is passed, but data is not, we have to copy one value to the other.
// For optimization purposes, we can rid of the input property or replace it with empty string.
if (transaction.input && transaction.data === undefined) {
// That means that, if input field is passed, but data is not, we have to copy value of input to the data to comply with mirror node.
// The second scenario occurs when both the data and input fields are present but hold different values.
// In this case, the value in the input field should be the one used for consensus based on this resource https://github.com/ethereum/execution-apis/blob/main/tests/eth_call/call-contract.io
// Eventually, for optimization purposes, we can rid of the input property or replace it with empty string.
if ((transaction.input && transaction.data === undefined) || (transaction.input && transaction.data)) {
transaction.data = transaction.input;
delete transaction.input;
}
Expand Down Expand Up @@ -1549,7 +1550,7 @@ export class EthImpl implements Eth {
* @param blockParam
*/
async call(call: any, blockParam: string | object | null, requestIdPrefix?: string): Promise<string | JsonRpcError> {
const callData = call.data ? call.data : call.value;
const callData = call.data ? call.data : call.input;
// log request
this.logger.trace(
`${requestIdPrefix} call({to=${call.to}, from=${call.from}, data=${callData}, gas=${call.gas}, ...}, blockParam=${blockParam})`,
Expand All @@ -1558,9 +1559,9 @@ export class EthImpl implements Eth {
const callDataSize = callData ? callData.length : 0;
this.logger.trace(`${requestIdPrefix} call data size: ${callDataSize}, gas: ${call.gas}`);
// metrics for selector
if (call.data?.length >= constants.FUNCTION_SELECTOR_CHAR_LENGTH)
if (callDataSize >= constants.FUNCTION_SELECTOR_CHAR_LENGTH)
this.ethExecutionsCounter
.labels(EthImpl.ethCall, call.data.substring(0, constants.FUNCTION_SELECTOR_CHAR_LENGTH))
.labels(EthImpl.ethCall, callData.substring(0, constants.FUNCTION_SELECTOR_CHAR_LENGTH))
.inc();

const blockNumberOrTag = await this.extractBlockNumberOrTag(blockParam, requestIdPrefix);
Expand Down
27 changes: 18 additions & 9 deletions packages/relay/tests/lib/eth/eth_call.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -817,17 +817,26 @@ describe('@ethCall Eth Call spec', async function () {
expect(transaction.gas).to.equal(50000);
});

it('should throw an error if both input and data fields are provided', () => {
it('should accepts both input and data fields but copy value of input field to data field', () => {
const inputValue = 'input value';
const dataValue = 'data value';
const transaction = {
input: 'input data',
data: 'some data',
input: inputValue,
data: dataValue,
};
try {
ethImpl.contractCallFormat(transaction);
} catch (error) {
expect(error.code).eq(-32000);
expect(error.message).eq('Invalid arguments: Cannot accept both input and data fields. Use only one.');
}
ethImpl.contractCallFormat(transaction);
expect(transaction.data).to.eq(inputValue);
expect(transaction.data).to.not.eq(dataValue);
expect(transaction.input).to.be.undefined;
});

it('should not modify transaction if only data field is present', () => {
const dataValue = 'data value';
const transaction = {
data: dataValue,
};
ethImpl.contractCallFormat(transaction);
expect(transaction.data).to.eq(dataValue);
});

it('should copy input to data if input is provided but data is not', () => {
Expand Down
21 changes: 12 additions & 9 deletions packages/relay/tests/lib/eth/eth_estimateGas.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -449,21 +449,24 @@ describe('@ethEstimateGas Estimate Gas spec', async function () {
expect(transaction.gas).to.eq(14250000);
});

it('should throw on estimateGas precheck', async function () {
it('should accepts both input and data fields but copy value of input field to data field', () => {
const inputValue = 'input value';
const dataValue = 'data value';
const transaction = {
from: '0x05fba803be258049a27b820088bab1cad2058871',
data: '0x',
input: '0x',
input: inputValue,
data: dataValue,
value: '0xA186B8E9800',
gasPrice: '0xF4240',
gas: '0xd97010',
};

try {
ethImpl.contractCallFormat(transaction);
} catch (error) {
expect(error.code).to.equal(-32000);
expect(error.message).to.equal('Invalid arguments: Cannot accept both input and data fields. Use only one.');
}
ethImpl.contractCallFormat(transaction);
expect(transaction.data).to.eq(inputValue);
expect(transaction.data).to.not.eq(dataValue);
expect(transaction.input).to.be.undefined;
expect(transaction.value).to.eq(1110);
expect(transaction.gasPrice).to.eq(1000000);
expect(transaction.gas).to.eq(14250000);
});
});
37 changes: 18 additions & 19 deletions packages/server/tests/acceptance/rpc_batch2.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -391,25 +391,24 @@ describe('@api-batch-2 RPC Server Acceptance Tests', function () {
expect(res).to.not.be.equal('0x0');
});

it('should throw on "eth_estimateGas" with both input and data fields present in the txObject', async function () {
try {
await relay.call(
RelayCalls.ETH_ENDPOINTS.ETH_ESTIMATE_GAS,
[
{
from: '0x114f60009ee6b84861c0cdae8829751e517bc4d7',
to: '0xae410f34f7487e2cd03396499cebb09b79f45d6e',
value: '0xa688906bd8b00000',
gas: '0xd97010',
input: '0x',
data: '0x',
},
],
requestId,
);
} catch (e) {
expect(e).to.equal(predefined.INVALID_ARGUMENTS('Cannot accept both input and data fields. Use only one.'));
}
it('should execute "eth_estimateGas" with both input and data fields present in the txObject', async function () {
const res = await relay.call(
RelayCalls.ETH_ENDPOINTS.ETH_ESTIMATE_GAS,
[
{
from: '0x114f60009ee6b84861c0cdae8829751e517bc4d7',
to: '0xae410f34f7487e2cd03396499cebb09b79f45d6e',
value: '0xa688906bd8b00000',
gas: '0xd97010',
input: '0x',
data: '0x',
},
],
requestId,
);
expect(res).to.contain('0x');
expect(res).to.not.be.equal('0x');
expect(res).to.not.be.equal('0x0');
});
});

Expand Down
31 changes: 14 additions & 17 deletions packages/server/tests/acceptance/rpc_batch3.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -294,6 +294,20 @@ describe('@api-batch-3 RPC Server Acceptance Tests', function () {
expect(res).to.eq(BASIC_CONTRACT_PING_RESULT);
});

it('should execute "eth_call" with both data and input fields', async function () {
const callData = {
from: accounts[0].address,
to: basicContractAddress,
data: BASIC_CONTRACT_PING_CALL_DATA,
input: BASIC_CONTRACT_PING_CALL_DATA,
};

// deploymentBlockNumber to HEX
const block = numberTo0x(deploymentBlockNumber);
const res = await relay.call(RelayCall.ETH_ENDPOINTS.ETH_CALL, [callData, { blockNumber: block }], requestId);
expect(res).to.eq(BASIC_CONTRACT_PING_RESULT);
});

it('should fail to execute "eth_call" with wrong block tag', async function () {
const callData = {
from: accounts[0].address,
Expand Down Expand Up @@ -333,23 +347,6 @@ describe('@api-batch-3 RPC Server Acceptance Tests', function () {
await Assertions.assertPredefinedRpcError(errorType, relay.call, false, relay, args);
});

it('should fail to execute "eth_call" with both data and input fields', async function () {
const callData = {
from: accounts[0].address,
to: basicContractAddress,
data: BASIC_CONTRACT_PING_CALL_DATA,
};

// deploymentBlockNumber to HEX
const block = numberTo0x(deploymentBlockNumber);
try {
await relay.call(RelayCall.ETH_ENDPOINTS.ETH_CALL, [callData, { blockNumber: block }], requestId);
} catch (error) {
expect(error.code).eq(-32000);
expect(error.message).eq('Invalid arguments: Cannot accept both input and data fields. Use only one.');
}
});

it('should fail to execute "eth_call" with wrong block number object', async function () {
const callData = {
from: accounts[0].address,
Expand Down

0 comments on commit 084547f

Please sign in to comment.