-
-
Notifications
You must be signed in to change notification settings - Fork 192
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
feat: Show Staked ETH position in mobile homepage along with other tokens #4879
feat: Show Staked ETH position in mobile homepage along with other tokens #4879
Conversation
359007a
to
4d90470
Compare
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.
Just some questions but overall LGTM!
d6c3ee7
to
a60a14b
Compare
…troller - add SupportedStakedBalanceNetworks enum to assetsUtils.ts - add AssetsContractController->getStakedBalanceForChain to support stakewise contract balance calls - add ability to includeStakedAssets in AccountTrackerController class options which returns staked balances along with native balance on refresh/sync
b3a6d75
to
7eeffae
Compare
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!
…epage-along-with-other-tokens
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.
Approving to unblock staking features in the short term. We'll likely want to revisit this and optimize it via account API or multicalls in the future.
Explanation
What is the current state of things and why does it need to change?
metamask-mobile
app using theassets-controllers
to get information about account token balances. We need to be able to support getting a new type of asset on mainnet and holesky chains, Staked Ethereum, which is not a token but represents the amount of ETH staked using our products.What is the solution your changes offer and how does it work?
AssetContractController
with a new method,getStakedBalanceForChain
, which gets staked ethereum balances per account from the Stakewise vault contract. We update the AccountTrackerController with options toincludeStakedAssets
and to add agetStakedBalanceForChain
method.AssetContractController.getStakedBalanceForChain
togetStakedBalanceForChain
option property inmetamask-mobile
code and then setincludeStakingAssets
option to the boolean feature flag for ETH Staking on Mobile.balance
and now, if enabled thestakedBalance
as well.Are there any changes whose purpose might not obvious to those unfamiliar with the domain?
stakedBalance
if not on a supported network, and so return undefined vs defaulting to zero hex. If there is an error and we are on a supported network, we want to default to zero hex.If your primary goal was to update one package but you found you had to update another one along the way, why did you do so?
If you had to upgrade a dependency, why did you do so?
References
Are there any issues that this pull request is tied to?
Are there other links that reviewers should consult to understand these changes better?
assets-controllers@38.3.0
Are there client or consumer pull requests to adopt any breaking changes?
Changelog
@metamask/assets-controllers
ADDED: AssetsContractController.getStakedBalanceForChain method to get staked ethereum balance for an address
ADDED: AccountTrackerController options
includeStakedEthereum
andgetStakedBalanceForChain
for turning on staked balance functionality and providing a method to do soChecklist