feat: add ability to tap to disable from networks in send & bridge flow #19392
Conversation
Jenkins BuildsClick to see older builds (167)
|
briansztamfater
force-pushed
the
feat/disable-from-chains
branch
5 times, most recently
from
bd53bbf to
5fc2394
Compare
briansztamfater
changed the title
briansztamfater
requested review from
OmarBasem,
J-Son89,
ulisesmac,
shivekkhurana,
rasom,
FFFra,
smohamedjavid,
mmilad75 and
vkjr
|
@status-im/wallet-mobile-devs - this is the sort of pr that will definitely benefit from one of us doing a quick manual test on. |
| :stroke source-color}] | ||
| [svg/path | ||
| {:d | ||
| "M62.9999 1L53.6356 1C42.5899 1 33.6356 9.9543 33.6356 21L33.6356 93C33.6356 104.046 24.6813 113 13.6356 113L5.71778e-05 113" |
There was a problem hiding this comment.
SVG's confuse the hell out of me 😅 . Is there any way to comment the general idea behind these line-1x and line2x views to give some understanding of how this should work, or how you came up with this? 🙏
There was a problem hiding this comment.
It's basically the d value from the exported SVG from Figma. We need to use it as a base and extend it by code to make it stretchable.
src/status_im/constants.cljs
Outdated
| @@ -446,6 +446,8 @@ | |||
| (def ^:const optimism-short-name "opt") | |||
| (def ^:const arbitrum-short-name "arb1") | |||
|
|
|||
| (def ^:const default-address-prefix "eth:opt:arb1:") | |||
There was a problem hiding this comment.
so in Status Go this is referred to as:
:prod-preferred-chain-ids
and in the design it's generally referred to as
"network-preferences".
I think we should be consistent in our language for concepts across the codebase.
Imo address-prefix does not suggest the use case and so I prefer the use of default-network-preference or
default-network-preferences-prefix.
cc @OmarBasem, @shivekkhurana, @smohamedjavid as I think we were having the same discussion in another pr.
I don't mind which we choose but let's at least be consistent across usages 👍
There was a problem hiding this comment.
Agree, I don't have a strong opinion on this, I can change it to whatever name we agree on!
There was a problem hiding this comment.
@J-Son89 I don't mind using another name, but we need to differentiate between the current user's network preferences, and the "address prefix" of a receiver when sending tokens (which is network preferences of the receiver not the sender)
There was a problem hiding this comment.
Both network-preferences and address-prefix are valid names here. The former from the backend perspective, and latter from UI perspective.
But I see this leaking in multiple PR. I have introduced these methods to encapsulate them:
(defn chain-ids->address-prefix [chain-ids])
(defn chain-short-ids->address-prefix [chain-short-ids])
(defn address-prefix->chain-ids [adress-prefix])
(defn address-prefix->chain-short-ids [address-prefix])```
There will probably be a few more to manage all use-cases.
| @@ -20,3 +20,44 @@ | |||
| expected-eip1559-disabled-result (money/bignumber 0.000760406)] | |||
| (is (money/equal-to (utils/calculate-gas-fee data-eip1559-disabled) | |||
| expected-eip1559-disabled-result)))))) | |||
|
|
|||
| (deftest test-find-affordable-networks | |||
| (rf/reg-event-fx | ||
| :wallet/select-send-address | ||
| (fn [{:keys [db]} [{:keys [address recipient stack-id start-flow?]}]] | ||
| (let [[prefix to-address] (utils/split-prefix-and-address address) | ||
| test-net? (get-in db [:profile/profile :test-networks-enabled?]) | ||
| goerli-enabled? (get-in db [:profile/profile :is-goerli-enabled?]) | ||
| prefix (if (string/blank? prefix) constants/default-address-prefix prefix) |
There was a problem hiding this comment.
hmm, prefix is a bit generic imo. Can we make this more descriptive. multi-chain-prefix maybe? 🤔
Also I wonder can we abstract the below code into some utility?
e,g resolve-network-preferences? 🤔
I mean this code block btw.
prefix (if (string/blank? prefix) constants/default-address-prefix prefix)
prefix-seq (string/split prefix #":")
selected-networks (->> prefix-seq
(remove string/blank?)
(mapv #(utils/short-name->id (keyword %) test-net? goerli-enabled?)))
| (debounce/debounce-and-dispatch | ||
| [:wallet/get-suggested-routes {:amount @input-value}] | ||
| 2000) | ||
| (if (= bounce-duration-ms 0) |
There was a problem hiding this comment.
hmm, perhaps you can comment to why this is necessary?
| (rn/use-mount | ||
| (fn [] | ||
| (let [dismiss-keyboard-fn #(when (= % "active") (rn/dismiss-keyboard!)) | ||
| app-keyboard-listener (.addEventListener rn/app-state "change" dismiss-keyboard-fn)] | ||
| #(.remove app-keyboard-listener)))) | ||
| (rn/use-effect | ||
| #(fetch-routes input-num-value (current-limit)) | ||
| #(when (> (count affordable-networks) 0) |
There was a problem hiding this comment.
do we really need two use-effects?
There was a problem hiding this comment.
I initially reused the same useEffect, but then I found we need different bounce durations when changing disabled networks than when changing the input amount.
| {:margin-top 8 | ||
| (defn routes-inner-container | ||
| [first-item?] | ||
| {:margin-top (if first-item? 7.5 11) |
There was a problem hiding this comment.
Yeah there are many places, it can be sometimes noticeable depending on the device dpi. In this case is because of this:
| {:flex 0.5 | ||
| :margin-left margin-left}) | ||
| (def section-label-right | ||
| {:width 135}) |
There was a problem hiding this comment.
why are we hardcoding width? 🤔
There was a problem hiding this comment.
It is fixed width in Figma. 
Spacing will vary thanks to :justify-content :space-between in the container (
| {token-balance :total-balance | ||
| token-symbol :symbol | ||
| token-networks :networks | ||
| token-balances-per-chain :balances-per-chain |
There was a problem hiding this comment.
actually this is something we should avoid,
please see my pr here:
Basically the issue here is that this file is now creating two sets of the same data, one in the account and one in the send data model. It is better to keep one source of truth so we should store the minimum here and instead look up the account balances based off the send token symbol.
There was a problem hiding this comment.
cc @ulisesmac as it's something to consider about the data model. We should probably be sure to watch out for this in other places where we can be duplicating some parts of the db
There was a problem hiding this comment.
I see, and I agree. Maybe we can filter :balances-per-chain and other sensible props from :wallet/wallet-send-token sub to avoid unwanted bugs.
Anyways, just updated the code to use :balances-per-chain from the same sub of your PR 👍
| [data chain-id loading-suggested-routes?] | ||
| (let [network (utils/id->network chain-id) | ||
| new-item-priority (network-priority network)] | ||
| (or (->> data |
There was a problem hiding this comment.
Sorry, some leftover, I updated with more descriptive naming for functions and variables!
| (defn- find-insertion-index | ||
| [data chain-id loading-suggested-routes?] | ||
| (let [network (utils/id->network chain-id) | ||
| new-item-priority (network-priority network)] |
There was a problem hiding this comment.
same for item, can we be more descriptive?
| @@ -1,5 +1,8 @@ | |||
| (ns status-im.contexts.wallet.send.utils | |||
There was a problem hiding this comment.
what's the difference between wallet/common/utils/send and this one?
I see that the files can have different uses, but is there a pattern we are following?
There was a problem hiding this comment.
The difference lies in their scope of use: wallet/common/utils/send likely contains utility functions used broadly across the wallet application, while the other is specialized for use within the sibling namespaces (in this case in events.cljs). Open to move it of course!
There was a problem hiding this comment.
Nope that sounds perfect! 🙌🏼
|
Thanks for the re-review @J-Son89! I pushed some changes and think I covered all the feedback. Let me know if you find anything else! |
briansztamfater
force-pushed
the
feat/disable-from-chains
branch
from
f8c7de1 to
0eabc7b
Compare
|
@status-im/mobile-qa - can you prioritise this pr over other wallet pr's? 🙏 |
briansztamfater
force-pushed
the
feat/disable-from-chains
branch
2 times, most recently
from
a9b4e37 to
0888038
Compare
|
hi @briansztamfater thank you for PR. On issue is found. But, it's okay if it will be better to fix it in a separate follow-up. Let me know if this will be addressed in this PR, otherwise, the PR can be merged ISSUE 1: The routes are not found when entered data is changedSteps:
Actual result:The route searching is not performed routes_f.mp4Expected result:Potential solutions:
|
0% of end-end tests have passedFailed tests (2)Click to expandClass TestWalletMultipleDevice:
|
|
Hi @VolodLytvynenko , thanks for testing! Let's merge this PR first and I can work on that issue on a follow up PR. Let me know if you created the issue or I can create it if you want. Thanks! |
briansztamfater
force-pushed
the
feat/disable-from-chains
branch
from
0888038 to
ee5669c
Compare
|
Hi @briansztamfater thanx! just created #19650 |
fixes #18655
fixes #19440
Summary
Well, this PR turned to be larger than I wanted to be, but I think it is worth it.
The main purpose of this PR is to adds the ability for the user to disable sender networks when tapping a network on the "From" column in the Send or Bridge flow.
For example, this enables a way for the user to select the "From" network when bridging. The steps would be:
Alternatively, user can re-enable the network by tapping again on it.
Demo video of the main added feature:
disablechains.mp4
Now, about what additionally included in this PR:
:wallet/wallet-send-from-values-by-chainand:wallet/wallet-send-to-values-by-chain.Stretchable Network Link Demo
networklinkdemo.mp4
Platforms
Areas that maybe impacted
Functional
Steps to test
status: ready