[#17823] Refactor tokens data app-db #17880
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ulisesmac
requested review from
rasom,
vkjr,
briansztamfater,
smohamedjavid,
J-Son89,
OmarBasem and
mmilad75
ulisesmac
commented
Nov 10, 2023
Comment on lines
+116
to
+122
| add-tokens (fn [stored-accounts tokens-per-account] | ||
| (reduce-kv (fn [accounts address tokens-data] | ||
| (if (accounts address) | ||
| (update accounts address assoc :tokens tokens-data) | ||
| accounts)) | ||
| stored-accounts | ||
| tokens-per-account))] |
There was a problem hiding this comment.
(merge-with merge accounts address) would do the job, but:
1 - It's less performant than just a reduce-kv
2 - If we receive tokens for a non-stored account, it'll lead to many UI bugs. Better only store tokens for a stored account.
There was a problem hiding this comment.
This part of the code transforms the token data in camel-case and fix keywords starting with a number
ulisesmac
commented
Nov 10, 2023
Comment on lines
-85
to
+100
| :custom-subtitle (fn [] [quo/address-text | ||
| {:networks [{:network-name :ethereum :short-name "eth"} | ||
| {:network-name :optimism :short-name "opt"} | ||
| {:network-name :arbitrum :short-name "arb1"}] | ||
| :address address | ||
| :format :long}]) | ||
| :custom-subtitle (fn [] | ||
| [quo/address-text | ||
| {:networks [{:network-name :ethereum :short-name "eth"} | ||
| {:network-name :optimism :short-name "opt"} | ||
| {:network-name :arbitrum :short-name "arb1"}] | ||
| :address address | ||
| :format :long}]) | ||
| :on-press (fn [] | ||
| (rf/dispatch [:show-bottom-sheet | ||
| {:content (fn [] [network-preferences/view | ||
| {:on-save #(js/alert | ||
| "calling on save")}])}])) | ||
| {:content (fn [] | ||
| [network-preferences/view | ||
| {:on-save #(js/alert "calling on save")}])}])) |
There was a problem hiding this comment.
Just style change on (fn[]), nothing important
ulisesmac
commented
Nov 10, 2023
Comment on lines
-37
to
+44
| (save-account {:account account | ||
| :type :color | ||
| :value edited-color | ||
| :theme theme}))) | ||
| (save-account {:account account | ||
| :updated-key :color | ||
| :new-value edited-color | ||
| :theme theme}))) |
There was a problem hiding this comment.
We received :type and when destructuring it was overriding clojure.core/type, IMO it's clearer :updated-key according to the problem solved in this view.
ulisesmac
commented
Nov 10, 2023
Comment on lines
-75
to
+78
| (fn [_ [{:keys [account callback]}]] | ||
| (fn [_ [{:keys [account on-success]}]] |
There was a problem hiding this comment.
callback is too generic, actually, that callback is only executed when the rpc call was successful, so a better name is on-success
Jenkins BuildsClick to see older builds (4)
|
ulisesmac
commented
Nov 10, 2023
Comment on lines
+123
to
+125
| {:db (-> db | ||
| (update-in [:wallet :accounts] add-tokens tokens) | ||
| (assoc-in [:wallet :ui :tokens-loading?] false))}))) |
There was a problem hiding this comment.
tokens data are now inside accounts and the tokens-loading? is in a separate branch
ulisesmac
commented
Nov 10, 2023
Comment on lines
+31
to
+36
| (rf/reg-sub | ||
| :wallet/account-cards-data | ||
| :<- [:wallet/accounts] | ||
| :<- [:wallet/balances] | ||
| :<- [:wallet/tokens-loading?] | ||
| (fn [[accounts balances tokens-loading?]] |
There was a problem hiding this comment.
New sub to get account cards data, previously, it was calculated in the view
ulisesmac
commented
Nov 10, 2023
Comment on lines
-41
to
+29
| (re-frame/reg-sub | ||
| (rf/reg-sub | ||
| :wallet/balances | ||
| :<- [:wallet/accounts] | ||
| :<- [:wallet/tokens] | ||
| (fn [[accounts tokens]] | ||
| (for [{:keys [address]} accounts] | ||
| {:address address | ||
| :balance (calculate-balance address tokens)}))) | ||
| (fn [accounts] | ||
| (zipmap (map :address accounts) | ||
| (map #(-> % :tokens utils/calculate-balance) accounts)))) |
There was a problem hiding this comment.
balances is no longer a sequence of maps, now it's just a map. we no longer need a helper funciton, just extract the address from the map as any other regular key (i.e. (get balances "0x1.."))
ulisesmac
commented
Nov 10, 2023
Comment on lines
-95
to
+88
| (is (= `({:address "0x1" | ||
| :balance 3250} | ||
| {:address "0x2" | ||
| :balance 2100}) | ||
| (testing "a map: address->balance" | ||
| (swap! rf-db/app-db #(assoc-in % [:wallet :accounts] accounts)) | ||
|
|
||
| (is (= {"0x1" 3250 "0x2" 2100} |
There was a problem hiding this comment.
Balances sub test updated
ulisesmac
commented
Nov 10, 2023
| (is | ||
| (= `({:path "m/44'/60'/0'/0/0" |
There was a problem hiding this comment.
We were using ` instead of ' to quote lists. It's unnecessary, actually, if we try to add a var inside, (like I did for the :tokens key), we will need to also evaluate inside (using ~). So changed the backticks to just '
vkjr
approved these changes
Nov 13, 2023
|
Great work! Thank you! |
J-Son89
reviewed
Nov 13, 2023
| @@ -145,10 +145,6 @@ | |||
| (reg-root-key-sub :communities/selected-tab :communities/selected-tab) | |||
| (reg-root-key-sub :contract-communities :contract-communities) | |||
|
|
|||
| ;;wallet | |||
J-Son89
approved these changes
Nov 13, 2023
| (map #(calculate-raw-balance (:raw-balance %) decimals)) | ||
| (reduce +))) | ||
|
|
||
| (defn calculate-balance |
There was a problem hiding this comment.
Would be great to add unit tests for this function (also for every utils public function, but that can be done on a separate PR), wdyt?
There was a problem hiding this comment.
+1 on this! Although I think we should do in a follow up - would be great to have this pr merged in asap as it will likely affect other prs but the changes brought in here are the right way to go 🚀
There was a problem hiding this comment.
Yes, I'm trying to merge it ASAP.
And bout that function, it already existed in the code and it's being used by a subscription (:wallet/balances), that sub already has tests.
I refactored the implementation but it's still passing the tests, so it's already being tested in some way.
briansztamfater
approved these changes
Nov 13, 2023
Note for QAPlease check the Areas that may be impacted in this PR description. |
71% of end-end tests have passedFailed tests (9)Click to expandClass TestActivityMultipleDevicePR:
Class TestCommunityOneDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Expected to fail tests (4)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Passed tests (32)Click to expandClass TestGroupChatMultipleDeviceMergedNewUI:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestCommunityOneDeviceMerged:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityMultipleDeviceMerged:
Class TestActivityMultipleDevicePRTwo:
|
OmarBasem
approved these changes
Nov 14, 2023
pavloburykh
force-pushed
the
17823/refactor-tokens-data-app-db
branch
from
e06f3fe
to
d3ab631
Compare
64% of end-end tests have passedFailed tests (13)Click to expandClass TestActivityMultipleDevicePR:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityOneDeviceMerged:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestGroupChatMultipleDeviceMergedNewUI:
Expected to fail tests (3)Click to expandClass TestGroupChatMultipleDeviceMergedNewUI:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Passed tests (29)Click to expandClass TestActivityCenterContactRequestMultipleDevicePR:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityOneDeviceMerged:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestActivityMultipleDevicePRTwo:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityMultipleDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
|
80% of end-end tests have passedFailed tests (6)Click to expandClass TestCommunityOneDeviceMerged:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Expected to fail tests (3)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestGroupChatMultipleDeviceMergedNewUI:
Passed tests (36)Click to expandClass TestActivityMultipleDevicePR:
Class TestCommunityMultipleDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityOneDeviceMerged:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestActivityMultipleDevicePRTwo:
|
|
Hi @ulisesmac thank you for your work. No issues from my side. PR is ready to be merged |
- Fix `(fn [])` code style. - Avoid overriding clojure.core/type by destructuring the `:type` key. - Split the toast callback to a different function. - `:wallet/save-account` receives `:on-success` instead of `:callback` to improve readability.
- Remove the root sub `:wallet/tokens-loading?`, now it's in app-db in `[:wallet :ui :tokens-loading?]`.
- Remove the root subs `:wallet/tokens`, now the value is returned along with the account data by the sub
`:wallet/accounts`, it's stored in app-db in `[:wallet :address "0x..." :tokens]`.
- Fix the format of the token data returned by the endpoint `"wallet_getWalletToken"` and the fn `js->clj`,
- Addresses are no longer keywords (since keywords mustn't start with a number).
- Keys are now kebab-case.
- Chain-ids are no longer keywords, now they are integers.
- Update tests.
- Move logic to `status-im2.contexts.wallet.common.utils`
- The `:wallet/balances` value returned by the sub had the following structure:
[{:address "0x1...", :balance 12345}
{:address "0x2...", :balance 67890} ...]
This required a helper function to get the balance for an address (`get-balance-by-address`)
It has been changed to a map:
{"0x1..." 12345
"0x2..." 67890, ...}
So now we don't need a helper function (just the hashmap itself or `clojure.core/get`).
- Because of the previous change, now the `get-balance-by-address` has been removed.
- The function `get-account-by-address` has zero uses, so it has been removed.
- The test for the sub has been updated.
This sub returns a vector of maps to render the account cards in the wallet page. This logic was previously in the `view` namespace, but it was completely calculated from the subs `:wallet/accounts`, `:wallet/balances` and `:wallet/tokens-loading?`, so it was clear that's a derived value.
ulisesmac
force-pushed
the
17823/refactor-tokens-data-app-db
branch
from
d3ab631
to
5ca9c30
Compare
87% of end-end tests have passedFailed tests (3)Click to expandClass TestCommunityOneDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Expected to fail tests (3)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestGroupChatMultipleDeviceMergedNewUI:
Passed tests (39)Click to expandClass TestGroupChatMultipleDeviceMergedNewUI:
Class TestActivityMultipleDevicePR:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestActivityMultipleDevicePRTwo:
Class TestCommunityOneDeviceMerged:
Class TestCommunityMultipleDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
|
yevh-berdnyk
pushed a commit
that referenced
this pull request
Dec 8, 2023
* Refactor edit-account view and events:
- Fix `(fn [])` code style.
- Avoid overriding clojure.core/type by destructuring the `:type` key.
- Split the toast callback to a different function.
- `:wallet/save-account` receives `:on-success` instead of `:callback` to improve readability.
* Refactor app-db for `:wallet/tokens` & `:wallet/tokens-loading?`
- Remove the root sub `:wallet/tokens-loading?`, now it's in app-db in `[:wallet :ui :tokens-loading?]`.
- Remove the root subs `:wallet/tokens`, now the value is returned along with the account data by the sub
`:wallet/accounts`, it's stored in app-db in `[:wallet :address "0x..." :tokens]`.
- Fix the format of the token data returned by the endpoint `"wallet_getWalletToken"` and the fn `js->clj`,
- Addresses are no longer keywords (since keywords mustn't start with a number).
- Keys are now kebab-case.
- Chain-ids are no longer keywords, now they are integers.
- Update tests.
* Move logic to calculate `:wallet/balances` and change value returned
- Move logic to `status-im2.contexts.wallet.common.utils`
- The `:wallet/balances` value returned by the sub had the following structure:
[{:address "0x1...", :balance 12345}
{:address "0x2...", :balance 67890} ...]
This required a helper function to get the balance for an address (`get-balance-by-address`)
It has been changed to a map:
{"0x1..." 12345
"0x2..." 67890, ...}
So now we don't need a helper function (just the hashmap itself or `clojure.core/get`).
- Because of the previous change, now the `get-balance-by-address` has been removed.
- The function `get-account-by-address` has zero uses, so it has been removed.
- The test for the sub has been updated.
* Create sub `:wallet/account-cards-data`
This sub returns a vector of maps to render the account cards in the wallet page.
This logic was previously in the `view` namespace, but it was completely calculated from
the subs `:wallet/accounts`, `:wallet/balances` and `:wallet/tokens-loading?`, so it was
clear that's a derived value.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
fixes #17823
Summary
This PR refactors app-db for the wallet data related to tokens and accounts. Full list of changes:
Refactor edit-account view and events:
(fn [])code style.clojure.core/typeby destructuring the:typekey.:wallet/save-accountreceives:on-successinstead of:callbackto improve readability.Refactor app-db for
:wallet/tokens&:wallet/tokens-loading?:wallet/tokens-loading?, now it's in app-db in[:wallet :ui :tokens-loading?].:wallet/tokens, now the value is returned along with the account data by the sub:wallet/accounts, it's stored in app-db in[:wallet :address "0x..." :tokens]."wallet_getWalletToken"and the fnjs->clj,Move logic to calculate
:wallet/balancesand change value returnedstatus-im2.contexts.wallet.common.utils:wallet/balancesvalue returned by the sub had the following structure:[{:address "0x1...", :balance 12345} {:address "0x2...", :balance 67890} ,,,]This required a helper function to get the balance for an address (
get-balance-by-address)It has been changed to a map:
{"0x1..." 12345 "0x2..." 67890, ,,,}So now we don't need a helper function (just the hashmap itself or
clojure.core/get).get-balance-by-addresshas been removed.get-account-by-addresshas zero uses, so it has been removed.Create sub
:wallet/account-cards-dataviewnamespace, but it was completely calculated from the subs:wallet/accounts,:wallet/balancesand:wallet/tokens-loading?, so it was clear that's a derived value.Review notes
All points listed in the issue had been solved:
Platforms
Areas that maybe impacted
status: ready