[Bug]: eth_getTransactionByHash stopped returning r, s, v about metamask-extension HOT 8 OPEN

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.

from metamask-extension.

@dbrans Great work! I have included v29.0.1 into my local metamask-extension (v11.15.5), tested it and I can confirm that the rsv is now back.

from metamask-extension.

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

from metamask-extension.

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.

from metamask-extension.

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.

from metamask-extension.

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.

from metamask-extension.

Just looking at the metamask-core issues, and it looks like this is fixed here:

MetaMask/core#4255

I will test it once metamask-extension upgrades transaction-controller to the version >= 29.0.1

from metamask-extension.

@keithchew great catch and good job tracking this down. I'm adding this bug as fixed by #24701. If you have a moment, could you take a look at the builds on that PR to ensure the bug is fixed?

from metamask-extension.