resolve issue with missing template error by brad-decker · Pull Request #10692 · MetaMask/metamask-extension

brad-decker · 2021-03-22T17:26:09Z

Possible cause?

    this.setState({ mounted: true });
    if (isNotification && totalUnapprovedCount === 0) {
      global.platform.closeCurrentWindow();
    } else if (!isNotification && showAwaitingSwapScreen) {
      history.push(AWAITING_SWAP_ROUTE);
    } else if (!isNotification && haveSwapsQuotes) {
      history.push(VIEW_QUOTE_ROUTE);
    } else if (!isNotification && swapsFetchParams) {
      history.push(BUILD_QUOTE_ROUTE);
    } else if (firstPermissionsRequestId) { // <------ If this is undefined, which is an approval
      history.push(`${CONNECT_ROUTE}/${firstPermissionsRequestId}`);
    } else if (unconfirmedTransactionsCount > 0) {
      history.push(CONFIRM_TRANSACTION_ROUTE);
    } else if (Object.keys(suggestedTokens).length > 0) {
      history.push(CONFIRM_ADD_SUGGESTED_TOKEN_ROUTE);
    } else if (pendingApprovals.length > 0) { // <------- Then this would be true
      history.push(CONFIRMATION_V_NEXT_ROUTE);
    }

Why would firstPermissionsRequestId be undefined when there is pending approval of this type? Not sure. This change just makes it so that the only approvals that will be caught on the last block of the else/if are ones that have templates. This might mean for that user that they don't get routed to the notification at all -- perhaps not a real solution but one that doesn't cause an error.

metamaskbot · 2021-03-22T17:53:55Z

Builds ready [e4cab02]

builds: chrome, firefox, opera
bundle viz: background, ui, inpage, contentscript, ui-libs, bg-libs, phishing-detect
build viz: Build System
code coverage: Report
storybook: Storybook
all artifacts

Page Load Metrics (604 ± 11 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	44	74	58	9	4
		domContentLoaded	560	647	602	23	11
		load	566	648	604	22	11
		domInteractive	559	646	602	23	11

Gudahtt · 2021-03-23T13:16:28Z

Why would firstPermissionsRequestId be undefined when there is pending approval of this type? Not sure.

I think this is the question we need to answer. I don't have much confidence that this change would make things less-broken without better understanding what went wrong here.

Gudahtt · 2021-03-23T13:50:43Z

I've just traced through how these permission requests are handled, and it looks like we store two different related states for each request. We have permissionsRequests which tracks a request for permissions, and we have the Approval controller state which is the user confirmation asking for approval for this permission request.

It looks like the lifetime of the permissionsRequests entry should exceed the lifetime of the approval request 🤔 At least that's what I'm seeing. Either when rejecting or approving the confirmation, the approval request is removed first, before the permissions request. There must be some edge case at play here...

brad-decker · 2021-03-23T14:34:32Z

I think this change is still likely a good one, but should be batched together with any adjustments we want to make to this flow to be safe. I'll also look through the trace and see if I can pinpoint a culprit or suggest a change to make this safer.

Gudahtt · 2021-03-23T15:07:12Z

I agree that we should protect against routing things without templates to the template-based confirmation page. But I'm concerned that this PR as-written might result in the error being hidden, which would make fixing it properly more difficult.

brad-decker · 2021-03-23T15:19:19Z

I agree that we should protect against routing things without templates to the template-based confirmation page. But I'm concerned that this PR as-written might result in the error being hidden, which would make fixing it properly more difficult.

I agree, that's what I meant by batching together, but in retrospect, that wasn't clearly decipherable from what I wrote.

Gudahtt · 2021-03-23T15:28:46Z

Ah, sorry, understood. Even still though, should we not state our expectations here and throw an error if they're not met? e.g. maybe an else clause that throws an error for an unrecognized/invalid approval request would be useful. Otherwise it could hide errors in the future, even after this is fixed.

brad-decker · 2021-03-23T15:38:07Z

If we rewind to before the template system/approval part of this logic, there wasn't an else statement catching errors there. I just added another else if onto the chain. Unless something changed with the permissions/approval controllers that landed alongside the template system, not before it, then the edge case existed then. We didn't know about it because it doesn't present as an error.

So I'd be in favor of adding an else statement, but maybe instead of throwing an Error we should just log it to sentry? Doesn't throwing an error in that case cause the user flow to be interrupted?

Gudahtt · 2021-03-23T17:21:03Z

So I'd be in favor of adding an else statement, but maybe instead of throwing an Error we should just log it to sentry? Doesn't throwing an error in that case cause the user flow to be interrupted?

Sounds reasonable in principle, but what would happen to the user in this case? Throwing an error might be all we can do with the information we have, if it's an unaccounted for situation.

metamaskbot · 2021-03-23T17:27:32Z

Builds ready [9dfd273]

builds: chrome, firefox, opera
bundle viz: background, ui, inpage, contentscript, ui-libs, bg-libs, phishing-detect
build viz: Build System
code coverage: Report
storybook: Storybook
all artifacts

Page Load Metrics (614 ± 18 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	44	83	58	10	5
		domContentLoaded	553	735	613	38	18
		load	554	736	614	38	18
		domInteractive	553	735	613	38	18

brad-decker · 2021-03-23T17:31:45Z

Sounds reasonable in principle, but what would happen to the user in this case? Throwing an error might be all we can do with the information we have, if it's an unaccounted for situation.

Yeah, fair. In any event, we should think about what to do in this case once all confirmations are using the approval controller. In that case, if we get to the point where the confirmation is not in the template system/not cared for in our logic, we could automatically reject it on the user's behalf and display a warning to the user while recording the details of the approval. That'd give us enough clues to work with I think.

Gudahtt · 2021-03-23T17:47:35Z

ui/app/pages/confirmation/confirmation.js

  const dispatch = useDispatch();
  const history = useHistory();
-  const pendingConfirmations = useSelector(getUnapprovedConfirmations, isEqual);
+  const pendingConfirmations = useSelector(


Was this the underlying error? I'm not quite sure I follow 🤔

My guess is that the problem occurs when there is a "request permissions" confirmation queued up behind a templated confirmation. I don't yet understand why it only affects the request permissions confirmation though 🤔

wallet_requestPermissions uses the approval controller, which creates an entry in the pendingApprovals object. I was expecting that type of approval to always be handled by the routing login in the home component, and supersede this new page ever being called. However, if a wallet_addEtherumChain is queued up before a wallet_requestPermissions, the routing logic is skipped because there is already a notification window open and the user is viewing the addEthereumChain confirmation. Once they click confirm this page would have attempted to pull the remaining approval out which would be the wallet_requestPermissions one.

Ahhh I see! That makes sense, thanks

Gudahtt · 2021-03-23T18:05:59Z

ui/app/pages/home/home.container.js

    originOfCurrentTab,
    shouldShowWeb3ShimUsageNotification,
-    pendingApprovals: Object.values(pendingApprovals),
+    pendingApprovals,


Nit: it might be worth renaming this prop, now that it no longer includes all pending approvals. Maybe something like pendingConfirmations, like you used on the confirmation page.

Gudahtt

LGTM!

It would be better if we handled the else case, but, not a blocker as that's a pre-existing problem.

metamaskbot · 2021-03-23T20:25:20Z

Builds ready [50e8783]

builds: chrome, firefox, opera
bundle viz: background, ui, inpage, contentscript, ui-libs, bg-libs, phishing-detect
build viz: Build System
code coverage: Report
storybook: Storybook
all artifacts

Page Load Metrics (1229 ± 172 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	NaN	NaN	NaN	NaN	NaN
		domContentLoaded	587	2103	1222	358	172
		load	588	2106	1229	358	172
		domInteractive	587	2103	1222	358	172

brad-decker · 2021-03-23T20:56:13Z

@Gudahtt gonna do the else block in a different PR -- need to think through the logic there. it won't be an else block, but rather another else-if or perhaps reworking the entire structure of this logic because the else block will trigger always because its in the componentDidMount method.

resolve issue with missing template error

e4cab02

Welziewagers2015 previously approved these changes Mar 22, 2021

View reviewed changes

brad-decker marked this pull request as ready for review March 22, 2021 17:57

brad-decker requested a review from a team as a code owner March 22, 2021 17:57

brad-decker requested review from Gudahtt, rekmarks and ryanml March 22, 2021 17:57

also apply filtering to confirmation page

9dfd273

brad-decker dismissed Welziewagers2015’s stale review via 9dfd273 March 23, 2021 17:10

Gudahtt reviewed Mar 23, 2021

View reviewed changes

Gudahtt previously approved these changes Mar 23, 2021

View reviewed changes

rename variable

50e8783

brad-decker dismissed Gudahtt’s stale review via 50e8783 March 23, 2021 20:09

Gudahtt approved these changes Mar 23, 2021

View reviewed changes

brad-decker merged commit 255586a into develop Mar 23, 2021

brad-decker deleted the fix-template-not-found-issue branch March 23, 2021 21:12

github-actions bot locked and limited conversation to collaborators Mar 23, 2021

Conversation

brad-decker commented Mar 22, 2021

Uh oh!

metamaskbot commented Mar 22, 2021

Uh oh!

Gudahtt commented Mar 23, 2021

Uh oh!

Gudahtt commented Mar 23, 2021

Uh oh!

brad-decker commented Mar 23, 2021

Uh oh!

Gudahtt commented Mar 23, 2021

Uh oh!

brad-decker commented Mar 23, 2021

Uh oh!

Gudahtt commented Mar 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

brad-decker commented Mar 23, 2021

Uh oh!

Gudahtt commented Mar 23, 2021

Uh oh!

metamaskbot commented Mar 23, 2021

Uh oh!

brad-decker commented Mar 23, 2021

Uh oh!

Gudahtt Mar 23, 2021

Choose a reason for hiding this comment

Uh oh!

brad-decker Mar 23, 2021

Choose a reason for hiding this comment

Uh oh!

Gudahtt Mar 23, 2021

Choose a reason for hiding this comment

Uh oh!

Gudahtt Mar 23, 2021

Choose a reason for hiding this comment

Uh oh!

Gudahtt left a comment

Choose a reason for hiding this comment

Uh oh!

metamaskbot commented Mar 23, 2021

Uh oh!

brad-decker commented Mar 23, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Gudahtt commented Mar 23, 2021 •

edited

Loading