[Feature] Wallet - Network based assets and fiat balance calculation #19150
Conversation
Jenkins BuildsClick to see older builds (42)
|
smohamedjavid
force-pushed
the
feature/wallet-network-filter
branch
4 times, most recently
from
4a2a85c
to
d85ed44
Compare
| currency-symbol (rf/sub [:profile/currency-symbol]) | ||
| all-balances (:balances-per-chain token) | ||
| balance-for-chain (utils/get-balance-for-chain all-balances chain-id) | ||
| crypto-formatted (or (:balance balance-for-chain) "0.00") |
There was a problem hiding this comment.
For some reason, this screen was using the balance key which we shouldn't. Instead, we should rely on raw-balance. So, updated it.
| (get chain-id-map short-name))) | ||
| mainnet-chain-id)) | ||
|
|
||
| (defn network->chain-id |
There was a problem hiding this comment.
Now this method accepts both network short (opt) and full name (optimism).
| @@ -0,0 +1,9 @@ | |||
| (ns status-im.contexts.wallet.db | |||
There was a problem hiding this comment.
Created a ns for wallet-related app-db defaults.
There was a problem hiding this comment.
Hey @smohamedjavid
I'm a bit against this abstraction, I think it's too soon to add it and probably we don't need it.
I don't see a benefit here from marking this as :const, probably we are over :const-ing stuff? I've had problems while reloading some namespaces in the REPL because consts aren't allowed to re-evaluate 🤔 but probably it's just me.
The main reason I'm against is that it hides the keys used, we must navigate to this namespace to know the actual values, this might lead to bugs or unexpected cases since it's easier to forget what are the involved keys, for example, while writing a new event that involves updating an existing map. I believe keys used in event/sub handlers shouldn't be decoupled since they give the whole context and our db is not small.
Additionally, the values written here are totally derived from status-im.constants, I think we could just add a def (or even let) in the namespace/function we want to use it.
cc-ing @J-Son89 to get another perspective.
There was a problem hiding this comment.
I'm a bit against this abstraction, I think it's too soon to add it and probably we don't need it.
The intention is to make a separate def for default values which can be used all over the codebase like when initializing the app-db (with defaults for specific keys), events that reset value and testing those events.
I don't see a benefit here from marking this as :const, probably we are over :const-ing stuff?
I didn't know this caused an issue in REPL 🙏 . We can remove the :const and make it a simple def. 👍
The main reason I'm against is that it hides the keys used, we must navigate to this namespace to know the actual values, this might lead to bugs or unexpected cases since it's easier to forget what are the involved keys, for example, while writing a new event that involves updating an existing map. I believe keys used in event/sub handlers shouldn't be decoupled since they give the whole context and our db is not small.
I felt not to crowd the status-im.db ns. Most of the time, we use REPL (to print app-db) or Re-frisk to see the structure of app-db. Additionally, the default values don't mean those are the only keys in the app-db. For instance, we have defaults for the activity centre and that is not the only key that will be there after login.
I'm fine to remove this ns and use the key value directly. It's just that it will be repetitive across the codebase. and any changes require updates on all places (if not done properly, may introduce bugs as well).
Let me know your thoughts.
There was a problem hiding this comment.
It's just that it will be repetitive across the codebase. and any changes require updates on all places (if not done properly, may introduce bugs as well).
I know, I've had this same problem in different apps I've built using re-frame. I use an IntelliJ feature that returns all the usages of a keyword, but I know, it may still create bugs 🤔
It's just that it will be repetitive across the codebase.
Sometimes I think we over abstract, I've seen components with a constants ns only containing some vars being used in its style ns and they could be defined in the same style ns.
I've also seen a component being spread across many files, each file containing a piece for the main component, I know that's a pattern in JS, but in CLJS we don't always do that.
I prefer to abstract when I clearly see theres a use that will be repeated too constatly, and when the abstraction really makes things easier.
In this case, if you believe abstracting makes things easier, go ahead please.
| @@ -27,11 +17,17 @@ | |||
| :description (i18n/label :t/here-is-a-cat-in-a-box-instead) | |||
| :image (resources/get-themed-image :cat-in-box theme) | |||
| :container-style style/empty-container-style}] | |||
| [gesture/flat-list | |||
There was a problem hiding this comment.
Since the parent already uses a scroll view and we had to remove the flat list as it was throwing the error:
VirtualizedLists should never be nested inside plain ScrollViews with the same orientation because it can break windowing and other functionality - use another VirtualizedList-backed container instead.
Now, it uses the plain old for loop to render the account items.
There was a problem hiding this comment.
Two things:
-
If the parent is always a
scroll-view, we can replace thern/viewwrapping this(doall ...)by[:<>]and provide its styles to thescroll-view(inside:content-container-styleI think) -
If we aren't dyncamically
deref-ing an ratom inside this lazy seq (created byfor), we can omit thedoalland keep it lazy, otherwhise, if we need to realize this seq, then we could useintoso that we no longer need the meta:keygiven, something like:
(into [:<>] ;; or rn/view
(map (fn [{:keys [color address] :as account}]
[quo/account-item {,,,}]))
other-accounts)There was a problem hiding this comment.
If the parent is always a scroll-view, we can replace the rn/view wrapping this (doall ...) by [:<>] and provide its styles to the scroll-view (inside :content-container-style I think)
We can't apply that style to the scroll view as it will affect the whole page.
I guess the second point is the way to go given that we don't deref any ratom.
| :<- [:wallet/current-viewing-account] | ||
| :<- [:wallet/network-details] | ||
| (fn [[account networks] [_ query]] | ||
| (let [tokens (map (fn [token] | ||
| (assoc token | ||
| :networks (utils/network-list token networks) | ||
| :total-balance (utils/total-token-units-in-all-chains token) | ||
| :total-balance-fiat (utils/calculate-balance-for-token token))) |
There was a problem hiding this comment.
Removed the total-balance-fiat key as it's not used.
smohamedjavid
force-pushed
the
feature/wallet-network-filter
branch
from
d85ed44
to
a194746
Compare
smohamedjavid
requested review from
shivekkhurana,
vkjr,
FFFra,
briansztamfater,
J-Son89,
OmarBasem,
mmilad75 and
ulisesmac
smohamedjavid
changed the title
There was a problem hiding this comment.
Hey @smohamedjavid
Thanks for fixing various issues, I left some comments
| (defn- filter-collectible? | ||
| [collectible chain-ids] | ||
| (let [chain-id (get-in collectible [:id :contract-id :chain-id])] | ||
| (contains? chain-ids chain-id))) | ||
|
|
There was a problem hiding this comment.
I think we could improve this function name.
are there some collectibles not containing :chain-id? if not, then this predicate could just be called :collectible? since it is not filtering, the filtering is performed below
There was a problem hiding this comment.
To my knowledge, A Collectible can NOT live without a chain-id (It needs a blockchain). So, It's safe to say the chain-id will present at any given time.
Yes, the actual filter is done below. I will move the filter to a separate function along with this function.
| (fn [[collectibles chain-ids]] | ||
| (filter #(filter-collectible? % chain-ids) collectibles))) | ||
|
|
There was a problem hiding this comment.
Since this functions is the same as below we could extract it to a defn and just use it in these two different subscriptions. wdyt?
There was a problem hiding this comment.
Yes, we will move this to a separate method 👍
| (condp contains? (keyword network) | ||
| #{constants/mainnet-network-name (keyword constants/mainnet-short-name)} | ||
| (get-chain-id | ||
| {:mainnet-chain-id constants/ethereum-mainnet-chain-id | ||
| :sepolia-chain-id constants/ethereum-sepolia-chain-id | ||
| :goerli-chain-id constants/ethereum-goerli-chain-id | ||
| :testnet-enabled? testnet-enabled? | ||
| :goerli-enabled? goerli-enabled?}) | ||
|
|
||
| #{constants/optimism-network-name (keyword constants/optimism-short-name)} | ||
| (get-chain-id | ||
| {:mainnet-chain-id constants/optimism-mainnet-chain-id | ||
| :sepolia-chain-id constants/optimism-sepolia-chain-id | ||
| :goerli-chain-id constants/optimism-goerli-chain-id | ||
| :testnet-enabled? testnet-enabled? | ||
| :goerli-enabled? goerli-enabled?}) | ||
|
|
||
| #{constants/arbitrum-network-name (keyword constants/arbitrum-short-name)} | ||
| (get-chain-id | ||
| {:mainnet-chain-id constants/arbitrum-mainnet-chain-id | ||
| :sepolia-chain-id constants/arbitrum-sepolia-chain-id | ||
| :goerli-chain-id constants/arbitrum-goerli-chain-id | ||
| :testnet-enabled? testnet-enabled? | ||
| :goerli-enabled? goerli-enabled?}))) |
There was a problem hiding this comment.
I think the implementation is a bit complex to understand, is there a way to reduce the parameters passed to get-chain-id?
It's also probably due to condp, I was thinking about using case or a regular cond instead 🤔 but we might need to use some extra helper functions or definitions.
Not needed to change, but if you find a way to improve the readability, it'd be nice. Please consider the previous implementation where the constants were inside the get-chain-id` function.
There was a problem hiding this comment.
I intend to make this method as flexible as can to accept both the network's short name and full name. I agree the implementation is layered. It's needed for this moment to check the network name and then the rest of the conditions to return the correct chain-id.
It's also probably due to condp, I was thinking about using case or a regular cond instead 🤔 but we might need to use some extra helper functions or definitions.
case was my initial thought as well but the docs that The test-constants are not evaluated. They must be compile-time literals, and need not be quoted. So, I had to move to condp.
Please consider the previous implementation where the constants were inside the get-chain-id` function.
We can check for other possibilities. One reason to have the get-chain-id method separately is for the conditional checks (Testnet, Testnest+Goerli, and Mainnet). If not, we will end up repeating the condition in each case.
| (defn filter-tokens-in-chains | ||
| [tokens chain-ids] | ||
| (map #(update % :balances-per-chain select-keys chain-ids) tokens)) |
| @@ -0,0 +1,9 @@ | |||
| (ns status-im.contexts.wallet.db | |||
There was a problem hiding this comment.
Hey @smohamedjavid
I'm a bit against this abstraction, I think it's too soon to add it and probably we don't need it.
I don't see a benefit here from marking this as :const, probably we are over :const-ing stuff? I've had problems while reloading some namespaces in the REPL because consts aren't allowed to re-evaluate 🤔 but probably it's just me.
The main reason I'm against is that it hides the keys used, we must navigate to this namespace to know the actual values, this might lead to bugs or unexpected cases since it's easier to forget what are the involved keys, for example, while writing a new event that involves updating an existing map. I believe keys used in event/sub handlers shouldn't be decoupled since they give the whole context and our db is not small.
Additionally, the values written here are totally derived from status-im.constants, I think we could just add a def (or even let) in the namespace/function we want to use it.
cc-ing @J-Son89 to get another perspective.
| @@ -27,11 +17,17 @@ | |||
| :description (i18n/label :t/here-is-a-cat-in-a-box-instead) | |||
| :image (resources/get-themed-image :cat-in-box theme) | |||
| :container-style style/empty-container-style}] | |||
| [gesture/flat-list | |||
There was a problem hiding this comment.
Two things:
-
If the parent is always a
scroll-view, we can replace thern/viewwrapping this(doall ...)by[:<>]and provide its styles to thescroll-view(inside:content-container-styleI think) -
If we aren't dyncamically
deref-ing an ratom inside this lazy seq (created byfor), we can omit thedoalland keep it lazy, otherwhise, if we need to realize this seq, then we could useintoso that we no longer need the meta:keygiven, something like:
(into [:<>] ;; or rn/view
(map (fn [{:keys [color address] :as account}]
[quo/account-item {,,,}]))
other-accounts)| (keep | ||
| (fn [{:keys [chain-id related-chain-id layer test?]}] | ||
| (let [network-details (get-network-details (if test? related-chain-id chain-id))] | ||
| (assoc network-details | ||
| :chain-id chain-id | ||
| :related-chain-id related-chain-id | ||
| :layer layer)))) | ||
| (map | ||
| (fn [{:keys [chain-id related-chain-id layer]}] | ||
| (assoc (get-network-details chain-id) | ||
| :chain-id chain-id | ||
| :related-chain-id related-chain-id | ||
| :layer layer))) |
There was a problem hiding this comment.
Do we want to keep nils? or it no longer returns nil?
There was a problem hiding this comment.
It doesn't return any nil values as far as I checked (Mainnet, testnet - Sepolia and Goerli).
smohamedjavid
force-pushed
the
feature/wallet-network-filter
branch
from
a194746
to
4b21453
Compare
smohamedjavid
force-pushed
the
feature/wallet-network-filter
branch
from
4b21453
to
c0fcd8c
Compare
smohamedjavid
force-pushed
the
feature/wallet-network-filter
branch
from
c0fcd8c
to
dc95166
Compare
smohamedjavid
force-pushed
the
feature/wallet-network-filter
branch
2 times, most recently
from
49b59a3
to
dd4a4bb
Compare
|
hi @smohamedjavid could you please resolve existing conflicts? |
smohamedjavid
force-pushed
the
feature/wallet-network-filter
branch
from
dd4a4bb
to
216bac7
Compare
|
Hi @smohamedjavid, great job. When I saw the scope of this PR, I thought a lot of issues might be introduced :) But its not. Thank you for your work. Just a few low-priority issues that might be considered as follow-ups. Let me know if they will be addressed in this PR, otherwise, the PR can be merged, and I will create them separately. ISSUE 1: The balances are not shown when the account with some assets is removedSteps to reproduce:
Actual result:Balances are not shown in the network preferences. remove.mp4Expected result:Balances should be shown in the network preferences even after removing an account. |
ISSUE 2 Selected networks during editing flow are not preselected in shared Wallet QRSteps:
Actual result:All networks are predefined in the address, regardless of the selected networks. selectednetworks.mp4Expected result:Only the selected networks should be predefined in the address |
|
I am not sure this is an issue, for me this is just an inconvenience, and didn't see such behavior in other apps, that's why reporting it. Let's see the design team reply https://discord.com/channels/624634427930312714/852533718790570015/1225502769193160894 ISSUE 3: Other checkboxes are unselected when one checkbox is selectedPreconditions:The user has an account with one or more networks selected. Steps to Reproduce:
Actual Result:When a checkbox is selected, other checkboxes are automatically unchecked. checkbox3.mp4Expected Result:After a checkbox is checked, it should not influence the unchecking of other checkboxes. |
Question 1:Is it correct behavior for a user to still see an asset with a 0 balance if the network is deselected where that asset is held? For example, if a user has "wrapped ether" only on the Optimism network and then deselects the Optimism network, should "wrapped ether" still be present with a 0 balance, or should this token not be shown at all? |
|
One more issue, which is probably not related to the PR and might be considered as a follow-up ISSUE 4: The network marks are not shown on the 'send to' page for accountsSteps:
Actual result:'Account 2' is shown with predefined network marks. Expected result:
|
|
@VolodLytvynenko - Thank you 🙏 for testing the mammoth PR in a short time. I would be happy to take these issues as a follow-up, as this PR is already going out of hand in terms of scope. 👍 Adding more information about the issues: Issues related to this PR:
It's probably due to the recovery account. I will check and fix that. Issues NOT related to this PR:
This PR doesn't make any changes to displaying selected networks only in the wallet address of the shell share screen. I believe it's the existing bug. We will fix that.
Yes, this must be frustrating sometimes. We can discuss this with Design and we will apply the behaviour. It will be a quick one.
I believe this is an existing bug. We will fix that.
Yes. That's logically correct. 👍 It's the user's choice to deselect a network which has assets and would expect the balance to reflect that. |
Signed-off-by: Mohamed Javid <19339952+smohamedjavid@users.noreply.github.com>
Signed-off-by: Mohamed Javid <19339952+smohamedjavid@users.noreply.github.com>
Signed-off-by: Mohamed Javid <19339952+smohamedjavid@users.noreply.github.com>
Signed-off-by: Mohamed Javid <19339952+smohamedjavid@users.noreply.github.com>
Signed-off-by: Mohamed Javid <19339952+smohamedjavid@users.noreply.github.com>
smohamedjavid
force-pushed
the
feature/wallet-network-filter
branch
from
216bac7
to
7fe4cfa
Compare
fixes #18418
fixes #18419
fixes #18972
Additionally fixes #18067
Summary
This PR
(UI changes)
ethnot shown on the multichain address (e.g. Account Options bottom sheet)blurtype in theconfirmbutton in the network preferences sheetShell > Share Multichain address(Non-UI changes)
Network.Filter.mp4
Shell.-.Share.Address.mp4
Testing notes
This PR does a refactor of Wallet in various places (more importantly on the balance calculation). Please do a regression test and if any bug is discovered, please verify it against the latest develop.
Platforms
Steps to test
Prerequisites: Make sure you have tokens/collectibles in all three networks
Shell > Shareand Tap on theWallettabMultichaintab on any accountstatus: Ready