feat: some limits for wallet section #15989
Conversation
Jenkins BuildsClick to see older builds (21)
|
saledjenic
requested review from
alaibe,
Cuteivist,
alexjba and
Khushboo-dev-cpp
saledjenic
force-pushed
the
feat/issue-15934
branch
from
d2651c2
to
556900d
Compare
There was a problem hiding this comment.
Nice work! Glad to see some limits here!
suggestion: WDYT of unifying the code for the limit popup in Popups.qml? Would remove some of the redundancy.
issue: Looks like the source of truth for the accounts nb might not be correct in some situations (please see the video below). I'm wondering if the limit is correctly computed for synced/imported Status accounts. I think it won't be correctly computed for accounts that migrate from an older version where we didn't had the wallet DB.
Repro:
- Started with 5 accounts (2 status accounts + 3 watch only) and a fresh wallet db. Had to delete the older one because the app started freezing after adding a few accounts. But this might simulate scenarios where users sync/import/migrate the Status account and the app starts with a new wallet DB.
- Add new accounts to hit the limit (limit was reached to 23 accounts + 3 watch only).
- Immediately after the first limit warning I've started deleting the accounts one by one.
- The limit warning was removed only after deleting the 18th account (still had other 3 watch only)
Screen.Recording.2024-08-06.at.11.08.08.mov
question: From the video above looks like the accounts limit is also including the watch only accounts. Is this intended? E.g. The user is allowed to add 17 accounts and 3 watch only accounts.
thought: Have you considered defining the wallet limits in the backend and provide the limits to nim/qml as a constant value. E.g. getAccountsLimit would return 20. It would reduce the status-go traffic
| active: false | ||
| asynchronous: true | ||
|
|
||
| sourceComponent: StatusModal { |
There was a problem hiding this comment.
StatusModal is deprecated and will be removed in the future. It's better to use the StatusDialog
| limitPopup.active = true | ||
| } | ||
|
|
||
| sourceComponent: StatusModal { |
There was a problem hiding this comment.
Same here. StatusDialog is better 😄
I was thinking but didn't want to get with having a separate qml file just for limit warnings since the popup contains only title and a single message as the content of it.
We're checking for capacity on the statusgo side (there are new endpoints for returning the correct capacity), looking at the video you provided, not sure how you get into this situation. How did you add 23 accounts?
Yes, we cannot delete accounts if the user already has more than the limit, but every time he tries to add a new account we should display the message that the limit of 20 (for wallet accs + wo accs) is reached until he removes enough accounts to get under that limit.
So you started from an empty Status folder, created a new account, and added 23 accounts? If so, very weird, cause I tested that and was not able to add more than 20 anyhow, will check again.
Yes, the user can add max 20 accounts in total, watch-only + wallet accounts.
Why do we need that? We already have statusgo calls we do when about to add a new account, those are:
@alexjba thanks a lot for testing this, I will try to reproduce the issue you mentioned above. |
Not an empty Status folder, but I didn't have the wallet DB. The Status account had already 5 accounts (3 of them watch -only). Reached this configuration by accident, but I was thinking this flow might be relevant for people restoring from seed, syncing profiles or migrating from older Status versions.
Yes, I've seen the calls. I was thinking that it might have 2 benefits. Reduce status-go traffic and we wouldn't see issues the one in the video because the limit would also be imposed by QML/nim based on the data we display (E.g. Receive from status-go the max number of accounts and compare with the accounts model count whenever the user clicks on add account). Just a thought to consider if it's difficult to fix the issue in the video with the current approach.
Added more repro steps in the initial comment. But long story short: |
saledjenic
force-pushed
the
feat/issue-15934
branch
from
556900d
to
c1fd1bf
Compare
Maybe that's the case, accounts, watch-only accounts and keypairs are not part of the wallet db, maybe somethings got corrupted that way.
Before opening this PR I tested all that and all worked fine. Also tested it just now and all was good.
In case the user has more than 3 watch-only accounts added and more than 20 accounts added in total and more than 5 keypairs added in total, we do nothing, like we don't delete any of them on our own. Instead when user tries to add any new, we display popup saying "You reached the limit of X ...", which is ok, what we want. Once user deletes enough accounts/keypairs that they are under the limit, the app will allows him to add more, till then he can just edit.
Statusgo is part of the app, so there is no problem in traffic between nim and statusgo, we opted for that approach/technologies and that's it, having the same on the client side will just duplicate the code. Also, that's something shared between desktop and mobile app. Regarding second point, it must not be the difference between the UI and what we have on the statusgo side.
Well, accounts are not stored in the wallet db, so from that point I don't think it's related, but what I do think is that the db was somehow corrupted, maybe due to syncing or whatever, cause otherwise because of conditions we have you shouldn't be able to get available capacity from the statusgo side to reach to 23 added accounts. Very weird. I've tried, but on my side all was fine. @alexjba I've checked all you stated above and tested once more, but all was good on my side. |
|
@saledjenic Thanks for checking this! Indeed it seems the issue is not the wallet DB! Sorry, got confused by all the workarounds needed to keep my account alive 😄 Had a look on this issue and it seems that the limit is checked Screen.Recording.2024-08-07.at.09.19.17.mov |
|
@alexjba yes, that's right! Thanks a lot, updating now. |
saledjenic
force-pushed
the
feat/issue-15934
branch
from
c1fd1bf
to
afcfb42
Compare
|
@alexjba fixed, thanks. |
|
just to confirm, are we bringing watched addresses back in this PR with a limit of 3 addresses or we still want to hide the possibility to add watched addresses in UI? @alexjba @saledjenic |
AFAIK these are 2 independent decisions. |
|
@anastasiyaig adding watch-only addresses is disabled in production, limits added here are added for watch-only addresses as well. |
Added limitations: - allowed adding of max 20 accounts - allowed adding of max 3 watch only accounts - allowed adding of max 5 key pairs (including the profile key pair) - allowed adding of max 20 saved addresses per mode Closes #15934
saledjenic
force-pushed
the
feat/issue-15934
branch
from
afcfb42
to
3b40090
Compare
Related
status-goPR:Added limitations:
Limits applied when running keycard flows that add new key pairs/accounts
Closes #15934
Accounts/keypairs flow:
LimitAccounts.mov
Keycard flow:
LimitKeycardImportFlow.mov