-
Notifications
You must be signed in to change notification settings - Fork 5k
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 hidden tokens to store #9320
Conversation
ccf2819
to
729bf67
Compare
CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes. |
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.
The code looks good to me!
The only reservation I have is that this list of hidden tokens isn't visible to the user. This could be confusing, especially considering our old behaviour. Maybe we could add it in settings somewhere? 🤔
Or perhaps this is fine for now, and we can add UI later as an enhancement?
8120096
to
f00dd3e
Compare
f00dd3e
to
86cf2a8
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.
Regardless of how this goes, we have some follow-up work to do:
86cf2a8
to
cb4c8f3
Compare
Oops, this is failing one test. Please have another look @PatrykLucka ! Thank you! |
cb4c8f3
to
aff258e
Compare
@rekmarks I think this is ready for another look |
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.
There are a number of methods in the preferences controller related to adding, removing, or otherwise modifying identies/accounts, and they modify accountTokens
. Shouldn't we also modify accountHiddenTokens
in these cases?
2e00a59
to
4993fbd
Compare
5d9e1b6
to
ad193c9
Compare
@rekmarks you're right, I updated preferences controller, please take a look. |
From a behavioral standpoint this PR fixes the issue with tracking, and persisting, tokens that the user hides. Whether we can/should optimize this to prevent duplicates of the accountHiddenTokens and hiddenToken is a point of contention, but it acts similiarly to how we track tokens and accountTokens. Also to note, for tokens under a custom network there is no way to distinguish different sets of custom network hidden tokens, they are all under the LGTM! |
PR adds hidden tokens to store, to prevent adding them automatically if balance > 0 is detected.
Fixes #5055