fix: transaction-controller – approveTransaction should not throw away raw signed transaction #4255
Conversation
|
@metamaskbot publish-preview |
|
Preview builds have been published. See these instructions for more information about preview builds. Expand for full list of packages and versions. |
|
|
||
| if (status === TransactionStatus.submitted) { | ||
| if (updatedTransactionMeta.status === TransactionStatus.submitted) { |
There was a problem hiding this comment.
Do we need to change these?
The logic here is based on the original input so the merged metadata shouldn't be relevant.
There was a problem hiding this comment.
I added this for typescript type checking: A TransactionMeta only has a submittedType when its status is submitted. It only has an error when status is failed.
| let releaseNonceLock: (() => void) | undefined; | ||
|
|
||
| let txMeta = this.getTransactionOrThrow(transactionId); |
There was a problem hiding this comment.
Can we keep this called transactionMeta for readability and to minimise the diff?
| nonce, | ||
| chainId, | ||
| gasLimit: txParams.gas, | ||
| ...(isEIP1559 && { |
There was a problem hiding this comment.
It seems that previously we first did an update with the nonce, chainId, gasLimit, and status.
Then the estimatedBaseFee and type were updated by the signTransaction method which also does additional updates.
Then later, we update the hash, status and submittedTime.
For the sake of risk and matching state history, should we match that now and ensure the same updates happen in the same order?
It also ensures we at least get the approved status even if the preTxBalance query failed for example.
There was a problem hiding this comment.
Matt, based on your comment, I took a closer look at what is being updated and when. It's not straightforward.
Before this PR:
Update 1:
- status, nonce, chainid are updated
- gasLimit: looks like it's being set but it is not. It's actually set on baseTxParams which is not attached to updatedTransactionMeta and so is not stored.
Update 2 (in signTransaction):
- status, r, s, v are updated
- gasLimit, estimatedBaseFee, type: look like they're being updated. They're actually set on txParams. txParams is signed, but never attached to updatedTransactionMeta. Wait... estimatedBaseFee is already there from the original txParams. So just "type" and “gasLimit” are in limbo: they are on the signed txParams (rawTx) but not on the updatedTransactionMeta in storage.
Update 3 (in signTransaction):
- rawTx is added
[At this point, before publishing, preTxBalance is fetched]
Update 4:
- status, hash, submittedTime, preTxBalance are updated
- rawTx, r, s, v are all lost in this update because updatedTransactionMeta is now stale. This was the original reason for this PR.
Based on your suggestion, I made some changes to closer match the order of operations, while addressing inconsistencies between the signed txParams and what is in storage. I also endeavoured to make updates more obvious.
After this PR:
Update 1:
- status, nonce, chainid, gasLimit, type are updated. estimatedBaseFee is already there from the original txParams.
- This is the biggest change in what gets saved: the txParams that what will be signed is the one that is saved.
Update 2 (in signTransaction):
- status, r, s, v are updated
Update 3 (in signTransaction):
- rawTx is added
[At this point, before publishing, preTxBalance is fetched]
Update 4:
- status, hash, submittedTime, preTxBalance are updated
| @@ -3611,8 +3610,8 @@ export class TransactionController extends BaseController< | |||
| note, | |||
| skipHistory, | |||
| }: { transactionId: string; note?: string; skipHistory?: boolean }, | |||
| callback: (transactionMeta: TransactionMeta) => TransactionMeta | void, | |||
| ) { | |||
| mutationFn: (transactionMeta: TransactionMeta) => TransactionMeta | void, | |||
There was a problem hiding this comment.
This was for consistency with the update method which refers to the argument as callback.
Plus it's not necessarily always a mutation as you can return a fresh metadata if that is easier.
|
@metamaskbot publish-preview |
|
Preview builds have been published. See these instructions for more information about preview builds. Expand for full list of packages and versions. |
## Explanation Patch release of TransactionController to fix rawTx being dropped from Tx Metadata #4255. ## References * Related MetaMask/metamask-extension#24183 --------- Co-authored-by: Mark Stacey <markjstacey@gmail.com>
|
I caught this regression and posted in metamask-extension a while back, just wanted to link it from here, so that one can also be closed: |
Explanation
Changed
approveTransactionto use#updateTransactionInternal.Addresses MetaMask/metamask-extension#24183 – Manually verified in extension.
@matthewwalsh0 observed:
The implementation of approveTransaction is throwing away the raw signed transaction that signTransaction was adding to the metadata state by holding onto a stale version of the tx metadata.
Long-term @matthewwalsh0 recommends replaying updateTransaction with the more modern updateTransactionInternal everywhere to avoid bugs like this.
References
Changelog
@metamask/transaction-controllerChecklist