Update wallet delegate status on refresh #77
Conversation
- use accountLabel pipe consistently (don't user label if wallet is a delegate)
| if (response.success) { | ||
| this.wallet.deserialize(response.account); | ||
| if (save) { this.saveWallet(); } | ||
| this.arkApiProvider |
There was a problem hiding this comment.
Here we can avoid unnecessary requests: if (!wallet.isDelegate)
There was a problem hiding this comment.
right... it's not possible to unbecome a delegate then right? If so should I also remove the corresponding logic in the ensureWalletDelegateProperties method?
There was a problem hiding this comment.
Yes, please. To make it more readable.
| @@ -217,33 +213,15 @@ export class MyApp implements OnInit, OnDestroy { | |||
| }); | |||
| } | |||
|
|
|||
| // Verify if any account registered is a delegate | |||
There was a problem hiding this comment.
I don't see a good reason to remove this, the purpose is to save mobile data
There was a problem hiding this comment.
hmm I think I don't understand you? I removed this, because the only thing which it did was set isDelegate to true on the wallets. However the isDelegate state is now set when creating / importing a wallet and also if the user has become a delegate (refresh method). So this method shouldn't be required anymore?
There was a problem hiding this comment.
Well, if you register a delegate in the desktop wallet will only be displayed when you open the wallet
There was a problem hiding this comment.
Well this only fetches the 102 delegates anyway I think. But you're still right and it probably doesn't hurt to have it. I will readd it again.
There was a problem hiding this comment.
You're right, this change to only 102 delegates was made by a PR after I deployed. So this method may not make more sense 😥
There was a problem hiding this comment.
I see. Should i do anything there or leave it like it is?
Another alternative would be to check the delegate status for all wallets in the WalletsListPage (if they are not already a delegate) in the OnInit method. However this would mean potentially unneeded requests. What do you thik?
There was a problem hiding this comment.
Nah, your solution is good, just add the refresh condition
There was a problem hiding this comment.
Sorry: Which refresh condition are you talking about? 😅
There was a problem hiding this comment.
=> that means I did not change anything here (see 4ffa368)
- simplify set method (it's not possible to unbecome a delegate)
Note:
I wrote the
ensureWalletDelegatePropertiesso that the wallet is only saved when the delegate state actually changed, i.e. you became a delegate.Also what's different now is, that the wallet can actually be saved after a "walletRefresh" (method is called) happens, before we never saved it - hope that's not a problem.