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 encypt/decrypt tx queueing #10350

Merged
merged 1 commit into from
Feb 17, 2021
Merged

Fix encypt/decrypt tx queueing #10350

merged 1 commit into from
Feb 17, 2021

Conversation

tmashuang
Copy link
Contributor

Fixes #10231

Use unconfirmedTransactionsListSelector in the encypt/decrypt components to render the appropriate data to the component at the appropriate time(?).
I am still unsure how sometimes the state.confirmTransaction can we left empty sometimes on rendering the component, possibly the issue with the ConfirmTransaction componentDidUpdate constantly hitting this section.

if (
paramsTransactionId &&
transactionId &&
prevProps.paramsTransactionId !== paramsTransactionId
) {
clearConfirmTransaction()
getContractMethodData(data)
setTransactionToConfirm(paramsTransactionId)

For now this seems to be an intermediate fix.

Manual testing steps:
I've tried queueing other types of transactions in the middle and around these requests and there doesn't seem to be an issue with the order, but if anyone else can confirm much appreciated.

  • Request 2 encrypt requests. Confirm/Approve both shouldn't be an issue.
    The requests can be from two different origins (edge-case check).
  • Request 2 decrypt requests. Confirm/Decrypt both shouldn't be an issue. The requests can be from two different origins, and it should return the request decrypted string to the appropriate origin.

@tmashuang tmashuang requested a review from a team as a code owner February 3, 2021 01:01
@tmashuang tmashuang requested a review from darkwing February 3, 2021 01:01
@github-actions
Copy link
Contributor

github-actions bot commented Feb 3, 2021

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

@metamaskbot
Copy link
Collaborator

Builds ready [d0898b9]
Page Load Metrics (678 ± 59 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint49826684
domContentLoaded42385667612259
load42485867812359
domInteractive42285667512259

Fixes #10231

Use unconfirmedTransactionsListSelector in the encypt/decrypt components to render the appropriate data to the component at the appropriate time(?).
I am still unsure how sometimes the state.confirmTransaction can we left empty sometimes on rendering the component, possibly the issue with the ConfirmTransaction componentDidUpdate constantly hitting this section.
https://github.com/MetaMask/metamask-extension/blob/develop/ui/app/pages/confirm-transaction/confirm-transaction.component.js#L94-L101

For now this seems to be an intermediate fix.
@tmashuang tmashuang force-pushed the queue-decrypt-requests branch from d0898b9 to 6772703 Compare February 4, 2021 18:27
@metamaskbot
Copy link
Collaborator

Builds ready [6772703]
Page Load Metrics (656 ± 60 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint448563126
domContentLoaded41193665512560
load41293865612560
domInteractive41093665412460

Copy link
Contributor

@darkwing darkwing left a comment

Choose a reason for hiding this comment

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

I can confirm that this works as described! Nice @tmashuang !

@Gudahtt
Copy link
Member

Gudahtt commented Feb 8, 2021

This is a weird one. I do think there is a problem in the base transaction component in the function you highlighted, but even still it looks like confirmTransaction would still be undefined at least for one render. That's enough to crash the UI.

I'm unsure what the consequences are of not using confirmTransaction here. It's a bit weird to leave that state unused here, given that there are still various hooks into it 🤔

@Gudahtt
Copy link
Member

Gudahtt commented Feb 8, 2021

The base confirmation component seems to assume that each confirmation page is tolerant of confirmTransaction.txData being undefined briefly. See conf-tx.js for example, which shows a loading screen if txData or txData.msgParams is undefined: https://github.com/MetaMask/metamask-extension/blob/develop/ui/app/pages/confirm-transaction/conf-tx.js#L250

That style approach would be more in-line with how our other confirmations work. That might be better than the approach taken in this PR, which leaves a vestigial confirmTransaction state in Redux that might further confuse people in the future or lead to state inconsistencies.

That said, the whole confirm-transaction Redux slice seems entirely useless for signature and decryption requests. It looks like it was designed to hold gas-related state temporarily to allow the user to adjust gas parameters on the confirm screen, and that's about it. It doesn't really help for confirmations that don't involve gas. It would be nice at some point in the future to disassociate it entirely from these confirmations, but it's tied up in various places in the React component hierarchy right now so that wouldn't be a simple task.

@tmashuang tmashuang closed this Feb 10, 2021
@tmashuang tmashuang reopened this Feb 10, 2021
@github-actions github-actions bot locked and limited conversation to collaborators Feb 10, 2021
@MetaMask MetaMask unlocked this conversation Feb 10, 2021
Copy link
Member

@Gudahtt Gudahtt left a comment

Choose a reason for hiding this comment

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

We can merge this now and create an issue to clean it up further later, since it does seem to fix the immediate problem.

Edit: I've created a ticket to track this: #10470

@tmashuang tmashuang merged commit 21aec63 into develop Feb 17, 2021
@tmashuang tmashuang deleted the queue-decrypt-requests branch February 17, 2021 20:47
@github-actions github-actions bot locked and limited conversation to collaborators Feb 17, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Sending eth_decrypt multiple times throws "Cannot read property 'from' of undefined" error
4 participants