[Feature] Process wallet accounts from backup on account recovery #20160
Conversation
Jenkins BuildsClick to see older builds (45)
|
smohamedjavid
force-pushed
the
feature/process-wallet-data-from-backup
branch
from
4c99ec0
to
f6676e9
Compare
| (defn get-keypair-operability | ||
| [{:keys [accounts]}] | ||
| (cond | ||
| (some #(= (:operable %) :no) accounts) | ||
| :no | ||
|
|
||
| (some #(= (:operable %) :partially) accounts) | ||
| :partially | ||
|
|
||
| :else | ||
| :fully)) | ||
|
|
||
| (defn- add-keys-to-keypair | ||
| [keypair] | ||
| (assoc keypair :operable (get-keypair-operability keypair))) | ||
|
|
||
| (defn rpc->keypair | ||
| [keypair] | ||
| (-> keypair | ||
| (update :type keyword) | ||
| (update :accounts #(map rpc->account %)) | ||
| add-keys-to-keypair)) | ||
|
|
||
| (defn rpc->keypairs | ||
| [keypairs] | ||
| (let [renamed-data (sort-and-rename-keypairs keypairs)] | ||
| (cske/transform-keys csk/->kebab-case-keyword renamed-data))) | ||
| (->> (map rpc->keypair keypairs) | ||
| (sort-by #(if (= (:type %) :profile) 0 1)))) |
There was a problem hiding this comment.
Refactored key pair data store functions
| @@ -57,16 +57,16 @@ | |||
| (rf/reg-event-fx | |||
| :wallet/get-accounts-success | |||
| (fn [{:keys [db]} [accounts]] | |||
| (let [wallet-accounts (filter #(not (:chat %)) accounts) | |||
| (let [wallet-accounts (data-store/rpc->accounts accounts) | |||
There was a problem hiding this comment.
The filter is already in the rpc->accounts method. Just removed the duplication.
| (rf/reg-event-fx | ||
| :wallet/process-account-from-signal | ||
| (fn [{:keys [db]} [{:keys [address] :as account}]] | ||
| {:db (assoc-in db [:wallet :accounts address] (data-store/rpc->account account)) | ||
| :fx [[:dispatch [:wallet/get-wallet-token-for-account address]] | ||
| [:dispatch [:wallet/request-new-collectibles-for-account-from-signal address]] | ||
| [:dispatch [:wallet/check-recent-history-for-account address]]]})) |
There was a problem hiding this comment.
Introduced this new event to process accounts/addresses individually.
I have used the suffix -from-signal as right now we use this event for processing backed-up data but in the near future, we will handle sync messages (If an account is modified/created/deleted in a paired device, the other device is notified when online and this can handle those scenarios too)
There was a problem hiding this comment.
It's good to dispatch smaller events because they'll take less time to process, the problem is if they are touching the database too often and we have subscriptions mounted that react to it. We may end up having a lot of not needed rerenders.
To fix it we can write in the "subscribed" keys in batches, similar to what I did with collectibles.
Have you checked if this approach is triggering more rerenders than needed, for example, in the wallet home?
There was a problem hiding this comment.
I doubt touching app-db often as this event is dispatched only when we process the backed-up data* (during recovery) or receive any new data* from the paired device (yet to be done).
*watch-only account or Key pair
In the case of recovery, The user won't immediately land in the wallet home after we receive the login signal (goes through Identifier, Notification, and get started page) which gives us a bit of time to update the app-db and unwrapping/update the assets data upon receiving from status-go before the user lands in the wallet tab. Later in the future, we will have a progress screen for backup recovery (just like Desktop).
Yes, there are re-renders whenever the loading state of an account changes and the card in the home updates. Apart from that, I don't see any unwanted re-renders.
| (log/debug ::unknown-wallet-event | ||
| :type event-type | ||
| :event (transforms/js->clj event-js)))))) | ||
| (log/debug ::unknown-wallet-event :type event-type))))) |
There was a problem hiding this comment.
Removed the transform method in the log as it's not needed.
| (rf/reg-sub | ||
| :wallet/tokens-loading? | ||
| :wallet/tokens-loading | ||
| :<- [:wallet/ui] | ||
| :-> :tokens-loading?) | ||
| :-> :tokens-loading) | ||
|
|
||
| (rf/reg-sub | ||
| :wallet/home-tokens-loading? | ||
| :<- [:wallet/tokens-loading] | ||
| (fn [tokens-loading] | ||
| (->> tokens-loading | ||
| vals | ||
| (some true?)))) | ||
|
|
||
| (rf/reg-sub | ||
| :wallet/current-viewing-account-tokens-loading? | ||
| :<- [:wallet/tokens-loading] | ||
| :<- [:wallet/current-viewing-account-address] | ||
| (fn [[tokens-loading current-viewing-account-address]] | ||
| (get tokens-loading current-viewing-account-address))) |
There was a problem hiding this comment.
Made the tokens loading flag granular to the account
There was a problem hiding this comment.
Let's be careful with this change, it may create bugs. Is there a special reason to change this?
Since we aren't able to see two accounts at the same time, I wonder if makes sense to know that an account that is off screen is loaded or loading 🤔
I think this change needs some extra refactors, such as when we create a new account, make sure to properly mark it this key as loading, same when they are received.
I'd like to know the reason to make this change because we may be adding some extra complexity that maybe we don't need.
There was a problem hiding this comment.
Is there a special reason to change this?
When we receive a backed-up watch-only account or keypair from Waku, we process the accounts individually (by storing them in our app-db, fetching assets, and balances) as we can't rely on a single boolean/flag (tokens-loading?) as it will affect existing account(s). So, I made it granular so that each account can refer to its token loading state (in the account screen and account card at home). For the home tab - aggregated tokens and balance - if any accounts are loading, it will show the loading UI.
Additionally, we won't receive all data at once. So, we need to make it a granular approach for easy processing.
Also, it's not good to call the wallet_getAccounts (or any assets RPC) every time we receive any backed-up data as it will make a lot of requests/processing time.
Since we aren't able to see two accounts at the same time, I wonder if makes sense to know that an account that is off screen is loaded or loading 🤔
The existing flag tokens-loading? is global and shows loading for all account cards as it fetches all accounts in one shot. Making it granular at least shows balances for accounts that are loaded while the user can scroll through it.
I think this change needs some extra refactors, such as when we create a new account, make sure to properly mark it this key as loading, same when they are received.
When a new account is added, we fetch all accounts and other chain of events (tokens, collectibles,..etc) that are dispatched as usual, It's the existing feature/code. It ensures the key is marked as loading (for every account).
There was a problem hiding this comment.
❗ Just for information: Every watch-only account is sent in a signal separately and every keypair is sent separately in a signal. For the derived accounts, we need to parse through keypairs and fetch assets for those accounts. Same for the watch-only accounts.
There was a problem hiding this comment.
@smohamedjavid Thanks for the explanation.
Again, just make sure to properly update the code that is related and that may be affected.
Will we ask for manual QA to make sure nothing is broken?
There was a problem hiding this comment.
Will we ask for manual QA to make sure nothing is broken?
Yes, we will do a manual QA for this PR 👍
smohamedjavid
requested review from
seanstrom,
briansztamfater,
J-Son89,
OmarBasem and
ulisesmac
There was a problem hiding this comment.
Hey @smohamedjavid !
THanks for the PR, I left some comments with some questions, it's a big refactor so just want to make sure we have everything under control
src/status_im/contexts/wallet/add_account/create_account/select_keypair/view.cljs
Show resolved
Hide resolved
| (defn get-keypair-operability | ||
| [{:keys [accounts]}] | ||
| (cond | ||
| (some #(= (:operable %) :no) accounts) | ||
| :no | ||
|
|
||
| (some #(= (:operable %) :partially) accounts) | ||
| :partially | ||
|
|
||
| :else | ||
| :fully)) |
There was a problem hiding this comment.
Although the function name is clear enough, I'd suggest renaming the returned keywords to be more explicit, since we may pass these values and it may be confusing.
What about :partially-operable o :partial-operability?
There was a problem hiding this comment.
I don't understand what is get-keypair-operability doing? What does partially operable mean?
Also is the sorting not required?
There was a problem hiding this comment.
What about :partially-operable o :partial-operability?
@ulisesmac - I have no strong opinion as I followed the same value we use in status-go. Additionally, I think it's repetitive as the key for that value is :operable and we include the same as the suffix. But, I'm happy to change it if everyone felt like that.
There was a problem hiding this comment.
I don't understand what is get-keypair-operability doing? What does partially operable mean?
@OmarBasem - Based on the operability of accounts from a key pair we determine whether that keypair is fully operable or not. This helps to sort and show keypairs which don't have keystore files in the wallet settings (Maybe in the wallet home too).
What does partially operable mean?
The Keystore for that keypair (of derived account) is present but we need the user's authentication to make it fully operable. This case happens mostly/only in the profile keypair of the recovered accounts.
To the user, the fully operable and partially operable looks the same. Only the no operable account won't be able to perform any transactions in that account.
Also is the sorting not required?
We still do the sorting to show the profile keypair first (by checking the type of the keypair).
There was a problem hiding this comment.
@smohamedjavid thanks for the clarification. I currently do not see usages of operable in the codebase. Is that functionality yet to be implemented?
There was a problem hiding this comment.
Yes @OmarBasem, it will be used soon in our codebase.
- To sort the missing keypairs in wallet settings
- To show the accounts that are non-operable in different types of account cards in the wallet home
There was a problem hiding this comment.
Now I'm having more questions on this function.
Do we receive a keypair and its accounts? If so, consider adding :as _keypair to the destructuring.
If so, then each account inside a keypair has an operability defined? why? I don't understand it, I believe all the accounts inside a keypair have the same operability as the keypair, since they are derived from it.
I'd really appreciate an explanation :)
There was a problem hiding this comment.
@ulisesmac - As we discussed async about the different keypairs and accounts, I have updated the key name to lowest-operability to NOT confuse it with the operable key of the accounts.
Add a short answer to your question for others to know:
Do we receive a keypair and its accounts? If so, consider adding :as _keypair to the destructuring.
Yes, each keypair map will have an accounts key which has all the accounts generated from it.
If so, then each account inside a keypair has an operability defined? why? I don't understand it, I believe all the accounts inside a keypair have the same operability as the keypair, since they are derived from it.
It depends on the keypair.
Let's take Profile keypair. If you recover the account, the default account is fully operable.
But, if there is any other account generated from it, it will be partially operable for security reasons. status-im/status-desktop#10772
If there is any seed-phrase key pair or private key - key pair, all the accounts generated from it are no operable as its keystore is missing.
Fully -> Keystore files are there and validated
Partially -> Keystore files are there but need to be validated (by the user's password)
No -> There are no keystore files for this keypair
For Partially operable accounts/keypair -> It looks the same as the fully operable to the user. Whenever there is password authentication in the app, we need to call one other method to make partially operable accounts fully operable.
Introducing this new key lowest-operability will help us sort the missing keypairs quickly in the wallet settings.
| (rf/reg-event-fx | ||
| :wallet/process-account-from-signal | ||
| (fn [{:keys [db]} [{:keys [address] :as account}]] | ||
| {:db (assoc-in db [:wallet :accounts address] (data-store/rpc->account account)) | ||
| :fx [[:dispatch [:wallet/get-wallet-token-for-account address]] | ||
| [:dispatch [:wallet/request-new-collectibles-for-account-from-signal address]] | ||
| [:dispatch [:wallet/check-recent-history-for-account address]]]})) |
There was a problem hiding this comment.
It's good to dispatch smaller events because they'll take less time to process, the problem is if they are touching the database too often and we have subscriptions mounted that react to it. We may end up having a lot of not needed rerenders.
To fix it we can write in the "subscribed" keys in batches, similar to what I did with collectibles.
Have you checked if this approach is triggering more rerenders than needed, for example, in the wallet home?
smohamedjavid
force-pushed
the
feature/process-wallet-data-from-backup
branch
from
f6676e9
to
5ed7327
Compare
smohamedjavid
force-pushed
the
feature/process-wallet-data-from-backup
branch
from
5ed7327
to
ef6be8c
Compare
smohamedjavid
force-pushed
the
feature/process-wallet-data-from-backup
branch
2 times, most recently
from
5893e85
to
cf13d0a
Compare
smohamedjavid
force-pushed
the
feature/process-wallet-data-from-backup
branch
2 times, most recently
from
f7ad755
to
ac45fd2
Compare
|
can't check this PR due blocker #20370 |
VolodLytvynenko
force-pushed
the
feature/process-wallet-data-from-backup
branch
from
d4a14f9
to
784cb63
Compare
|
hi @smohamedjavid thank you for PR. Take a look found issues PR_ISSUE 1: Endless spinner after attempting to update wallet on iOSSteps:
Actual result:The spinner does not disappear spinner.mp4Expected result:The spinner should disappear once the update process is completed, indicating that the update was successful. OS:IOS Devices:
Logs |
|
@smohamedjavid You mentioned that this PR is necessary for further work related to the wallet to be unblocked. If you think this issue is better to fix separately, then we can merge the current PR, but please, take this issue as the next for fixing. Otherwise, let me know if this issue is fixed in the current PR. |
smohamedjavid
force-pushed
the
feature/process-wallet-data-from-backup
branch
from
784cb63
to
9cdffc6
Compare
|
@VolodLytvynenko - Thank you for testing this PR 🙏 I have fixed the issue. Please re-test. |
86% of end-end tests have passedNot executed tests (1)Failed tests (4)Click to expandClass TestWalletMultipleDevice:
Class TestCommunityMultipleDeviceMergedTwo:
Expected to fail tests (3)Click to expandClass TestGroupChatMultipleDeviceMergedNewUI:
Class TestWalletOneDevice:
Class TestCommunityOneDeviceMerged:
Passed tests (44)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestDeepLinksOneDevice:
Class TestActivityMultipleDevicePR:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestWalletOneDevice:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityMultipleDeviceMerged:
Class TestActivityMultipleDevicePRTwo:
Class TestCommunityOneDeviceMerged:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestActivityCenterContactRequestMultipleDevicePR:
|
|
Thank you for PR. No issues from my side. PR is ready to be merged |
Signed-off-by: Mohamed Javid <[email protected]>
Signed-off-by: Mohamed Javid <[email protected]>
Signed-off-by: Mohamed Javid <[email protected]>
Signed-off-by: Mohamed Javid <[email protected]>
Signed-off-by: Mohamed Javid <[email protected]>
Signed-off-by: Mohamed Javid <[email protected]>
smohamedjavid
force-pushed
the
feature/process-wallet-data-from-backup
branch
from
9cdffc6
to
b37fe75
Compare
fixes #18875
Summary
This PR:
Before and after comparison
develop.branch.mp4
feature.mp4
Known Issue⚠️
The collectables are not fetched for the recovered accounts until we re-login. This issue is also happening on the Desktop and needs to be investigated in status-go separately.
Platforms
Steps to test
Prerequisite: Make sure the data is backed up in the Waku
A quick regression test is highly appreciated to ensure the changes don't break any existing features.
status: ready