Replace controller context by Gudahtt · Pull Request #2416 · MetaMask/metamask-mobile

Gudahtt · 2021-03-22T14:49:08Z

The context object previously constructed by the ComposableController is no more. Instead each controller now accepts its dependencies directly as constructor parameters, in a similar manner to the extension controllers.

This was done in preparation for migrating to BaseControllerV2 and the new controller messaging system - this is just a temporary solution that will let us migrate controllers one at a time.

The style of dependency injection here matches the extension (at least with newer controllers anyway). Specific methods and state snapshots are injected rather than entire controllers, to help simplify unit tests and make it easier to understand how controllers interact.

The Engine.context property was used throughout mobile, so it has been preserved. It is now constructed explicitly, rather than being a re-export of the ComposableController context.

This PR depends upon MetaMask/core#387

Gudahtt · 2021-03-23T12:37:27Z

@ibrahimtaveras00: I don't anticipate this including any functional changes, so this just needs regression testing. Unfortunately I don't have my environment setup to build mobile anymore, so I didn't test this myself 😬 Just wired things up equivalently to how they were before.

app/core/Engine.js

app/components/Views/ApproveView/Approve/index.test.js

Gudahtt · 2021-04-07T18:44:11Z

I've just rebased this onto develop, and updated the controllers bundle to match the latest version of MetaMask/core#387
This is now ready for review again.

ibrahimtaveras00 · 2021-04-12T18:22:31Z

Issue 1:

I noticed as soon as I build the app I see this warning:

ibrahimtaveras00 · 2021-04-12T19:14:37Z

Issue 2:

Tapping on account switcher shows 0 balance (I do have a balance for these 3 accounts)

This may be related to above balance issue, but when attempting to connect to a dapp, I get this error:

seen here = http://recordit.co/GkoUastaJH

steps:

go to browser
navigate to https://metamask.github.io/test-dapp/
tap on connect

Gudahtt · 2021-04-12T19:16:02Z

Thanks, I'll take a look!

ibrahimtaveras00 · 2021-04-12T20:55:36Z

Issue 3:

If I add contacts, then kill app and relaunch, those contacts get lost and I would have to re-add them

seen here = http://recordit.co/zhNMmqw5o8

Steps I took:

go to send flow, and add a contact to address book
I then went to settings > contacts to make sure it was there
Kill app
Relaunch
double check if contacts are in the send flow and settings > contacts

app/core/Engine.js

estebanmino · 2021-04-16T14:43:16Z

don't merge this yet 🙏 , we have a node bump to v14 in the making and currently working with node 10 so just to be safe we'll be merging this when that work finishes cc @andrepimenta @rickycodes @omnat

Gudahtt · 2021-04-16T16:16:38Z

Sounds good! Linking the Node v14 issue: #2205

rickycodes · 2021-04-22T19:32:52Z

I just merged the node14 upgrade, so I think this should be good now.

mobularay · 2021-04-26T19:41:33Z

@rickycodes can this pull request be closed? Can you confirm if this PR went out with v2.2?

The `context` object previously constructed by the `ComposableController` is no more. Instead each controller now accepts its dependencies directly as constructor parameters, in a similar manner to the extension controllers. This was done in preparation for migrating to BaseControllerV2 and the new controller messaging system - this is just a temporary solution that will let us migrate controllers one at a time. The style of dependency injection here matches the extension (at least with newer controllers anyway). Specific methods and state snapshots are injected rather than entire controllers, to help simplify unit tests and make it easier to understand how controllers interact. The `Engine.context` property was used throughout mobile, so it has been preserved. It is now constructed explicitly, rather than being a re-export of the `ComposableController` context. This PR depends upon MetaMask/core#387

The API key was passed in directly by accident, instead of a function get returned the key. This has been fixed. Co-authored-by: Esteban Miño <efmino@uc.cl>

The `AccountTrackerController` option `initialIdentities` was replaced with `getIdentities`. The initial identities passed in here were incorrect anyway due to a typo (`initialState.preferencesController` was used instead of `initialState.PreferencesController`).

The `controllers` setter on `ComposedController` used to be responsible for setting initial state. Since that setter has been removed, the initial state is now set after the controllers have been constructed. This should be functionally equivalent to what it was before. We're setting the initial state by calling `update` on each controller, just as the `controller` setter used to.

Gudahtt · 2021-04-26T19:59:19Z

This has been rebased onto develop. A few conflicts needed to be resolved manually in Engine.js. I've just reviewed the diff for Engine.js again to ensure all of the changes were accounted for.

mobularay · 2021-04-26T20:11:37Z

yay!! Anything that needs to be done in order to close off this card? Does this need QA review, or is it good to go (to be part of v2.3 release in next sprint)?

wachunei · 2021-04-27T00:03:43Z

heads up, I ran into these while trying to login:

Gudahtt · 2021-04-27T14:27:13Z

Interesting. At first I was going to ask that you check to ensure you had a vault to unlock in that case, but... in trying to trace this, I can't figure out how the vault is supposed to get set in the KeyringController state in the first place. It looks like the KeyringController expects vault to be set in its initial state, but it never actually sets vault on its state. How did this ever work 😕

Gudahtt · 2021-04-27T15:27:40Z

Whoops, I think I found the problem. I had accidentally left the most recent commit out of that most recent rebase (07989f1).

I remain confused about how the vault state is restored, but that would explain the error you were seeing! Hopefully it works now.

…tch-1 * 'develop' of github.com:MetaMask/metamask-mobile: (51 commits) Feature/confusables (MetaMask#2464) fix typeface on login text field (MetaMask#2610) Replace controller context (MetaMask#2416) Fix adding custom token in custom network (MetaMask#2590) only add custom tokens if not in mainnet (MetaMask#2470) Address yarn lints (MetaMask#2524) Upgrade .nvmrc to node v14 (MetaMask#2588) Swaps: Add cache thresholds configuration (MetaMask#2514) Swaps: BSC Support (MetaMask#2468) Use node 14 (MetaMask#2539) resolve isENS without case sensitivity (MetaMask#2545) (MetaMask#2568) Revert "resolve isENS without case sensitivity (MetaMask#2545)" (MetaMask#2566) resolve isENS without case sensitivity (MetaMask#2545) Bump versioncode (MetaMask#2558) v2.2.0 (MetaMask#2555) Include decimalsToShow in balanceToFiatNumber (MetaMask#2547) Bug fix/sync import time (MetaMask#2554) bundle update (MetaMask#2549) Fix analytics try catch (MetaMask#2546) Only get nonce from the network if the feature is enabled (MetaMask#2543) ...

* resolve isENS without case sensitivity (#2545) (#2568) Co-authored-by: ricky <ricky.miller@gmail.com> Co-authored-by: Minh <minhle@canva.com> * Use node 14 (#2539) * Swaps: BSC Support (#2468) * Swaps: Add cache thresholds configuration (#2514) * Upgrade .nvmrc to node v14 (#2588) * Address yarn lints (#2524) * address yarn lints * add eslint-disable * Update isENS method * fix rn-fetch-blob.js mock * Add tests for isENS * useRef instead of useMemo * Update eslint * Use lastIndexOf and add test * Add test case for ricky.metamask.eth * Add offset * Fix AppConstants import * only add custom tokens if not in mainnet (#2470) * checkchainid * tests * Fix adding custom token in custom network (#2590) * Replace controller context (#2416) * Replace controller context The `context` object previously constructed by the `ComposableController` is no more. Instead each controller now accepts its dependencies directly as constructor parameters, in a similar manner to the extension controllers. This was done in preparation for migrating to BaseControllerV2 and the new controller messaging system - this is just a temporary solution that will let us migrate controllers one at a time. The style of dependency injection here matches the extension (at least with newer controllers anyway). Specific methods and state snapshots are injected rather than entire controllers, to help simplify unit tests and make it easier to understand how controllers interact. The `Engine.context` property was used throughout mobile, so it has been preserved. It is now constructed explicitly, rather than being a re-export of the `ComposableController` context. This PR depends upon MetaMask/core#387 * Pass in function for `getOpenSeaApiKey` rather than string The API key was passed in directly by accident, instead of a function get returned the key. This has been fixed. Co-authored-by: Esteban Miño <efmino@uc.cl> * Update `AccountTrackerController` options The `AccountTrackerController` option `initialIdentities` was replaced with `getIdentities`. The initial identities passed in here were incorrect anyway due to a typo (`initialState.preferencesController` was used instead of `initialState.PreferencesController`). * Fix `getIdentities` handler for `AccountTrackerController` * Set initial controller state The `controllers` setter on `ComposedController` used to be responsible for setting initial state. Since that setter has been removed, the initial state is now set after the controllers have been constructed. This should be functionally equivalent to what it was before. We're setting the initial state by calling `update` on each controller, just as the `controller` setter used to. * Fix initial state variable reference Co-authored-by: Esteban Miño <efmino@uc.cl> * fix typeface on login text field (#2610) * Feature/confusables (#2464) * Add confusable warning to SendTo * Highlight confusable characters * Replace zeroWidthPoints characters with ? * Add some notes * Add confusable highlight to confirm screen * Update checkZeroWidth function * Add exclamation mark to Confirm * Add handleConfusables method * Move this into one spot * Add hasZeroWidthPoints * Rename T to Texts * Use reduce * Add homoglyphic tests * Add Modal for confusable on confirm screen * Update snapshot * Use Swaps InfoModal * Increase lineheight on modals * Only display warning if address is not in addressBook * Update snapshot * Make texts lowercase * Remove unused state * Add patch * Display as warning in yelllow when not zero width * Only display confusables warnings if the user is not in addressbook * Add optional chaining for addressBook Co-authored-by: andrepimenta <andrepimenta7@gmail.com> * Add New Zealand Dollar to currency options (#2446) * Add New Zealand Dollar to currency options * Update snapshot to include nzd Co-authored-by: Ricky Miller <ricky.miller@gmail.com> * Move some errors to analytics instead of sentry (#2529) * Move some errors to analytics instead of sentry * Add swaps errors * Change to log just as 1 error * Fix typos * Log can't reach branch servers as analytics * Browser: Failed to resolve ENS name for chainId - log as analytics * Update tests Co-authored-by: Pedro Pablo Aste Kompen <wachunei@gmail.com> * Don't hide url modal on emulator (#2604) * Detox/Fix failing tests (#2607) * fixed all failing tests * remove a Co-authored-by: Ibrahim Taveras <ibrahimtaveras@ibrahims-mbp.myfiosgateway.com> * Upgrade wallet connect (#2552) * This will fix sentry errors with no title by using the extra info as a title (#2565) * Bugfix/android anr (#2603) * updated Sentry SDK and increase the default timeout for ANR to be thrown from 4 to 10 seconds #2498 * updated ANR reporting time to 8 seconds * removed increased timeout and correct sentry integrations vesion * updated pod dependencies * remove typo (#2613) * Upgrade swaps-controller v4 (#2586) * updated lock files (#2614) * Fix/respect custom spend limit on dapp approve modal (#2556) * Add better initial state reset for permission edit modal * Use spendLimitCustomValue for allowance * minimumSpendLimit 1 * Remove minimumSpendLimit prop * Get minimumSpendLimit from EditPermission component * Add MINIMUM_VALUE const * use export const * Coerce minimumSpendLimit to number * Log error * Remove callback from initialState Co-authored-by: Pedro Pablo Aste Kompen <wachunei@gmail.com> * Improve rpc errors logging and removing user rejected errors (#2564) * Improve rpc errors logging and removing user rejected errors * Update for even more cases * Add comments to code * Add trackErrorAsAnalytics * Use typeof * Feature/update seed phrase wording (#2605) * Move login strings to translation file * replace seed phrase with Secret Recovery phrase * Get an video working * get video working off disk * Add SeedPhraseVideo component * Add TODO: * Add SeedPhraseVideo to onboarding * Update snapshots * Add borderRadius * cleanup * Remove placeholder video and add recovery-phrase * Add video-controls * Add cover to video * Add play button to cover * adjust opacity to closer match design * Add marginTop to video on settings page * Remove subtitles for now * Update few remaining instances * Account for single word instances * update snapshots * Update snapshots * RC v2.3.0 (#2621) * bump version numbers * update change log * Implement 'hide zero balance token' setting for token balances on home screen (#2444) * Implement 'hide zero balance token' setting for token balances on home screen * Add localizations * Refactor how balances are detected, add tests * Fix lint, add spacing, create jest snapshots * Fix test, lint * Remove unnecessary proop from test * Remove 'paymentChannelsEnabled' prop that doesn't belong in this patch Co-authored-by: ricky <ricky.miller@gmail.com> * Fix isZero is undefined (#2625) * Fix isZero is undefined * Update app/components/UI/Tokens/index.js Co-authored-by: Esteban Miño <efmino@uc.cl> * add optional chaining Co-authored-by: Esteban Miño <efmino@uc.cl> * Address yarn audit (#2633) * updated change log (#2631) * updated change log * updated change log * updated change log * Exclude native asset from hiding when balance is zero (#2639) * Fix undefined is not an object identities[selectedAddress].importTime (#2643) * Fix undefined is not an object (evaluating 'identities[selectedAddress].importTime * Use accountImportTime for consistency * Safe navbar for iphone 12 (#2645) * safenavbar * Update app/util/Device.js Co-authored-by: ricky <ricky.miller@gmail.com> * mocks * lint * finally Co-authored-by: ricky <ricky.miller@gmail.com> * Fix missing seed phrase updates (#2657) * Fix Balance undefined for deeplink payment requests (#2656) * Check for transactionToName * Account for own accounts * Remove console.log * Use account names from identities * Remove async * Add some safety * Load video over the network (#2663) * updated version code and change logs (#2664) * updated version code and change logs * update change log * added export of iOS artifacts (#2667) * added export of iOS artifacts * updated destination directory * Bump hosted-git-info from 2.8.8 to 2.8.9 (#2670) Bumps [hosted-git-info](https://github.com/npm/hosted-git-info) from 2.8.8 to 2.8.9. - [Release notes](https://github.com/npm/hosted-git-info/releases) - [Changelog](https://github.com/npm/hosted-git-info/blob/v2.8.9/CHANGELOG.md) - [Commits](npm/hosted-git-info@v2.8.8...v2.8.9) Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Key off accounts (#2669) * Key off accounts * Key off accounts in ChoosePassword as well * Fix deploy contract and create token testnets (#2674) * updated change logs * upated version codes and change logs (#2675) Co-authored-by: ricky <ricky.miller@gmail.com> Co-authored-by: Minh <minhle@canva.com> Co-authored-by: Pedro Pablo Aste Kompen <wachunei@gmail.com> Co-authored-by: Esteban Miño <efmino@uc.cl> Co-authored-by: Mark Stacey <markjstacey@gmail.com> Co-authored-by: andrepimenta <andrepimenta7@gmail.com> Co-authored-by: Michael Standen <screaminghawk@gmail.com> Co-authored-by: Ibrahim Taveras <ibrahimtaveras00@gmail.com> Co-authored-by: Ibrahim Taveras <ibrahimtaveras@ibrahims-mbp.myfiosgateway.com> Co-authored-by: David Walsh <davidwalsh83@gmail.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Replace controller context The `context` object previously constructed by the `ComposableController` is no more. Instead each controller now accepts its dependencies directly as constructor parameters, in a similar manner to the extension controllers. This was done in preparation for migrating to BaseControllerV2 and the new controller messaging system - this is just a temporary solution that will let us migrate controllers one at a time. The style of dependency injection here matches the extension (at least with newer controllers anyway). Specific methods and state snapshots are injected rather than entire controllers, to help simplify unit tests and make it easier to understand how controllers interact. The `Engine.context` property was used throughout mobile, so it has been preserved. It is now constructed explicitly, rather than being a re-export of the `ComposableController` context. This PR depends upon MetaMask/core#387 * Pass in function for `getOpenSeaApiKey` rather than string The API key was passed in directly by accident, instead of a function get returned the key. This has been fixed. Co-authored-by: Esteban Miño <efmino@uc.cl> * Update `AccountTrackerController` options The `AccountTrackerController` option `initialIdentities` was replaced with `getIdentities`. The initial identities passed in here were incorrect anyway due to a typo (`initialState.preferencesController` was used instead of `initialState.PreferencesController`). * Fix `getIdentities` handler for `AccountTrackerController` * Set initial controller state The `controllers` setter on `ComposedController` used to be responsible for setting initial state. Since that setter has been removed, the initial state is now set after the controllers have been constructed. This should be functionally equivalent to what it was before. We're setting the initial state by calling `update` on each controller, just as the `controller` setter used to. * Fix initial state variable reference Co-authored-by: Esteban Miño <efmino@uc.cl>

Gudahtt requested a review from a team as a code owner March 22, 2021 14:49

Gudahtt marked this pull request as draft March 22, 2021 14:49

Gudahtt force-pushed the replace-controller-context branch 3 times, most recently from 4cd374c to 453edeb Compare March 22, 2021 16:07

Gudahtt marked this pull request as ready for review March 22, 2021 16:12

Gudahtt force-pushed the replace-controller-context branch from 453edeb to 8720f3a Compare March 22, 2021 16:12

Gudahtt added the No QA Needed Apply this label when your PR does not need any QA effort. label Mar 22, 2021

estebanmino reviewed Mar 23, 2021

View reviewed changes

app/core/Engine.js Outdated Show resolved Hide resolved

Gudahtt commented Mar 23, 2021

View reviewed changes

app/core/Engine.js Outdated Show resolved Hide resolved

Gudahtt mentioned this pull request Mar 23, 2021

Use DI for controller communication rather than context MetaMask/core#387

Merged

Gudahtt force-pushed the replace-controller-context branch from 8720f3a to f538356 Compare March 23, 2021 21:01

Gudahtt commented Mar 23, 2021

View reviewed changes

app/components/Views/ApproveView/Approve/index.test.js Show resolved Hide resolved

Gudahtt added the needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) label Mar 23, 2021

Gudahtt force-pushed the replace-controller-context branch 3 times, most recently from c45d102 to c46aeee Compare March 24, 2021 14:58

Gudahtt force-pushed the replace-controller-context branch from c46aeee to 0eaaa88 Compare April 7, 2021 18:32

omnat added the needs-qa Any New Features that needs a full manual QA prior to being added to a release. label Apr 12, 2021

omnat added this to the Tooling Sprint milestone Apr 12, 2021

ibrahimtaveras00 added QA in Progress QA has started on the feature. and removed No QA Needed Apply this label when your PR does not need any QA effort. needs-qa Any New Features that needs a full manual QA prior to being added to a release. labels Apr 12, 2021

Gudahtt commented Apr 12, 2021

View reviewed changes

app/core/Engine.js Outdated Show resolved Hide resolved

Gudahtt requested a review from estebanmino April 15, 2021 16:37

Gudahtt added the blocked label Apr 16, 2021

rickycodes removed the blocked label Apr 22, 2021

wachunei mentioned this pull request Apr 24, 2021

v4.0.0: Upgrade to controllers v8 MetaMask/swaps-controller#32

Merged

5 tasks

mobularay removed the needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) label Apr 26, 2021

Gudahtt and others added 5 commits April 26, 2021 17:14

Pass in function for getOpenSeaApiKey rather than string

5427019

The API key was passed in directly by accident, instead of a function get returned the key. This has been fixed. Co-authored-by: Esteban Miño <efmino@uc.cl>

Fix getIdentities handler for AccountTrackerController

ac25a11

Gudahtt force-pushed the replace-controller-context branch from b8d1044 to 33f36fe Compare April 26, 2021 19:44

Fix initial state variable reference

07989f1

wachunei mentioned this pull request Apr 28, 2021

Upgrade swaps-controller v4 #2586

Merged

1 task

mobularay removed the next release label Apr 28, 2021

estebanmino approved these changes Apr 28, 2021

View reviewed changes

Merge branch 'develop' into replace-controller-context

0ec5011

estebanmino merged commit c30bc3d into develop Apr 28, 2021

estebanmino deleted the replace-controller-context branch April 28, 2021 17:32

Uh oh!

Conversation

Gudahtt commented Mar 22, 2021

Uh oh!

Gudahtt commented Mar 23, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Gudahtt commented Apr 7, 2021

Uh oh!

ibrahimtaveras00 commented Apr 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ibrahimtaveras00 commented Apr 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Gudahtt commented Apr 12, 2021

Uh oh!

ibrahimtaveras00 commented Apr 12, 2021

Uh oh!

Uh oh!

estebanmino commented Apr 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Gudahtt commented Apr 16, 2021

Uh oh!

rickycodes commented Apr 22, 2021

Uh oh!

mobularay commented Apr 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Gudahtt commented Apr 26, 2021

Uh oh!

mobularay commented Apr 26, 2021

Uh oh!

wachunei commented Apr 27, 2021

Uh oh!

Gudahtt commented Apr 27, 2021

Uh oh!

Gudahtt commented Apr 27, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

ibrahimtaveras00 commented Apr 12, 2021 •

edited

Loading

ibrahimtaveras00 commented Apr 12, 2021 •

edited

Loading

estebanmino commented Apr 16, 2021 •

edited

Loading

mobularay commented Apr 26, 2021 •

edited

Loading