[Bug]: eth_getTransactionByHash stopped returning r, s, v

[keithchew]

Describe the bug

The call to eth_getTransactionByHash (after calling eth_sendTransaction) used to return the signature of the transaction, ie r, s, v values. But a recent update is causing the r s v to be missing. Is this a regression issue?

Expected behavior

Previous versions used to return rsv, expected to have the rsv values.

Screenshots/Recordings

No response

Steps to reproduce

Snippet below:

        const web3Metamask = new Web3(window.ethereum);
        const txnId = await web3Metamask.currentProvider.request({
            method: 'eth_sendTransaction',
            params: [txn],
        });
        console.log(`txnId [${txnId}]`);
        const transaction = await web3Metamask.currentProvider.request({
            method: 'eth_getTransactionByHash',
            params: [txnId],
        });
        console.log(`r [${transaction.r}] s [${transaction.s}] v [${transaction.v}]`);

The transaction above will be missing the rsv values.

Error messages or log output

No response

Version

11.14.4

Build type

None

Browser

Chrome

Operating system

Windows

Hardware wallet

No response

Additional context

No response

Severity

No response

[keithchew]

I can confirm the regression was introduced in v11.14.0, last working version was v11.13.3.

[keithchew]

Just digging into this a bit more, it looks like there was a major refactor from v11.13.3 to v11.14.0. At a glance, it doesn't look like the regression was introduced in metamask-extension, but perhaps further upstream.

I can see @metamask/transaction-controller was updated from v23.1.0 to v27.0.1, and there were also quite big changes between these versions. One thing which stood out was:

 private async updateTransactionMetaRSV(
    transactionMeta: TransactionMeta,
    signedTx: TypedTransaction,
  ): Promise<TransactionMeta> {
    const transactionMetaWithRsv = cloneDeep(transactionMeta);
...

The old version did not do a deep clone before setting the RSV, but new version above does. Maybe the TransactionMeta without the RSV is being returned from the API instead of the updated one?

Any guidance on this would be appreciated.

[keithchew]

Ok, I have tracked it down.

In transaction-controller.ts signTransaction(), it does update state transaction with rsv:

    this.updateTransaction(
      transactionMetaWithRsv,
      'TransactionController#approveTransaction - Transaction signed',
    );

However, upon returning to the caller approveTransaction(), it replaces the transaction with an older one without the rsv:

      this.updateTransaction(
        updatedTransactionMeta,
        'TransactionController#approveTransaction - Transaction submitted',
      );

There is a discontinuity between updatedTransactionMeta and transactionMetaWithRsv. In the previous version deep clone was not used, so the properties got shallow copied and persisted.

A few options, we can either not do a deep copy, or make signTransaction() return an updated meta that the caller will merge with updatedTransactionMeta.

[keithchew]

Yup, to confirm the findings above, I modified my local copy to remove deep clone:

    // const transactionMetaWithRsv = _lodash.cloneDeep.call(void 0, transactionMeta);
    const transactionMetaWithRsv = transactionMeta;

With the above RSV is persisted. Let me know if you would like me to prepare a PR, but it should be pretty straightforward.

[keithchew]

Just following up to see if there is progress on this bug? I am happy to submit the one-liner PR above to revert the regression if this will help speed things up. This change is causing some dapps to break as they are currently using RSV to verify that the txn has been signed after sending.