fix(wallet)_: token supported networks #21451
Conversation
Jenkins BuildsClick to see older builds (8)
|
smohamedjavid
force-pushed
the
fix/token-networks
branch
from
015a233
to
f59d0c0
Compare
smohamedjavid
requested review from
alwx,
shivekkhurana,
vkjr,
briansztamfater,
clauxx and
mohsen-ghafouri
There was a problem hiding this comment.
Hi @smohamedjavid thanks for you PR!
I added some minor comments related to the code.
| {:chain-id chain-id | ||
| :raw-balance (money/->bignumber 0) | ||
| :balance "0"}))) |
There was a problem hiding this comment.
nit: since this map is constant, I'd create it outside of this reduce, in a let.
| [chain-ids networks] | ||
| (into #{} | ||
| (mapv (fn [chain-id] | ||
| (first (filter #(or (= (:chain-id %) chain-id) | ||
| (= (:related-chain-id %) chain-id)) | ||
| networks))) | ||
| (keys balances-per-chain)))) | ||
| chain-ids))) |
There was a problem hiding this comment.
I know this code already existed, but there's a big room for improvement, for example: first + filter -> some and using the transducer syntax for into or just use (set) instead of (into #{})
| @@ -390,18 +390,21 @@ | |||
| receiver-networks disabled-from-chain-ids | |||
| from-locked-amounts bridge-to-chain-id] | |||
| :or {token updated-token}} (get-in db [:wallet :ui :send]) | |||
| bridge-tx? (= tx-type :tx/bridge) | |||
| (let [networks (-> (get-in wallet [:networks (if test-networks-enabled? :test :prod)]) | ||
| (network-utils/sorted-networks-with-details)) |
There was a problem hiding this comment.
nit: We are using -> for only one step:
(-> (get-in wallet [])
(do-something)Better to extract wallet, since threading macros of only one step do not have a big benefit:
(-> wallet
(get-in [])
(do-something)| (filter (fn [{:keys [networks]}] | ||
| (pos? (count networks)))) | ||
| (remove #(when hide-token-fn (hide-token-fn constants/swap-tokens-my %)))) | ||
| tokens (->> |
There was a problem hiding this comment.
the usual Clojure code style for ->> is putting the threaded var next to ->>:
;; Do:
(->> a
(f)
(g))
;; Don't:
(->>
a
(f)
(g))| (filter (fn [{:keys [networks]}] | ||
| (pos? (count networks)))) | ||
| (remove #(when hide-token-fn (hide-token-fn constants/swap-tokens-my %)))) |
There was a problem hiding this comment.
We can save another iteration by joining this filter + remove into a single one. (actually we can even join the previous map by using keep, but it's highly probable that the readability will be worse).
There was a problem hiding this comment.
hey @smohamedjavid, I noticed this PR adds changes in similar places as this one, just as an FYI.
Also, wondering if it's possible to isolate the changes in one place only, for instance: when we get the token data from status-go, can we update balances-per-chain to include the missing chains? I may be missing context here though, just wondering if the other changes are necessary.
@clauxx - thanks for the heads up 👍 I guess I missed the context. I was under the impression that we need to display the supported networks below each token as we used to display (depending on the balance-per-chain key). I guess I will change it. Please merge your PR and I will make changes on top of it.
That was my first thought too. I was under the impression that the status-go returns the balance even if it's zero in supported chains for all tokens, but it's different for non-default tokens ( I will quickly add the missing chain while we save it to the app-db. I need to ensure the token list (as we depend on it for supported chains for each token) is fetched before we fetch the tokens for each account so that we can add any missing chains in balance-per-chain. I will make a quick update. |
| @@ -494,10 +494,12 @@ | |||
| (sort-by (juxt :symbol priority) tokens))) | |||
|
|
|||
| (defn tokens-with-balance | |||
| [tokens networks chain-ids] | |||
| [tokens networks token-supported-chains chain-ids] | |||
There was a problem hiding this comment.
When I read name token-supported-chains my brain splits it like "[token-supported] [chains]" which is quite confusing because I can't figure out what are chains that are supported by tokens :)
I'd suggest to rename all similar variables, keywords and functions (17 entries) to make them easier to understand:
'token-supported-chains' -> 'chains-supported-by-token'
'token-supported-chains-by-symbol' -> 'supported-chains-by-token-symbol'
Wdyt?
smohamedjavid
force-pushed
the
fix/token-networks
branch
from
f59d0c0
to
f7dc535
Compare
smohamedjavid
force-pushed
the
fix/token-networks
branch
from
f7dc535
to
7e5df76
Compare
Signed-off-by: Mohamed Javid <[email protected]>
fixes #20988
Summary
This PR fixes the networks/chains supported by the token based on the token list fetched from status-go instead of relying on the
balance-per-chainmap as status-go returns balance for chains only if there is a positive balance.Platforms
Areas that may be impacted
Functional
Steps to test
Prerequisites: A wallet account with a token balance on one network
Wallet > Account > BridgeWallet > Account > SendNot availabletext is not shown on the receiver sidestatus: ready