-
Notifications
You must be signed in to change notification settings - Fork 985
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: show network card with zero value on the sender side if the router does not return a route for that network #20152
Conversation
Jenkins BuildsClick to see older builds (30)
|
e2c9e69
to
083b43f
Compare
f3a532f
to
31c85a1
Compare
(fn [{:keys [db]} [address]] | ||
{:db (assoc-in db [:wallet :current-viewing-account-address] address)})) | ||
(fn [{:keys [db]} [{:keys [address network-details update-balance?]}]] | ||
(let [current-token (-> db :wallet :ui :send :token) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm I would like to push back against this change. We're taking a very simple and effective method and coupling it with some send related logic.
Can we handle this some other way? - maybe just a separate event being fired for this particular action? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to the comment in this event:
;; When switching accounts, if there is a selected token we need to update
;; it to reflect balances of the new account
Seems more as a task for a subscription, the subscription that extracts the token from
[:wallet :ui :send :token]
could also receive the current-viewing-account and add the extra data needed.
wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it makes sense, will try to refactor this and let you know!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed this logic, send the updated token from subscription to :wallet/get-suggested-routes
event
Hey @briansztamfater
|
(fn [{:keys [db]} [address]] | ||
{:db (assoc-in db [:wallet :current-viewing-account-address] address)})) | ||
(fn [{:keys [db]} [{:keys [address network-details update-balance?]}]] | ||
(let [current-token (-> db :wallet :ui :send :token) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to the comment in this event:
;; When switching accounts, if there is a selected token we need to update
;; it to reflect balances of the new account
Seems more as a task for a subscription, the subscription that extracts the token from
[:wallet :ui :send :token]
could also receive the current-viewing-account and add the extra data needed.
wdyt?
(filter (fn [[_ {:keys [raw-balance]}]] | ||
(or (not only-with-balance?) | ||
(and only-with-balance? | ||
(and (money/bignumber? raw-balance) | ||
(money/greater-than raw-balance (money/bignumber "0"))))))) | ||
(filter (fn [[_ {:keys [chain-id]}]] | ||
(not (contains? disabled-set chain-id)))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can only use one and
in the filter condition:
(filter (fn [[_ {:keys [raw-balance]}]]
(or (not only-with-balance?)
(and only-with-balance?
(money/bignumber? raw-balance)
(money/greater-than raw-balance (money/bignumber "0"))))))
Have you consider extending the filter below instead of creating a new one? I think performing two filters is more expensive 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I've considered that option, thought that it was more "readable" even if it is a bit less performant, but I have no strong opinion on this tbh, I will change it to have only one filter :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can create two functions to make it more readable, something like
(filter (fn [my-data]
(and (check-conditions-1 my-data)
(check-conditions-2 my-data)))
If you take this approach, just make sure to provide good names :)
(filter | ||
#(not= bridge-to-chain-id %) | ||
sender-token-available-networks-for-suggested-routes) | ||
sender-token-available-networks-for-suggested-routes) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small comment, consider using remove
instead of filter
+ not
+ predicate
.
sender-token-available-networks-for-suggested-routes | ||
(when token | ||
(send-utils/token-available-networks-for-suggested-routes {:balances-per-chain | ||
balances-per-chain | ||
:disabled-chain-ids | ||
disabled-from-chain-ids})) | ||
disabled-from-chain-ids | ||
:only-with-balance? true})) | ||
receiver-token-available-networks-for-suggested-routes | ||
(when token | ||
(send-utils/token-available-networks-for-suggested-routes {:balances-per-chain | ||
balances-per-chain | ||
:disabled-chain-ids | ||
disabled-from-chain-ids | ||
:only-with-balance? false})) | ||
token-networks-ids (when token (mapv #(:chain-id %) (:networks token))) | ||
sender-network-values (when token-available-networks-for-suggested-routes | ||
sender-network-values (when sender-token-available-networks-for-suggested-routes | ||
(send-utils/loading-network-amounts | ||
{:valid-networks (if (= transaction-type :tx/bridge) | ||
(filter | ||
#(not= bridge-to-chain-id %) | ||
token-available-networks-for-suggested-routes) | ||
token-available-networks-for-suggested-routes) | ||
{:valid-networks | ||
(if (= transaction-type :tx/bridge) | ||
(filter | ||
#(not= bridge-to-chain-id %) | ||
sender-token-available-networks-for-suggested-routes) | ||
sender-token-available-networks-for-suggested-routes) | ||
:disabled-chain-ids disabled-from-chain-ids | ||
:receiver-networks receiver-networks | ||
:receiver-networks receiver-networks | ||
:token-networks-ids token-networks-ids | ||
:tx-type transaction-type | ||
:receiver? false})) | ||
receiver-network-values (when token-available-networks-for-suggested-routes | ||
:tx-type transaction-type | ||
:receiver? false})) | ||
receiver-network-values (when receiver-token-available-networks-for-suggested-routes | ||
(send-utils/loading-network-amounts | ||
{:valid-networks (if (= transaction-type :tx/bridge) | ||
(filter | ||
#(= bridge-to-chain-id %) | ||
token-available-networks-for-suggested-routes) | ||
token-available-networks-for-suggested-routes) | ||
{:valid-networks | ||
(if (= transaction-type :tx/bridge) | ||
(filter | ||
#(= bridge-to-chain-id %) | ||
receiver-token-available-networks-for-suggested-routes) | ||
receiver-token-available-networks-for-suggested-routes) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function/event looks very complex, it'd be great if we solve this problem in smaller functions with meaningful names, I know this is out of scope of this PR, but IMO this function needs to be refactored to be easier to understand and maintain.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree, we should even write tests for these events. Will file an issue for it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Created #20162
Hey @ulisesmac, let me answer those questions!
|
FYI, regarding question 2, we talked today with Alisher and cards in the receiver side with 0 won't be shown anymore, but will raise a separate PR with all the discussed changes once they are 100% defined and represented in designs 👍 . |
c18403e
to
c210884
Compare
@J-Son89 @ulisesmac just addressed the comments, can you please re-check? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the explanation and for adressing the comments @briansztamfater
Looks good
67% of end-end tests have passed
Failed tests (14)Click to expandClass TestCommunityMultipleDeviceMerged:
Class TestWalletMultipleDevice:
Class TestWalletOneDevice:
Expected to fail tests (3)Click to expandClass TestCommunityOneDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
Passed tests (35)Click to expandClass TestCommunityOneDeviceMerged:
Class TestDeepLinksOneDevice:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestActivityMultipleDevicePRTwo:
Class TestWalletOneDevice:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestActivityMultipleDevicePR:
Class TestGroupChatMultipleDeviceMergedNewUI:
|
(defn token-available-networks-for-suggested-routes | ||
[{:keys [balances-per-chain disabled-chain-ids]}] | ||
[{:keys [balances-per-chain disabled-chain-ids only-with-balance?]}] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would say only-with-balance
is not very clear. What is the purpose of this variable ? 🤔
(let [wallet-address (get-in db [:wallet :current-viewing-account-address]) | ||
token (get-in db [:wallet :ui :send :token]) | ||
token (or updated-token (get-in db [:wallet :ui :send :token])) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we need both token
and updated-token
? why not just one of these? 🤔
c210884
to
09410c4
Compare
09410c4
to
96a5b6c
Compare
79% of end-end tests have passed
Failed tests (3)Click to expandClass TestWalletOneDevice:
Class TestWalletMultipleDevice:
Passed tests (11)Click to expandClass TestCommunityMultipleDeviceMerged:
|
Hi @briansztamfater ! Thanks for your PR. Failed e2e are not related, you can merge your PR. |
…er does not return a route for that network Signed-off-by: Brian Sztamfater <brian@status.im>
96a5b6c
to
9f5b0aa
Compare
…er does not return a route for that network (#20152) Signed-off-by: Brian Sztamfater <brian@status.im>
fixes #20143
Summary
This PR implements showing network cards with 0 value on the sender side if the account has balance for that token that network when the router does not return a route for that network.
Some examples:
User has ETH on Mainnet, Optimism and Arbitrum
ethavailableonethoptarb.mp4
User has SNT only on Mainnet
sntavailableoneth.mp4
Platforms
Areas that maybe impacted
Functional
Steps to test
status: ready