add tx.chainId if tx.common.chainId not provided (#4293)

* add tx.chainId if tx.common not provided

* tx.common.customChain.chainId checks

* test: dnt look for chainId if common given

* test: get ChainId if common not provided

* change log

* customChain is not optional in common

* tx.common safety checks for runtime

* formatting / readability

* doc update

Author: jdevcs

CHANGELOG.md

@@ -450,3 +450,5 @@ Released with 1.0.0-beta.37 code base.
 ## [Unreleased]
 
 ## [1.5.4]
+### Changed
+- Not considering `tx.chainId` if `tx.common.customChain.chainId` is provided for `web3.eth.accounts.signTransaction` function (#4293)

docs/web3-eth-accounts.rst

@@ -151,7 +151,7 @@ Parameters
 
 1. ``tx`` - ``Object``: The transaction object as follows:
     - ``nonce`` - ``String``: (optional) The nonce to use when signing this transaction. Default will use :ref:`web3.eth.getTransactionCount() <eth-gettransactioncount>`.
-    - ``chainId`` - ``String``: (optional) The chain id to use when signing this transaction. Default will use :ref:`web3.eth.net.getId() <net-getid>`.
+    - ``chainId`` - ``String``: (optional) The chain id to use when signing this transaction. Default will use :ref:`web3.eth.net.getId() <net-getid>`. Web3 will ignore this field if `common.customChain.chainId` is provided.
     - ``to`` - ``String``: (optional) The recevier of the transaction, can be empty when deploying a contract.
     - ``data`` - ``String``: (optional) The call data of the transaction, can be empty for simple value transfers.
     - ``value`` - ``String``: (optional) The value of the transaction in wei.

packages/web3-eth-accounts/src/index.js

@@ -39,6 +39,10 @@ var isNot = function(value) {
     return (typeof value === 'undefined') || value === null;
 };
 
+var isExist = function(value) {
+    return (typeof value !== 'undefined') && value !== null;
+};
+
 var Accounts = function Accounts() {
     var _this = this;
 
@@ -154,6 +158,27 @@ Accounts.prototype.signTransaction = function signTransaction(tx, privateKey, ca
         return Promise.reject(error);
     }
 
+    if (isExist(tx.common) && isNot(tx.common.customChain)) {
+        error = new Error('If tx.common is provided it must have tx.common.customChain');
+
+        callback(error);
+        return Promise.reject(error);
+    }
+
+    if (isExist(tx.common) && isNot(tx.common.customChain.chainId)) {
+        error = new Error('If tx.common is provided it must have tx.common.customChain and tx.common.customChain.chainId');
+
+        callback(error);
+        return Promise.reject(error);
+    }
+
+    if (isExist(tx.common) && isExist(tx.common.customChain.chainId) && isExist(tx.chainId) && tx.chainId !== tx.common.customChain.chainId) {
+        error = new Error('Chain Id doesnt match in tx.chainId tx.common.customChain.chainId');
+
+        callback(error);
+        return Promise.reject(error);
+    }
+
     function signed(tx) {
         const error = _validateTransactionForSigning(tx);
 
@@ -264,21 +289,25 @@ Accounts.prototype.signTransaction = function signTransaction(tx, privateKey, ca
 
     // Otherwise, get the missing info from the Ethereum Node
     return Promise.all([
-        isNot(tx.chainId) ? _this._ethereumCall.getChainId() : tx.chainId,
+        ((isNot(tx.common) || isNot(tx.common.customChain.chainId)) ? //tx.common.customChain.chainId is not optional inside tx.common if tx.common is provided
+            ( isNot(tx.chainId) ? _this._ethereumCall.getChainId() : tx.chainId) 
+            : undefined ), 
         isNot(tx.nonce) ? _this._ethereumCall.getTransactionCount(_this.privateKeyToAccount(privateKey).address) : tx.nonce,
         isNot(hasTxSigningOptions) ? _this._ethereumCall.getNetworkId() : 1,
         _handleTxPricing(_this, tx)
     ]).then(function(args) {
-        if (isNot(args[0]) || isNot(args[1]) || isNot(args[2]) || isNot(args[3])) {
+        const [txchainId, txnonce, txnetworkId, txgasInfo] = args; 
+
+        if ( (isNot(txchainId) && isNot(tx.common) && isNot(tx.common.customChain.chainId)) || isNot(txnonce) || isNot(txnetworkId) || isNot(txgasInfo)) {
             throw new Error('One of the values "chainId", "networkId", "gasPrice", or "nonce" couldn\'t be fetched: ' + JSON.stringify(args));
         }
-    
+
     return signed({
             ...tx,
-            chainId: args[0],
-            nonce: args[1],
-            networkId: args[2],
-            ...args[3] // Will either be gasPrice or maxFeePerGas and maxPriorityFeePerGas
+            ... ((isNot(tx.common) || isNot(tx.common.customChain.chainId) ) ? {chainId: txchainId}:{}), // if common.customChain.chainId is provided no need to add tx.chainId
+            nonce: txnonce,
+            networkId: txnetworkId,
+            ...txgasInfo // Will either be gasPrice or maxFeePerGas and maxPriorityFeePerGas
         });
     });
 };

test/e2e.method.signing.js

@@ -281,7 +281,13 @@ describe('transaction and message signing [ @E2E ]', function() {
             gasLimit: web3.utils.toHex(21000),
             gasPrice: web3.utils.toHex(web3.utils.toWei('10', 'gwei')),
             chain: "ropsten",
-            common: {},
+            common: {
+                customChain: {
+                name: 'custom-network',
+                networkId: 1,
+                chainId: 1,
+                }
+            },
             hardfork: "istanbul"
         };
 

test/eth.accounts.signTransaction.js

@@ -878,7 +878,42 @@ describe("eth", function () {
                 });
 
 
-                it("signTransaction will call for chainId", function(done) {
+                it("signTransaction should not call for chainId if common.customChain.chainId provided", function(done) {
+                    var provider = new FakeHttpProvider();
+                    var web3 = new Web3(provider);
+
+                    provider.injectResult(
+                        test.transaction.common.hardfork === 'london' ?
+                        postEip1559Block:
+                        preEip1559Block
+                    );
+                    provider.injectValidation(function (payload) {
+                        assert.equal(payload.jsonrpc, '2.0');
+                        assert.equal(payload.method, 'eth_getBlockByNumber');
+                        assert.deepEqual(payload.params, ['latest', false]);
+                    });
+
+                    var ethAccounts = new Accounts(web3);
+
+                    var testAccount = ethAccounts.privateKeyToAccount(test.privateKey);
+                    assert.equal(testAccount.address, test.address);
+
+                    var transaction = clone(test.transaction);
+                    delete transaction.chainId;
+                    testAccount.signTransaction(transaction)
+                    .then(function (tx) {
+                        assert.isObject(tx);
+                        assert.isString(tx.rawTransaction);
+
+                        done();
+                    })
+                    .catch(e => {
+                        console.log(i, e)
+                        done(e);
+                    });
+                });
+
+                it("signTransaction should call for chainId if common.customChain.chainId not provided", function(done) {
                     var provider = new FakeHttpProvider();
                     var web3 = new Web3(provider);
 
@@ -888,6 +923,7 @@ describe("eth", function () {
                         assert.equal(payload.method, 'eth_chainId');
                         assert.deepEqual(payload.params, []);
                     });
+
                     provider.injectResult(
                         test.transaction.common.hardfork === 'london' ?
                         postEip1559Block:
@@ -906,6 +942,7 @@ describe("eth", function () {
 
                     var transaction = clone(test.transaction);
                     delete transaction.chainId;
+                    delete transaction.common;
                     testAccount.signTransaction(transaction)
                     .then(function (tx) {
                         assert.isObject(tx);