-
-
Notifications
You must be signed in to change notification settings - Fork 242
chore: remove usages of NetworkController:getState in EarnController #6153
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
chore: remove usages of NetworkController:getState in EarnController #6153
Conversation
@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.
|
243e64f
to
8886aa1
Compare
8886aa1
to
6a9a827
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.
|
7853624
to
d05158e
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.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
…in-id-in-earn-experiences
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍🏽, we may need some more coverage on the constants file @ameliejyc
|
||
readonly #env: EarnEnvironments; | ||
|
||
constructor({ | ||
messenger, | ||
state = {}, | ||
addTransactionFn, | ||
selectedNetworkClientId, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, we may have followed this pattern in the past, but this will require that NetworkController be initialized before EarnController. We are trying to eliminate dependencies on controllers like this and make it so that it doesn't matter which order controllers are initialized, they will "just work".
I was going to suggest that we make selectedNetworkClientId
optional, then add an init
method where we pull the selected network via NetworkController:getState
. But that's a bit similar to what we were doing before, and access to NetworkController:getState
seems to be getting removed in this PR. The PR description claims that this is due to removing reliance on the global network, but selectedNetworkClientId
is the global network. So I'm a bit confused on what this change in particular achieves. Are we also trying to increase performance?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @mcmire,
The main objective here is to remove the direct dependency on the globalNetwork selector, which currently relies on NetworkController.state.selectedNetworkClientId.
Instead of accessing the network directly from the NetworkController
, we want to pass selectedNetworkClientId
as a parameter to the EarnController constructor during initialization.
For now, since the globalNetwork selector is still in place on mobile, it will continue to be used. But in the upcoming PR, once it's removed, we’ll update the initialization to pass the enabledNetwork value instead.
So on the client side, the initialization will look like this:
new EarnController({
...,
selectedNetworkClientId: Engine.context.NetworkController.state.selectedNetworkClientId,
...
})
Passing the network value from the client has two main benefits:
- It allows us to easily swap or override the value as needed.
- It eliminates the tight coupling with NetworkController.selectedNetworkClientId, improving modularity and flexibility.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, thanks for the context. How will enabled networks work on mobile? Will multiple enabled networks be allowed like on extension or will only one be allowed?
Also, you mention that this eliminates the tight coupling to NetworkController.selectedNetworkClientId
, however, it seems that we are still listening to networkDidChange
below, so we are still coupled to the global network. Do we have plans to change that down the road as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we're still getting the selectedNetworkClientId
from the NetworkController
, this doesn't seem to change how coupled this is with the NetworkController
at all. I don't follow how this makes it easier to swap or override the value either - do you mean in a test? The messenger class was designed to be easily used in tests for mocking external code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mcmire This is just the initial step. The next phase will involve listening to events from the new NetworkEnablementController
and dropping the dependency on networkDidChange
. That said, the related PR is still under review and the controller hasn’t been published yet.
On mobile, the enabled network behavior will align with the extension.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Gudahtt Hmm, I’m not sure I fully follow. Currently, we’re still using selectedNetworkClientId
from the NetworkController
and passing it in param because the global network selector hasn’t been removed on mobile yet. Once that’s done, we’ll update this value directly from the mobile UI codebase at the initialisation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My point was that if you are passing selectedNetworkClientId
from the NetworkController
to the EarnController
, doing so via a constructor parameter is not less coupled than doing so through the messenger, it's equally coupled. The goals you stated above are not achieved by this PR.
But that was before I heard about your intention to pass in a network client ID from UI state. I can see how this might be a step towards the goals you referenced even if it doesn't achieve them alone. This makes a bit more sense with that context, at least if there is some obstacle to implementing the network enablement controller on mobile (otherwise why not go straight to using that?).
… to refreshPooledStakingVaultDailyApys
@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.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Redundant API Calls in Loop
The refreshEarnEligibility
method is called redundantly inside the refreshPooledStakingData
loop for each supported chain. As eligibility is a global, account-specific property (a compliance check against an address blocklist) and the method is chain-agnostic, this leads to unnecessary duplicate API calls and potential race conditions. It should be called once outside the loop.
packages/earn-controller/src/EarnController.ts#L650-L671
core/packages/earn-controller/src/EarnController.ts
Lines 650 to 671 in c209037
for (const chainId of this.#supportedPooledStakingChains) { | |
await Promise.all([ | |
this.refreshPooledStakes({ resetCache, address, chainId }).catch( | |
(error) => { | |
errors.push(error); | |
}, | |
), | |
this.refreshEarnEligibility({ address }).catch((error) => { | |
errors.push(error); | |
}), | |
this.refreshPooledStakingVaultMetadata(chainId).catch((error) => { | |
errors.push(error); | |
}), | |
this.refreshPooledStakingVaultDailyApys({ chainId }).catch((error) => { | |
errors.push(error); | |
}), | |
this.refreshPooledStakingVaultApyAverages(chainId).catch((error) => { | |
errors.push(error); | |
}), | |
]); | |
} |
Was this report helpful? Give feedback by reacting with 👍 or 👎
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One more thing!
@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.
|
@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.
|
Explanation
The Global Network Selector is being removed and these changes for the EarnController ensure that we are not relying on global state via
NetworkController:getState
. Instead, information required such as the selected network client ID or chain ID is passed in contextually.NetworkController:getState
. Instead:selectedNetworkClientId
to be passed in when constructed and it is used ininitializeSDK
.chainId
parameter with fallback to Ethereum Mainnet used in Pooled Staking data calls:refreshPooledStakingVaultApyAverages
,refreshPooledStakingVaultDailyApys
,refreshPooledStakingVaultMetadata
andrefreshPooledStakes
chainId
parameter required in all Lending transaction calls:executeLendingTokenApprove
,executeLendingWithdraw
andexecuteLendingDeposit
chainId
parameter required in Lending data calls:getLendingMarketDailyApysAndAverages
andgetLendingPositionHistory
. The other Lending data fetches do not require a chainId.NetworkController:networkDidChange
rather thanNetworkController:stateChange
as this is a more granular and less frequent event to listen to.References
Checklist