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.

Sign up for GitHub

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

chore: add precheck for receivers account in sendRawTransactionCheck #3310

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
+80 −3
Open

chore: add precheck for receivers account in sendRawTransactionCheck #3310

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a8fdd86
chore: add precheck for receivers account in sendRawTransactionCheck
nadezhdapopovaa Dec 5, 2024
a892247
fixup! chore: add precheck for receivers account in sendRawTransactio…
nadezhdapopovaa Dec 5, 2024
4604f1f
fixup! fixup! chore: add precheck for receivers account in sendRawTra…
nadezhdapopovaa Dec 11, 2024
3b85b1a
test: add test for receivers account precheck
nadezhdapopovaa Dec 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions packages/relay/src/lib/errors/JsonRpcError.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -238,6 +238,10 @@ export const predefined = {
code: -39013,
message: 'Invalid block range',
}),
RECEIVER_SIGNATURE_REQUIRED: new JsonRpcError({
code: -32000,
message: "Receiver's signature is required.",
}),
FILTER_NOT_FOUND: new JsonRpcError({
code: -32001,
message: 'Filter not found',
Expand Down
18 changes: 17 additions & 1 deletion packages/relay/src/lib/precheck.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@

import { ethers, Transaction } from 'ethers';
import { Logger } from 'pino';

import { prepend0x } from '../formatters';
import { MirrorNodeClient } from './clients';
import constants from './constants';
Expand Down Expand Up @@ -85,6 +84,7 @@ export class Precheck {
this.value(parsedTx);
this.gasPrice(parsedTx, networkGasPriceInWeiBars, requestDetails);
this.balance(parsedTx, mirrorAccountInfo, requestDetails);
await this.receiverAccount(parsedTx, requestDetails);
}

/**
Expand Down Expand Up @@ -376,4 +376,20 @@ export class Precheck {
throw predefined.UNSUPPORTED_TRANSACTION_TYPE;
}
}

/**
* Checks if the receiver account exists and has receiver_sig_required set to true.
* @param {Transaction} tx - The transaction.
* @param {RequestDetails} requestDetails - The request details for logging and tracking.
*/
async receiverAccount(tx: Transaction, requestDetails: RequestDetails) {
Copy link
Member

Choose a reason for hiding this comment

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

Please add test coverage for this method in the precheck.spec.ts

if (tx.to) {
const verifyAccount = await this.mirrorNodeClient.getAccount(tx.to!, requestDetails);

// When `receiver_sig_required` is set to true, the receiver's account must sign all incoming transactions.
if (verifyAccount !== null && verifyAccount.receiver_sig_required === true) {
throw predefined.RECEIVER_SIGNATURE_REQUIRED;
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Let's add another check to see if verifyAccount is null then throw a RESOURCE_NOT_FOUND and include the value of tx.to.

      throw predefined.RESOURCE_NOT_FOUND(`Cannot find receiver account: tx.to=${tx.to}`);

Copy link
Collaborator

Choose a reason for hiding this comment

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

Careful on this one since there are valid cases for tx.to not being set e.g. contract create.

Copy link
Member

Choose a reason for hiding this comment

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

Ahh yeah you're right tx.to is nullable if the transaction is a contract create transaction. Then maybe in the test make the tx.to a non-null address but mock not found. @nadezhdapopovaa please let me know if this is clear enough

Copy link
Member

Choose a reason for hiding this comment

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

Oh wait this block only executes when tx.to is not null; therefore, it is still possible for the mirror node to return a null result if tx.to is not found on the network. I'd suggest instead of checking if (verifyAccount !== null && verifyAccount.receiver_sig_required === true) for the sig required, we can pull the verifyAccount !== null out and add a new block to check the validity of the verifyAccount. If it's null then throw RESOURCE_NOT_FOUND like how we did with the tx.from.

}
}
9 changes: 9 additions & 0 deletions packages/relay/tests/lib/eth/eth_sendRawTransaction.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,7 @@ describe('@ethSendRawTransaction eth_sendRawTransaction spec', async function ()
let clock: any;
const accountAddress = '0x9eaee9E66efdb91bfDcF516b034e001cc535EB57';
const accountEndpoint = `accounts/${accountAddress}${NO_TRANSACTIONS}`;
const receiverAccountEndpoint = `accounts/${ACCOUNT_ADDRESS_1}${NO_TRANSACTIONS}`;
const gasPrice = '0xad78ebc5ac620000';
const transactionIdServicesFormat = '[email protected]';
const transactionId = '0.0.902-1684375868-230217103';
Expand Down Expand Up @@ -127,6 +128,13 @@ describe('@ethSendRawTransaction eth_sendRawTransaction spec', async function ()
balance: Hbar.from(100_000_000_000, HbarUnit.Hbar).to(HbarUnit.Tinybar),
},
};
const RECEIVER_ACCOUNT_RES = {
account: ACCOUNT_ADDRESS_1,
balance: {
balance: Hbar.from(100_000_000_000, HbarUnit.Hbar).to(HbarUnit.Tinybar),
},
receiver_sig_required: false,
};
const useAsyncTxProcessing = ConfigService.get('USE_ASYNC_TX_PROCESSING') as boolean;

beforeEach(() => {
Expand All @@ -135,6 +143,7 @@ describe('@ethSendRawTransaction eth_sendRawTransaction spec', async function ()
sdkClientStub = sinon.createStubInstance(SDKClient);
sinon.stub(hapiServiceInstance, 'getSDKClient').returns(sdkClientStub);
restMock.onGet(accountEndpoint).reply(200, ACCOUNT_RES);
restMock.onGet(receiverAccountEndpoint).reply(200, RECEIVER_ACCOUNT_RES);
restMock.onGet(networkExchangeRateEndpoint).reply(200, mockedExchangeRate);
});

Expand Down
3 changes: 2 additions & 1 deletion packages/relay/tests/lib/openrpc.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ import {
overrideEnvsInMochaDescribe,
signedTransactionHash,
} from '../helpers';
import { NOT_FOUND_RES } from './eth/eth-config';
import { CONTRACT_RESULT_MOCK, NOT_FOUND_RES } from './eth/eth-config';

const logger = pino();
const registry = new Registry();
Expand Down Expand Up @@ -229,6 +229,7 @@ describe('Open RPC Specification', function () {
mock.onGet(`accounts/${defaultContractResults.results[1].from}?transactions=false`).reply(200);
mock.onGet(`accounts/${defaultContractResults.results[0].to}?transactions=false`).reply(200);
mock.onGet(`accounts/${defaultContractResults.results[1].to}?transactions=false`).reply(200);
mock.onGet(`accounts/${CONTRACT_RESULT_MOCK.from}?transactions=false`).reply(200, CONTRACT_RESULT_MOCK);
mock.onGet(`contracts/${defaultContractResults.results[0].from}`).reply(404, NOT_FOUND_RES);
mock.onGet(`contracts/${defaultContractResults.results[1].from}`).reply(404, NOT_FOUND_RES);
mock.onGet(`contracts/${defaultContractResults.results[0].to}`).reply(200);
Expand Down
49 changes: 48 additions & 1 deletion packages/server/tests/acceptance/rpc_batch1.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,14 @@ import Constants from '@hashgraph/json-rpc-relay/dist/lib/constants';
// Errors and constants from local resources
import { predefined } from '@hashgraph/json-rpc-relay/dist/lib/errors/JsonRpcError';
import { RequestDetails } from '@hashgraph/json-rpc-relay/dist/lib/types';
import { FileInfo, FileInfoQuery, Hbar, TransferTransaction } from '@hashgraph/sdk';
import {
AccountCreateTransaction,
FileInfo,
FileInfoQuery,
Hbar,
PrivateKey,
TransferTransaction,
} from '@hashgraph/sdk';
import { expect } from 'chai';
import { ethers } from 'ethers';

Expand Down Expand Up @@ -1586,6 +1593,46 @@ describe('@api-batch-1 RPC Server Acceptance Tests', function () {
const error = predefined.NONCE_TOO_LOW(nonce, nonce + 1);
await Assertions.assertPredefinedRpcError(error, sendRawTransaction, true, relay, [signedTx, requestDetails]);
});

it('should fail "eth_sendRawTransaction" if receiver\'s account has receiver_sig_required enabled', async function () {
const newPrivateKey = PrivateKey.generateED25519();
const newAccount = await new AccountCreateTransaction()
.setKey(newPrivateKey.publicKey)
.setInitialBalance(100)
.setReceiverSignatureRequired(true)
.freezeWith(servicesNode.client)
.sign(newPrivateKey);

const transaction = await newAccount.execute(servicesNode.client);
const receipt = await transaction.getReceipt(servicesNode.client);

if (!receipt.accountId) {
throw new Error('Failed to create new account - accountId is null');
}

const toAddress = Utils.idToEvmAddress(receipt.accountId.toString());
const tx = {
nonce: await accounts[0].wallet.getNonce(),
chainId: CHAIN_ID,
to: toAddress,
from: accounts[0].address,
value: '0x2E90EDD000',
gasLimit: defaultGasLimit,
accessList: [],
maxPriorityFeePerGas: defaultGasPrice,
maxFeePerGas: defaultGasPrice,
};

const signedTx = await accounts[0].wallet.signTransaction(tx);
await new Promise((r) => setTimeout(r, 3000));

const error = predefined.RECEIVER_SIGNATURE_REQUIRED;

await Assertions.assertPredefinedRpcError(error, sendRawTransaction, false, relay, [
signedTx,
requestDetails,
]);
});
Comment on lines +1597 to +1635
Copy link
Member

Choose a reason for hiding this comment

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

Great test! Let's add another test to ensure it works fine when receiver.to has receiver_sig_required disabled.

Additionally, if we decide to throw a RESOURCE_NOT_FOUND error for tx.to, we should include another test that uses an address not present in the system to verify that it blocks the request.

});

it('@release should execute "eth_getTransactionByHash" for existing transaction', async function () {
Expand Down
Loading