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. |
Gudahtt
left a comment
There was a problem hiding this comment.
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
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 |
rekmarks
left a comment
There was a problem hiding this comment.
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