Skip to content
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

Add polling by networkClientId to AccountTrackerController #3586

Merged
merged 35 commits into from
Dec 12, 2023

Conversation

jiexi
Copy link
Contributor

@jiexi jiexi commented Nov 28, 2023

Explanation

Adds the PollingController to AccountTrackerController to help get us ready for multichain integration. Keeps the existing API intact while bringing in the PollingController methods. It's unclear if this controller or the Extension version of the controller will be combined and if so which pattern will used (subscription to block tracker vs time based polling)

References

Changelog

@metamask/assets-controllers

  • BREAKING: AccountTrackerController constructor params object requires getCurrentChainId and getNetworkClientById hooks
  • BREAKING: AccountTrackerController adds a mutex to refresh making it only possible for one call to be executed at time
  • ADDED: AccountTrackerController exposes accountByChainId property in state
  • ADDED: AccountTrackerController implements PollingController and can now poll by networkClientId via the new methods startPollingByNetworkClientId, stopPollingByPollingToken, and stopPollingByPollingToken.
  • CHANGED: AccountTrackerController accepts an optional networkClientId value on the refresh method
  • CHANGED: AccountTrackerController accepts an optional networkClientId value as the last parameter of the syncBalanceWithAddresses method

Checklist

  • I've updated the test suite for new or updated code as appropriate
  • I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate
  • I've highlighted breaking changes using the "BREAKING" category above as appropriate

@jiexi jiexi marked this pull request as ready for review November 28, 2023 23:56
@jiexi jiexi requested a review from a team as a code owner November 28, 2023 23:56
adonesky1
adonesky1 previously approved these changes Dec 1, 2023
Copy link
Contributor

@adonesky1 adonesky1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few non blocking questions/comments! otherwise LGTM! Thanks for cranking these out!

adonesky1
adonesky1 previously approved these changes Dec 7, 2023
Copy link
Contributor

@adonesky1 adonesky1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

shanejonas
shanejonas previously approved these changes Dec 8, 2023
@jiexi jiexi merged commit c823c2b into main Dec 12, 2023
136 checks passed
@jiexi jiexi deleted the jl/mmp-1029/multichain-v1-accounts-tracker branch December 12, 2023 17:18
chrisleewilcox pushed a commit to MetaMask/metamask-mobile that referenced this pull request Feb 16, 2024
## **Description**

Problem: When we change network or account, the native balance of the
ERC-20 tokens is not reset. It shows a wrong value for a couple seconds
and then updates.

We should reset the state and show a skeleton text instead of a wrong
value.

[PR](MetaMask/core#3586) already Merged on core 

## **Related issues**

Fixes: #6101 

## **Manual testing steps**

1. Go to any network where you hold native tokens
2. switch to another network when you have 0 balance
3. you can see that the wrong balance was displayed for a short time

## **Screenshots/Recordings**

<!-- If applicable, add screenshots and/or recordings to visualize the
before and after of your change. -->

### **Before**


https://github.com/MetaMask/metamask-mobile/assets/26223211/d96b474f-60fc-4c4a-a43f-fde18c4a4a26



### **After**



https://github.com/MetaMask/metamask-mobile/assets/26223211/462d8c0e-dcf0-4da7-ae2f-7fdba65b9076


## **Pre-merge author checklist**

- [x] I’ve followed [MetaMask Coding
Standards](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/CODING_GUIDELINES.md).
- [x] I've clearly explained what problem this PR is solving and how it
is solved.
- [x] I've linked related issues
- [x] I've included manual testing steps
- [x] I've included screenshots/recordings if applicable
- [x] I’ve included tests if applicable
- [ ] I’ve documented my code using [JSDoc](https://jsdoc.app/) format
if applicable
- [x] I’ve applied the right labels on the PR (see [labeling
guidelines](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/LABELING_GUIDELINES.md)).
Not required for external contributors.
- [x] I’ve properly set the pull request status:
  - [ ] In case it's not yet "ready for review", I've set it to "draft".
- [x] In case it's "ready for review", I've changed it from "draft" to
"non-draft".

## **Pre-merge reviewer checklist**

- [ ] I've manually tested the PR (e.g. pull and build branch, run the
app, test code being changed).
- [ ] I confirm that this PR addresses all acceptance criteria described
in the ticket it closes and includes the necessary testing evidence such
as recordings and or screenshots.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants