feat: added DeFiPositionsController #5400
Conversation
bergarces
force-pushed
the
feat/MMASSETS-553-create-defi-controller
branch
from
e6486b7 to
5a1fed2
Compare
packages/assets-controllers/src/DeFiPositionsController/DeFiPositionsController.ts
Outdated
Show resolved
Hide resolved
| ); | ||
|
|
||
| this.messagingSystem.subscribe( | ||
| 'NetworkController:stateChange', |
There was a problem hiding this comment.
Added this subscription because we need the positions to be fetched when logging into the wallet, and sicne the selectedAccountChange is not triggered at the start, this seems to be the only way I found to do it.
I'd appreciate to know if there's a more idiomatic way of doing it, as otherwise there's no need to refetch positions every time the chain changes.
There was a problem hiding this comment.
eeem i think you can use the event unlock from the keyring controller for that.
here is an example:
// Subscribe to keyring lock/unlock events.
this.messagingSystem.subscribe('KeyringController:lock', () => {
// Do something
});
this.messagingSystem.subscribe('KeyringController:unlock', () => {
// Do something
});
There was a problem hiding this comment.
If we need to fire something when the wallet is unlocked, we can try using the keyring controller?
Example
There was a problem hiding this comment.
Only concern with subscribing to :stateChange is that this is very frequent.
If we need to use state change subscripions, consider using the 3rd parameter when creating a subscription to use a selector to reduce calls.
this.messagingSystem.subscribe(
'NetworkController:stateChange', // event name
(prevState, newState) => { ... }, // handler
(state) => state.foo, // selector
)There was a problem hiding this comment.
As per @salimtb suggestion, I have now moved towards lock and unlock keyring events. They represent the intention of what we want better and they allow us to start and stop the polling.
packages/assets-controllers/src/DeFiPositionsController/DeFiPositionsController.ts
Outdated
Show resolved
Hide resolved
packages/assets-controllers/src/DeFiPositionsController/DeFiPositionsController.ts
Show resolved
Hide resolved
bergarces
changed the title
packages/assets-controllers/src/DeFiPositionsController/fetch-positions.test.ts
Outdated
Show resolved
Hide resolved
packages/assets-controllers/src/DeFiPositionsController/fetch-positions.ts
Outdated
Show resolved
Hide resolved
packages/assets-controllers/src/DeFiPositionsController/fetch-positions.ts
Show resolved
Hide resolved
|
Need to update readme |
bergarces
force-pushed
the
feat/MMASSETS-553-create-defi-controller
branch
3 times, most recently
from
53f5b3d to
5bb3ea5
Compare
|
@metamaskbot publish-preview |
|
Preview builds have been published. See these instructions for more information about preview builds. Expand for full list of packages and versions. |
| return groupPositions(defiPositionsResponse); | ||
| // eslint-disable-next-line @typescript-eslint/no-unused-vars | ||
| } catch (error) { | ||
| return null; |
There was a problem hiding this comment.
This is how we are going to represent that an error has occurred.
It is not the same as the entry for the address being undefined, which can happen whilst loading.
No point on creating new fields if we do not have custom messages for them.
There was a problem hiding this comment.
Nit: maybe we can remove the // eslint-disable-next-line @typescript-eslint/no-unused-vars
and just
catch { return null }
| 'AccountsController:listAccounts', | ||
| ); | ||
|
|
||
| const results = await reduceInBatchesSerially({ |
There was a problem hiding this comment.
I found this handy function lying around. I would have preferred something like p-queue, which doesn't do batches but limits how many promises execute at once, but really not worth adding a new dependency for that.
We set a healthy limit of 10 addresses at a time, we might have to increase or reduce it.
| @@ -7,6 +7,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 | |||
|
|
|||
| ## [Unreleased] | |||
|
|
|||
| ### Added | |||
|
|
|||
| - Add a new DeFiPositionsController that keeps an update list of DeFi positions for EVM accounts ([#5400](https://github.com/MetaMask/core/pull/5400)) | |||
There was a problem hiding this comment.
(we can do this when releasing) - the changelog may need to go into more detail on the new additions.
- the new Controller class. The Actions/Events it takes.
- Any new exported types or code a user can use.
There was a problem hiding this comment.
Added additional details to the changelog so it's easier for the release.
packages/assets-controllers/src/DeFiPositionsController/DeFiPositionsController.test.ts
Show resolved
Hide resolved
| ); | ||
|
|
||
| this.update((state) => { | ||
| state.allDeFiPositions = allDefiPositions; |
There was a problem hiding this comment.
The batching logic above is relatively new to me. Does it continue with next batch and we filter out batches that fail?
Reason I'm asking, is here we overwrite the defi positions. Is there a possibility of us overwriting some positions for an address that didn't resolve?
There was a problem hiding this comment.
Yes. This will overwrite the whole positions object.
If an address didn't resolve, then that means it errored, and we shouldn't be displaying those positions anyway as they could be stale.
| ], | ||
| "include": ["../../types", "./src"] | ||
| "include": ["../../types", "./src"], | ||
| "exclude": ["**/*.test.ts", "**/__fixtures__/"] |
There was a problem hiding this comment.
Great job excluding this!
This build config is extended from a shared build config.
IDK if we are overwriting or extend the original list of exclusions:
core/tsconfig.packages.build.json
Line 10 in deaf2dc
There was a problem hiding this comment.
It overwrites it, but it is fine to do it this way as the global patterns don't cover __fixtures__ directories and we are excluding **/*.test.ts.
We do not need to worry about **/jest.config.ts as it's in the root of the package, outside of src, and therefore not part of the include.
Explanation
This PR adds a new controller that will be used to fetch DeFi positions for both extension and mobile.
It does so on network state or account change.
Draft PR for extension: MetaMask/metamask-extension#31751
Draft PR for mobile: MetaMask/metamask-mobile#13925
References
Changelog
Pending Changelog edits
@metamask/assets-controllersChecklist