Use DI for controller communication rather than context#387
Merged
Conversation
Member
Author
|
TODO:
|
cc8238b to
c661433
Compare
c661433 to
bd9e930
Compare
Gudahtt
commented
Mar 19, 2021
Gudahtt
commented
Mar 19, 2021
Gudahtt
commented
Mar 19, 2021
Gudahtt
commented
Mar 19, 2021
Gudahtt
commented
Mar 19, 2021
Gudahtt
commented
Mar 19, 2021
d0d4332 to
c5eb242
Compare
d9bb774 to
345cf02
Compare
Base automatically changed from
add-NFT-contract-address-validation-to-get-URI-method
to
develop
March 20, 2021 00:54
58476c2 to
cbe3330
Compare
96f32ee to
4f0ddb0
Compare
279ca0b to
14a0d00
Compare
Gudahtt
commented
Apr 13, 2021
| const { name } = controller; | ||
| this.context[name] = controller; | ||
| controller.context = this.context; | ||
| this.cachedState?.[name] && controller.update(this.cachedState[name]); |
Member
Author
There was a problem hiding this comment.
Apparently this line is how most controllers on mobile had their initial state set. I had wrongly assumed they were being passed initial state via their constructors.
This initial state setting has been re-implemented in mobile: MetaMask/metamask-mobile@2b80235
Going forward we should set initial state in the constructor instead.
Gudahtt
commented
Apr 13, 2021
shanejonas
previously approved these changes
Apr 13, 2021
8ee50c0 to
c991da5
Compare
c991da5 to
747e9c3
Compare
shanejonas
previously approved these changes
Apr 14, 2021
estebanmino
previously approved these changes
Apr 15, 2021
We now setup inter-controller communication using dependency injection rather than the `context` property. Any dependency a controller has is passed in via a constructor parameter. This was done in preparation for migrating to BaseControllerV2 and the new controller messaging system - it's just a temporary solution that will let us migrate controllers one at a time. The style of dependency injection here matches the extension. Just as we do there, we inject state snapshots, means of subscribing to state, and individual methods rather than entire controllers. This helps to simplify tests and makes it easier to understand how controllers interact.
The `initialIdentities` option for the `AccountTrackerController` has been replaced with a `getIdentities` option that returns the identities on-demand. This bypasses the need to manage a copy of the identity state in the `AccountTrackerController`, and is a bit more similar to how this would be done with the new base controller API.
The constructor JSDoc comment has been fixed. Also a mistakenly removed newline has been restored.
747e9c3 to
f63c6f1
Compare
Member
Author
|
This has been rebased to resolve a conflict with #439 |
Gudahtt
added a commit
that referenced
this pull request
Apr 15, 2021
- Add restricted controller messenger ([#378](#378)) - **BREAKING:** Update minimum Node.js version to v12 ([#441](#441)) - **BREAKING:** Replace controller context ([#387](#387)) - Bump @metamask/contract-metadata from 1.23.0 to 1.24.0 ([#440](#440)) - Update lint rules ([#442](#442), [#426](#426)) - Don't remove collectibles during auto detection ([#439](#439))
Gudahtt
added a commit
to MetaMask/metamask-mobile
that referenced
this pull request
Apr 26, 2021
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
estebanmino
added a commit
to MetaMask/metamask-mobile
that referenced
this pull request
Apr 28, 2021
* 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>
sethkfman
added a commit
to MetaMask/metamask-mobile
that referenced
this pull request
May 12, 2021
* 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>
cronny25
pushed a commit
to cronny25/metamask-mobile
that referenced
this pull request
Jul 27, 2022
* 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>
MajorLift
pushed a commit
that referenced
this pull request
Oct 11, 2023
We now setup inter-controller communication using dependency injection rather than the `context` property. Any dependency a controller has is passed in via a constructor parameter. This was done in preparation for migrating to BaseControllerV2 and the new controller messaging system - it's just a temporary solution that will let us migrate controllers one at a time. The style of dependency injection here matches the extension. Just as we do there, we inject state snapshots, means of subscribing to state, and individual methods rather than entire controllers. This helps to simplify tests and makes it easier to understand how controllers interact.
MajorLift
pushed a commit
that referenced
this pull request
Oct 11, 2023
We now setup inter-controller communication using dependency injection rather than the `context` property. Any dependency a controller has is passed in via a constructor parameter. This was done in preparation for migrating to BaseControllerV2 and the new controller messaging system - it's just a temporary solution that will let us migrate controllers one at a time. The style of dependency injection here matches the extension. Just as we do there, we inject state snapshots, means of subscribing to state, and individual methods rather than entire controllers. This helps to simplify tests and makes it easier to understand how controllers interact.
christina-pan-windsurf
pushed a commit
to christina-pan-windsurf/metamask-mobile
that referenced
this pull request
Aug 19, 2025
* 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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
We now setup inter-controller communication using dependency injection rather than the
contextproperty. Any dependency a controller has is passed in via a constructor parameter. This was done in preparation for migrating to BaseControllerV2 and the new controller messaging system - it's 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 the 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 mobile PR that demonstrates this is here: MetaMask/metamask-mobile#2416