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

Use input as data in eth_sendTransaction #300

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

Use input as data in eth_sendTransaction #300

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
90baaf9
Use input as data in eth_sendTransaction
jiexi Apr 10, 2024
fa1f16a
update spec
jiexi Apr 11, 2024
ac17d2a
only add data field if necessary
jiexi Apr 11, 2024
7f3cf6d
lint
jiexi Apr 26, 2024
b47e1ba
set data and input
jiexi Apr 26, 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
98 changes: 98 additions & 0 deletions src/wallet.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,103 @@ describe('wallet', () => {
);
});

it('processes transaction with data field but without input field', async () => {
const { engine } = createTestSetup();
const getAccounts = async () => testAddresses.slice(0, 2);
const witnessedTxParams: TransactionParams[] = [];
const processTransaction = async (_txParams: TransactionParams) => {
witnessedTxParams.push(_txParams);
return testTxHash;
};
engine.push(createWalletMiddleware({ getAccounts, processTransaction }));
const txParams = {
from: testAddresses[0],
data: '0x0',
};

const payload = { method: 'eth_sendTransaction', params: [txParams] };
const sendTxResponse = await pify(engine.handle).call(engine, payload);
const sendTxResult = sendTxResponse.result;
expect(sendTxResult).toBeDefined();
expect(sendTxResult).toStrictEqual(testTxHash);
expect(witnessedTxParams).toHaveLength(1);
expect(witnessedTxParams[0]).toStrictEqual(txParams);
});

it('processes transaction with input field but without data field', async () => {
const { engine } = createTestSetup();
const getAccounts = async () => testAddresses.slice(0, 2);
const witnessedTxParams: TransactionParams[] = [];
const processTransaction = async (_txParams: TransactionParams) => {
witnessedTxParams.push(_txParams);
return testTxHash;
};
engine.push(createWalletMiddleware({ getAccounts, processTransaction }));
const txParams = {
from: testAddresses[0],
input: '0x0',
};

const payload = { method: 'eth_sendTransaction', params: [txParams] };
const sendTxResponse = await pify(engine.handle).call(engine, payload);
const sendTxResult = sendTxResponse.result;
expect(sendTxResult).toBeDefined();
expect(sendTxResult).toStrictEqual(testTxHash);
expect(witnessedTxParams).toHaveLength(1);
expect(witnessedTxParams[0]).toStrictEqual({
from: txParams.from,
data: txParams.input,
});
});

it('processes transaction with matching input and data field', async () => {
const { engine } = createTestSetup();
const getAccounts = async () => testAddresses.slice(0, 2);
const witnessedTxParams: TransactionParams[] = [];
const processTransaction = async (_txParams: TransactionParams) => {
witnessedTxParams.push(_txParams);
return testTxHash;
};
engine.push(createWalletMiddleware({ getAccounts, processTransaction }));
const txParams = {
from: testAddresses[0],
data: '0x0',
input: '0x0',
};

const payload = { method: 'eth_sendTransaction', params: [txParams] };
const sendTxResponse = await pify(engine.handle).call(engine, payload);
const sendTxResult = sendTxResponse.result;
expect(sendTxResult).toBeDefined();
expect(sendTxResult).toStrictEqual(testTxHash);
expect(witnessedTxParams).toHaveLength(1);
expect(witnessedTxParams[0]).toStrictEqual({
from: txParams.from,
data: txParams.data,
});
});

it('throws when input and data fields are both defined but do not match', async () => {
const { engine } = createTestSetup();
const getAccounts = async () => testAddresses.slice(0, 2);
const witnessedTxParams: TransactionParams[] = [];
const processTransaction = async (_txParams: TransactionParams) => {
witnessedTxParams.push(_txParams);
return testTxHash;
};
engine.push(createWalletMiddleware({ getAccounts, processTransaction }));
const txParams = {
from: testAddresses[0],
data: '0x0',
input: '0x1',
};

const payload = { method: 'eth_sendTransaction', params: [txParams] };
await expect(pify(engine.handle).call(engine, payload)).rejects.toThrow(
new Error('Invalid input.'),
);
});

