Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Remove async operation from updateTransactionGasFees #5539

Merged

Conversation

OGPoyraz
Copy link
Member

@OGPoyraz OGPoyraz commented Mar 25, 2025

Explanation

This PR aims to fix an issue where updateTransactionGasFees updater rejects updating draft state with following error:
TypeError: Cannot perform 'get' on a proxy that has been revoked

The fix is to remove that async operation because it's unnecessary to check it again since it's been check while addTransaction

References

Changelog

@metamask/transaction-controller

  • FIXED: Fix type error when enableTxParamsGasFeeUpdates is set to true

Checklist

  • I've updated the test suite for new or updated code as appropriate
  • I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate
  • I've highlighted breaking changes using the "BREAKING" category above as appropriate
  • I've prepared draft pull requests for clients and consumer packages to resolve any breaking changes

@OGPoyraz OGPoyraz requested review from a team as code owners March 25, 2025 09:51
@@ -3881,7 +3881,7 @@ export class TransactionController extends BaseController<
log('Updated simulation data', transactionId, simulationData);
}

#onGasFeePollerTransactionUpdate({
async #onGasFeePollerTransactionUpdate({
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is fired from an event listener, so there is technically now a risk that if we have a very slow update, that they begin to overlap.

Should we add a mutex to prevent two happening at once?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call Matthew, added gasFeeUpdateMutexes map for locking same transactionId gas updates now.

@OGPoyraz OGPoyraz force-pushed the fix/remove-async-operation-from-updateTransactionGasFees branch 2 times, most recently from 975db53 to fbd9969 Compare March 25, 2025 10:58
@OGPoyraz OGPoyraz requested a review from matthewwalsh0 March 25, 2025 12:05
const release = await mutex.acquire();
try {
const transaction = this.getTransaction(transactionId) as TransactionMeta;
const isEIP1559Compatible =
Copy link
Member

@matthewwalsh0 matthewwalsh0 Mar 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry Goktug, I've looked into this deeper and I don't think we need to call getEIP1559Compatability at all meaning this doesn't need to be async and no mutexes.

Because we already call this inside addTransaction and then go through validateEIP1559Compatibility which would throw if there was a maxFeePerGas or maxPriorityFeePerGasprovided but the network wasn't EIP-1159.

So we could instead just set the type early at that point in addTransaction based on the existing compatibility call, assuming the user didn't explicit specify the legacy type.

Then here, we would just trust the envelope type?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Matthew, I added a tiny function that sets type after validation if nothing defined, so we make sure that type is always present and it give us the flexibility to remove that async check.

@OGPoyraz OGPoyraz force-pushed the fix/remove-async-operation-from-updateTransactionGasFees branch from 3230990 to b6decfe Compare March 26, 2025 09:55
@OGPoyraz OGPoyraz enabled auto-merge (squash) March 26, 2025 13:05
@OGPoyraz OGPoyraz merged commit 85d1522 into main Mar 26, 2025
193 checks passed
@OGPoyraz OGPoyraz deleted the fix/remove-async-operation-from-updateTransactionGasFees branch March 26, 2025 13:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants