fix: ethQuery is not defined when refresh is called

[tommasini]

Description

We were experiencing undefined is not an object (evaluating 'inputBn.toString') this warning multiple times when opening the app, because this.ethQuery it was not defined yet at the AccountTrackerController when .

It was added a condition to only update the balance if it was possible to get balance from chain.

Core PR: MetaMask/core#3933

Related issues

Fixes:

Manual testing steps

1. Open the app
2. Shouldn't have the warning undefined is not an object (evaluating 'inputBn.toString')

Screenshots/Recordings

Before

After

e

Smoke E2E test builds: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/a075edf7-676e-4d24-8b0d-014890863b6d

Regression E2E test builds: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/30f56c4e-3811-432c-83f0-139ea5b16ff5

Pre-merge author checklist

• I’ve followed MetaMask Coding Standards.
• I've clearly explained what problem this PR is solving and how it is solved.
• I've linked related issues
• I've included manual testing steps
• I've included screenshots/recordings if applicable
• I’ve included tests if applicable
• I’ve documented my code using JSDoc format if applicable
• I’ve applied the right labels on the PR (see labeling guidelines). Not required for external contributors.
• I’ve properly set the pull request status:
  • In case it's not yet "ready for review", I've set it to "draft".
  • In case it's "ready for review", I've changed it from "draft" to "non-draft".

Pre-merge reviewer checklist

• I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed).
• I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots.

[github-actions]

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.

[sonarcloud]

Quality Gate passed

Issues
0 New issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

[github-actions]

E2E test started on Bitrise: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/976a13b5-1b13-4bb9-a825-ad694812158e
You can also kick off another Bitrise E2E smoke test by removing and re-applying the (Run Smoke E2E) label

[Gudahtt]

Good catch! This bug seems to be upstream as well. Could you fix this in core?

[Gudahtt]

This does make me question how we're handling engine initialization as well though:

• Why are we attempting to get the balance before initialization? Perhaps we should show a loading indicator until after initialization has completed
• Initialization is happening in a disjointed manner; we're triggering it separately for each controller, and then some initialization steps are run at the end separately (configureControllersOnNetworkChange and startPolling). We should be initializing in one place, and awaiting any async steps so that we can know for certain when it has finished.
• The 500ms pause here is slowing down the response to network switching by 500ms. This is unnecessary now that the network controller has been updated. At one point it may have been a workaround do the fact that there was no event for when the provider has been updated, but we have that now (the networkDidChange event).
• Some of the steps in configureControllersOnNetworkChange seem questionable; I would audit those. Each controller should react to network changes on its own, we shouldn't need to manually configure anything. It's possible that some older controllers still need that, but I am doubtful.

[tommasini]

Why are we attempting to get the balance before initialization? Perhaps we should show a loading indicator until after initialization has completed
Definitely we should have a loading indicator! We can definitely seeing an zero balance for some time, and if the network is slower it will definitely seems like it's not getting the balance: https://recordit.co/BpyTp0wqjM

The 500ms pause here is slowing down the response to network switching by 500ms. This is unnecessary now that the network controller has been updated. At one point it may have been a workaround do the fact that there was no event for when the provider has been updated, but we have that now (the networkDidChange event).

Need to debug further but after removing this timeout, it seems that the token rates are not updating on the UI when network changes or the tokens are being detected

Some of the steps in configureControllersOnNetworkChange seem questionable; I would audit those. Each controller should react to network changes on its own, we shouldn't need to manually configure anything. It's possible that some older controllers still need that, but I am doubtful.

These detect function should be inside a promise.all for performance reasons right?
I need to understand why when we remove the timeout the networkDidChange is still not working for at least the tokens (only thing I tested), probably good to create a ticket for follow-up, wdyt?

[tommasini]

Good catch! This bug seems to be upstream as well. Could you fix this in core?

Yes I can! I have some core PRs that I've being always postponed to address (breaking changes and unit tests...). I will create this one, need also to dedicate sometime to solve the issues of the others!

[Gudahtt]

probably good to create a ticket for follow-up, wdyt?

Definitely, all of that stuff can be looked into as separate PRs/tickets, I just jotted them down here because I noticed it when looking at this PR.

[tommasini]

Core PR: MetaMask/core#3933

[tommasini]

Forgot to paste here, issue created: https://github.com/MetaMask/mobile-planning/issues/1566

[Cal-L]

Lgtm