it('should not override other request params', async () => {
const { engine } = createTestSetup();
const getAccounts = async () => testAddresses.slice(0, 2);
Expand All @@ -129,6 +226,7 @@ describe('wallet', () => {
const txParams = {
from: testAddresses[0],
to: testAddresses[1],
data: '0x0',
};

const payload = { method: 'eth_sendTransaction', params: [txParams] };
Expand Down
21 changes: 17 additions & 4 deletions src/wallet.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -159,10 +159,23 @@
throw rpcErrors.invalidInput();
}

const params = req.params[0] as TransactionParams | undefined;
const txParams: TransactionParams = {
...params,
from: await validateAndNormalizeKeyholder(params?.from || '', req),
const params = req.params[0] as
| (TransactionParams & {
input?: string;
data?: string;
})
| undefined;

const { from, data, input, ...restParams } = params || {};
if (data && input && data !== input) {
throw rpcErrors.invalidInput();
}

const txParamsData = data || input;
const txParams: TransactionParams & { data?: string } = {
...restParams,
...(txParamsData ? { data: txParamsData } : undefined),
Copy link
Contributor

@legobeat legobeat Apr 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

For initial maximal compatibility, how about setting both input and data, removing one in a follow-up release?
After this change, input is now a special case filtered out from output while any other free-name fields are still passed through. This is a bit counter-intuitive and might be good to do in a two-step process?

Suggested change
...(txParamsData ? { data: txParamsData } : undefined),
...(
txParamsData
? { data: txParamsData, input: txParamsData }
: undefined
),

Copy link
Author

Choose a reason for hiding this comment

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

done here b47e1ba

Copy link
Contributor

Choose a reason for hiding this comment

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

are we sure we want to set both? the original geth PR says:

bail if both are set

Copy link
Contributor

@legobeat legobeat Apr 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Should we really modify the params at all? Considering there are other implementations which may work differently. Maybe the validation that both aren't provided with differing values is enough for the sake of this package?

Copy link
Author

Choose a reason for hiding this comment

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

The only consumer of this should be TransactionController, which drops input (and other unexpected fields) as one of the first steps of transaction processing

Copy link
Contributor

@legobeat legobeat Apr 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This is a public package with external dependents who rely on the current API, though, which also need to be considered.
https://www.npmjs.com/browse/depended/@metamask/eth-json-rpc-middleware

There are likely other dependents as well, which don't necessarily show up on npmjs.

Copy link
Contributor

Choose a reason for hiding this comment

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

MetaMask is also actively advising existing users of deprecated web3-provider-engine to migrate to @metamask/eth-json-rpc-middleware.

from: await validateAndNormalizeKeyholder(from || '', req),
};
res.result = await processTransaction(txParams, req);
}
Expand Down Expand Up @@ -458,7 +471,7 @@
*
* @param address - The address to validate and normalize.
* @param req - The request object.
* @returns {string} - The normalized address, if valid. Otherwise, throws

Check warning on line 474 in src/wallet.ts

View workflow job for this annotation

GitHub Actions / Build, lint, and test / Lint (16.x) 

There must be no hyphen before @returns description

Check warning on line 474 in src/wallet.ts

View workflow job for this annotation

GitHub Actions / Build, lint, and test / Lint (16.x) 

JSDoc description does not satisfy the regex pattern

Check warning on line 474 in src/wallet.ts

View workflow job for this annotation

GitHub Actions / Build, lint, and test / Lint (16.x) 

Types are not permitted on @returns

Check warning on line 474 in src/wallet.ts

View workflow job for this annotation

GitHub Actions / Build, lint, and test / Lint (18.x) 

There must be no hyphen before @returns description

Check warning on line 474 in src/wallet.ts

View workflow job for this annotation

GitHub Actions / Build, lint, and test / Lint (18.x) 

JSDoc description does not satisfy the regex pattern

Check warning on line 474 in src/wallet.ts

View workflow job for this annotation

GitHub Actions / Build, lint, and test / Lint (18.x) 

Types are not permitted on @returns

Check warning on line 474 in src/wallet.ts

View workflow job for this annotation

GitHub Actions / Build, lint, and test / Lint (20.x) 

There must be no hyphen before @returns description

Check warning on line 474 in src/wallet.ts

View workflow job for this annotation

GitHub Actions / Build, lint, and test / Lint (20.x) 

JSDoc description does not satisfy the regex pattern

Check warning on line 474 in src/wallet.ts

View workflow job for this annotation

GitHub Actions / Build, lint, and test / Lint (20.x) 

Types are not permitted on @returns
* an error
*/
async function validateAndNormalizeKeyholder(
Expand Down
Loading