Despite the documentation, method web3.eth.accounts.signTransaction does not allow passing chainId as String

[3sGgpQ8H]

Description

Documentation for method web3.eth.accounts.signTransaction says that parameter chainId is of type String, though example in the documentation sets chainId like this:

chainId: 1

i.e. as number not as string.

Actually, when chainId is specified as string this method silently produces incorrect signature.

Expected behavior

Method web3.eth.accounts.signTransaction allows specifying chainId and either string or number and works properly in both cases.

Actual behavior

Method web3.eth.accounts.signTransaction works correctly only when chainId is specified as number, but when chainId is specified as string this method silently produces incorrect signature.

Steps to reproduce the behavior

1. Pass chainId parameter to method web3.eth.accounts.signTransaction as string.
2. Recover address from returned signed transaction.
3. Compare recovered address with address used to sign the transaction.
4. These addresses will be different.

Versions

• web3.js: 1.0.0

[princesinha19]

@3sGgpQ8H Which web3 version are you using?
I tried reproducing the bug as per steps, given by you. But It is working fine.

I am testing with web3 version 1.2.1.

[3sGgpQ8H]

1.0.0-beta.55

[princesinha19]

It is working fine with the current version which is 1.2.1. Please check once.

[3sGgpQ8H]

I looked through the code, and it seems that the actual problem was in ethereumjs-tx called from packages/web3-eth/src/signers/TransactionSigner.js. This file is missing (or moved) in 1.2.1: https://github.com/ethereum/web3.js/blob/v1.2.1/packages/web3-eth/src/signers/TransactionSigner.js , that is probably the reason why you cannot reproduce the problem on 1.2.1. Though, this file is present in 2.x branch: https://github.com/ethereum/web3.js/blob/2.x/packages/web3-eth/src/signers/TransactionSigner.js and is very similar to the file used in 1.0.0-beta.55: https://github.com/ethereum/web3.js/blob/v1.0.0-beta.55/packages/web3-eth/src/signers/TransactionSigner.js

So, 1.2.1 looks very different from both, 1.0.0-beta.55 and 2.x branch, and the fact that problem is not reproducible in 1.2.1 does not necessary mean, that it is not reproducible in 2.x.

P.S. Yes, it seems that what is fixed in 1.2.1 is not fixed in 2.x:

1.2.1 implements transaction signing itself and properly converts chainId to number: https://github.com/ethereum/web3.js/blob/v1.2.1/packages/web3-eth-accounts/src/index.js#L187

2.x delegates to ethereumjs-tx: https://github.com/ethereum/web3.js/blob/2.x/packages/web3-eth-accounts/src/Accounts.js#L162 https://github.com/ethereum/web3.js/blob/2.x/packages/web3-eth/src/signers/TransactionSigner.js#L63

So the problem is probably still reproducible on 2.x branch.

[princesinha19]

Yes sure. I will try to reproduce it using 2.0

[cgewecke]

NB: Per comment above have re-labelled.

In #3070 it's proposed to resolve some signing bugs by upgrading to ethereumjs-tx. Leaving a cross ref there so this problem isn't replicated in the new work.

[dmihal]

Just ran into this issue using Web3 + HDWalletProvider, both the most recent versions.

I'm not sure what the solution is, but there should at least be an error thrown if the chainId is passed as a string. Returning a transaction signed by an incorrect key is really strange behavior, and took quite a while of debugging to figure out.

[github-actions]

This issue is stale because it has been open 30 days with no activity. Remove stale label or comment, otherwise this issue will be closed in 7 days

[dmihal]

I believe this is still an issue and shouldn't be closed

[github-actions]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions. If you believe this was a mistake, please comment.

[dmihal]

Bumping to keep this open

[ryanio]

Just ran into this issue using Web3 + HDWalletProvider, both the most recent versions.

I'm not sure what the solution is, but there should at least be an error thrown if the chainId is passed as a string. Returning a transaction signed by an incorrect key is really strange behavior, and took quite a while of debugging to figure out.

@dmihal hm that's weird, it looks like the src uses numberToHex which should handle strings fine. could you share some code or a snippet to reproduce?

[github-actions]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions. If you believe this was a mistake, please comment.