fix: no routes are found in case of valid amount in fiat is entered #20000
Conversation
Jenkins BuildsClick to see older builds (52)
|
briansztamfater
force-pushed
the
fix/routes-fetch-fiat
branch
2 times, most recently
from
1a6106e
to
f6f077c
Compare
briansztamfater
changed the title
briansztamfater
requested review from
ulisesmac,
OmarBasem,
vkjr,
J-Son89,
BalogunofAfrica and
mmilad75
briansztamfater
force-pushed
the
fix/routes-fetch-fiat
branch
from
0bf1584
to
efcff9c
Compare
| @@ -12,30 +12,22 @@ | |||
| {:padding-horizontal 20 | |||
There was a problem hiding this comment.
I am curious what are the styling changes for as I don't see ui-related problem in the issue description
There was a problem hiding this comment.
Well, the main issue doesn't specify UI bugs, but after fixing the main issue, some other issues started to be more noticeable. For instance:
- Visual overflow of the suffix text of the input when swapping mode to fiat and the number is too big. Needed to refactor a bit the views and styles of Token Input component.
- Don't allow the user to use more decimals than the token supports.
- Fiat character symbol was not showing in the conversion.
- Fiat character symbol was not used when showing the max amount.
- Many trailing zeroes issues when leading with large numbers.
- Routes being fetched when swapping to fiat, and vise versa
Added this in the description, will add some comments also to specify those changes.
|
Hey @briansztamfater, can you please review your code a little bit to explain are the other bugs being fixed? Also would be good if you can add a video to PR description. |
|
🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 20000 pr's 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 |
| @@ -29,3 +30,13 @@ | |||
| if `num` exceeds a given bound, then returns the bound exceeded." | |||
| [number lower-bound upper-bound] | |||
| (max lower-bound (min number upper-bound))) | |||
|
|
|||
| (defn remove-trailing-zeroes | |||
There was a problem hiding this comment.
probably should add a test to cover this functionality? 🤔
is this really core "numbers" or is it more specific to the wallet also? 🤔
There was a problem hiding this comment.
Well, this function was already in a common file for wallet, but moved to a utils as it is a general purpose function and needed to use it un token input component
There was a problem hiding this comment.
Now that we have a focus on this function, maybe we can simplify the logic a bit?
(let [[integral fractional] (string/split (str num) #"\.")
fractional-fixed (some-> fractional
(string/replace #"0+$" ""))]
(if fractional
(str integral "." fractional-fixed)
integral))I think in this implementation is clearer what we do to the number 👀 wdyt?
| (when (and active-screen? | ||
| (> (count token-available-networks-for-suggested-routes) 0) | ||
| (not lock-fetch-routes?)) | ||
| (fetch-routes value valid-input? 2000))) |
There was a problem hiding this comment.
btw what is 2000 here? perhaps it's better to put this in a named var, or instead use a map for the arguments so it's obvious of the uses without looking at the function
There was a problem hiding this comment.
It is an arbitrary time for debouncing-dispatch fetch routes event. Here #19100 we changed it to 2000, I can add it in a named var of course.
There was a problem hiding this comment.
I think just a map for params explains it.
(fetch-routes {:value value
:valid-input? valid-input?
:debounce-ms 2000})
There was a problem hiding this comment.
I think this number 2000 is used in multiple places, we can refactor it to a constants file
| @@ -92,36 +84,49 @@ | |||
| (oops/oget "nativeEvent.selection") | |||
| (js->clj :keywordize-keys true) | |||
| (on-selection-change))))] | |||
| (fn [{:keys [token customization-color show-keyboard? crypto? currency value error? selection] | |||
| (fn [{:keys [token customization-color show-keyboard? crypto? currency value error? selection | |||
| handle-on-swap] | |||
| :or {show-keyboard? true}}] | |||
There was a problem hiding this comment.
it can be quite hard to distinguish what changes you have actually made in code blobs like this.
It would be nice if you could leave some comments to help find the changes :)
There was a problem hiding this comment.
The main reason is this one https://github.com/status-im/status-mobile/pull/20000/files#r1601228153
| :placeholder-text-color (style/placeholder-text theme) | ||
| :auto-focus true | ||
| :ref set-ref | ||
| :placeholder "0" |
There was a problem hiding this comment.
is this placeholder correct? Probably should be an i81n label so it can be set.
There was a problem hiding this comment.
I think it is fine, the placeholder should be "0" in this case as this is the token amount input
src/status_im/constants.cljs
Outdated
| @@ -513,3 +513,7 @@ | |||
| (def ^:const community-joined-notification-type "communityJoined") | |||
|
|
|||
| (def ^:const default-telemetry-server-url "https://telemetry.status.im") | |||
|
|
|||
| (def currency-symbols | |||
There was a problem hiding this comment.
hmm, do we not already have currencies already defined somewhere? 🤔
I don't think we need to start adding this list. What exactly did you need it for?
cc @smohamedjavid
There was a problem hiding this comment.
Hmmm I found one in quo.foundations.common. Not sure if we should use it from status-im namespace?
There was a problem hiding this comment.
This I'm not sure of, but we get the symbol from the sub I think, which comes from Status-Go.
I don't think we should add this map tbh, probably need to nuke the one in quo.foundations.common too
There was a problem hiding this comment.
I think they were added temporarily. We need to fetch currencies data from status-go (probably settings team will work on that)
There was a problem hiding this comment.
We use the existing list from legacy.status-im.utils.currency ns for currency list display and selection. But, it's needs to be fetched from status-go as @OmarBasem mentioned, to serve as a single point of truth for both Mobile and Desktop.
I believe those APIs are wallet_getCachedCurrencyFormats and wallet_fetchAllCurrencyFormats.
(probably settings team will work on that)
Not sure about that. 😅
There was a problem hiding this comment.
I ended up using :profile/currency-symbol and :profile/currency subs. No need for those maps from what I've seen, so I also removed quo.foundations.common file which only has that definition and was only used on Token Input.
| :right 0}) | ||
| (defn input-container | ||
| [window-width] | ||
| {:width (- window-width 120) |
There was a problem hiding this comment.
Token icon width + swap button width + spacing.
I will add a comment to make it more clear.
There was a problem hiding this comment.
You can also just put 120 in a named def
There was a problem hiding this comment.
What if we change the token icon width, the styling here will probably break. I think we need to have those 3 styles defined (and be used in the relevant components), and then the sum defined.
Or maybe there could be another approach without using that number here
There was a problem hiding this comment.
There was a bug that makes a visual overflow of the suffix text of the input when swapping mode to fiat and the number is too big. So I refactored a bit this component to fix those issues.
| (str (number/remove-trailing-zeroes | ||
| (.toFixed (/ num-value conversion) (or crypto-decimals 2))) |
There was a problem hiding this comment.
Numbers can be of 0.000100000, so need to remove trailing zeroes
There was a problem hiding this comment.
Would be good to add a test for the remove-trailing-zeroes util
| [button/button | ||
| {:icon true | ||
| :icon-only? true | ||
| :size 32 | ||
| :on-press handle-on-swap | ||
| :type :outline | ||
| :accessibility-label :reorder} | ||
| :i/reorder]])))) |
There was a problem hiding this comment.
Moved button to this view, not sure why it was outside of it, but refactored to fix token-label overflowing when the input is too large.
| (defn- make-limit-label-fiat | ||
| [amount currency-symbol] | ||
| (str currency-symbol amount)) |
There was a problem hiding this comment.
Formatting for fiat was incorrect.
Previously it showed like: 100 EUR, now it should show like €100
There was a problem hiding this comment.
I think that depends on the language/region?
I've seen in some places in Europe they use 100 € and in USA and Mexico we use € 100.
So we might need to move this to translations
| :on-swap #(reset! crypto-currency? %) | ||
| :value input-amount | ||
| :on-swap (fn [swap-to-crypto-currency?] | ||
| (set-just-toggled-mode? true) |
There was a problem hiding this comment.
Currently, when input value changes and the value is valid, routes are fetched. When swapping, the input value changes, the value is valid because it is still the same value in crypto, but routes should not be fetched.
This flag is used to prevent routes being fetched when swapping.
| amount (if @crypto-currency? | ||
| input-amount | ||
| (number/remove-trailing-zeroes | ||
| (.toFixed (/ input-amount conversion-rate) | ||
| crypto-decimals))) |
There was a problem hiding this comment.
Even if we are in "fiat mode", we need to calculate the equivalent in crypto token because it is what we are actually sending / bridging. Fiat values are just for visual representation.
| amount-text (str (.toFixed (js/parseFloat send-amount) | ||
| (min token-decimals 6)) |
There was a problem hiding this comment.
This is the amount string we show to the user. We cap the decimals to 6 to not overflow.
| (rn/use-effect | ||
| #(when input-error (debounce/clear-all)) | ||
| [input-error]) |
There was a problem hiding this comment.
This is a bit out of scope, we can move it to another PR, but I think it is fine to leave it here as it includes many minor fixes for the input.
Let's say user wants to send SNT, the balance is 10 SNT.
Then the user starts typing "1", causing an event to fetch routes is debounced to dispatch in 2 seconds.
Then the user types another "1", now the input shows "11". But the user has only 10 SNT, so the input enters in an error state.
Because the first event was already in the dispatch queue, routes will be fetched with amount of "1" even if the user continued typing an invalid amount.
So, calling debounce/clear-all fixes that.
| (set-input-state #(controlled-input/add-character % c)))) | ||
| (let [new-text (str input-amount c) | ||
| max-decimals (if @crypto-currency? crypto-decimals 2) | ||
| regex-pattern (str "^\\d*\\.?\\d{0," max-decimals "}$") |
There was a problem hiding this comment.
When typing, we limit the input to allow decimals up to the decimals amount of the token or 2 if it is a fiat amount.
|
@J-Son89 @OmarBasem added more context all over the PR, also improved the description describing related issues that were fixed here and added a demo video |
There was a problem hiding this comment.
@briansztamfater thanks for adding clarifications to your code. Left some comments 👍
| :right 0}) | ||
| (defn input-container | ||
| [window-width] | ||
| {:width (- window-width 120) |
There was a problem hiding this comment.
What if we change the token icon width, the styling here will probably break. I think we need to have those 3 styles defined (and be used in the relevant components), and then the sum defined.
Or maybe there could be another approach without using that number here
| (str (number/remove-trailing-zeroes | ||
| (.toFixed (/ num-value conversion) (or crypto-decimals 2))) |
There was a problem hiding this comment.
Would be good to add a test for the remove-trailing-zeroes util
| {:style {:width "100%" | ||
| :flex-direction :row} |
src/status_im/constants.cljs
Outdated
| @@ -513,3 +513,7 @@ | |||
| (def ^:const community-joined-notification-type "communityJoined") | |||
|
|
|||
| (def ^:const default-telemetry-server-url "https://telemetry.status.im") | |||
|
|
|||
| (def currency-symbols | |||
There was a problem hiding this comment.
I think they were added temporarily. We need to fetch currencies data from status-go (probably settings team will work on that)
| % | ||
| (let [value (controlled-input/input-value %) |
There was a problem hiding this comment.
This close usages of % can be confusing
There was a problem hiding this comment.
Yes, better to give a name to that var.
@briansztamfater I prefer to follow the approach @ilmotta once shared with me: use the shorthand (#()) only when its a simple one liner function, otherwise prefer to use (fn []) because it's clearer what is received or even if it's empty it's clearer than looking for a % in the function.
| (set-input-state #(controlled-input/add-character % c)))) | ||
| (let [new-text (str input-amount c) | ||
| max-decimals (if @crypto-currency? crypto-decimals 2) | ||
| regex-pattern (str "^\\d*\\.?\\d{0," max-decimals "}$") |
There was a problem hiding this comment.
Probably better to refactor this regex pattern to constants file
| (when (and active-screen? | ||
| (> (count token-available-networks-for-suggested-routes) 0) | ||
| (not lock-fetch-routes?)) | ||
| (fetch-routes value valid-input? 2000))) |
There was a problem hiding this comment.
I think this number 2000 is used in multiple places, we can refactor it to a constants file
| sender-network-values) | ||
| (not (nil? routes)) | ||
| (not loading-suggested-routes?)) | ||
| input-error (controlled-input/input-error input-state)] |
There was a problem hiding this comment.
I mentioned this recently, but just to draw attention to how it keeps getting longer. We have subs, dispatch events, utils, and defined values mixed together. We need to put some thought into cleaning it up a bit @J-Son89 @ulisesmac
There was a problem hiding this comment.
@OmarBasem
Sorry, I don't get it, do you mean that we have events, subs, utils and defs defined in the same file?
There was a problem hiding this comment.
I agree there are lots of var definitions in this file, it is getting bigger and it deserves a refactor.
Not sure if we want to refactor now? Maybe it is too risky as it is a complex and core component for wallet, I would vote for refactor once we have a stable version so we can easily revert commits in case we break things while refactoring. But I am also open to listen to other opinions, no strong opinion on this tbh.
There was a problem hiding this comment.
Hey @briansztamfater thanks for the PR and for addressing the bugs found!
The video and code look really good besides a couple of small comments.
💯
Maybe we want to ask QA to confirm the bugs have been properly solved
| (defn- token-label | ||
| [{:keys [theme value text]}] | ||
| [rn/view | ||
| {:style style/token-label-container | ||
| :pointer-events :none} | ||
| [rn/text-input | ||
| {:max-length 12 | ||
| :style style/text-input-dimensions | ||
| :editable false | ||
| :placeholder "0" | ||
| :opacity 0 | ||
| :value value}] | ||
| [token-name-text theme text]]) | ||
| [{:keys [theme text]}] | ||
| [token-name-text theme text]) |
There was a problem hiding this comment.
This component seems just like a wrapper for token-name-text do we need it? maybe we can use token-name-text directly?
| @@ -29,3 +30,13 @@ | |||
| if `num` exceeds a given bound, then returns the bound exceeded." | |||
| [number lower-bound upper-bound] | |||
| (max lower-bound (min number upper-bound))) | |||
|
|
|||
| (defn remove-trailing-zeroes | |||
There was a problem hiding this comment.
Now that we have a focus on this function, maybe we can simplify the logic a bit?
(let [[integral fractional] (string/split (str num) #"\.")
fractional-fixed (some-> fractional
(string/replace #"0+$" ""))]
(if fractional
(str integral "." fractional-fixed)
integral))I think in this implementation is clearer what we do to the number 👀 wdyt?
briansztamfater
force-pushed
the
fix/routes-fetch-fiat
branch
from
1d77b60
to
d7a73bd
Compare
87% of end-end tests have passedFailed tests (5)Click to expandClass TestWalletMultipleDevice:
Class TestCommunityMultipleDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestGroupChatMultipleDeviceMergedNewUI:
Expected to fail tests (2)Click to expandClass TestCommunityOneDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
Passed tests (45)Click to expandClass TestGroupChatMultipleDeviceMergedNewUI:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestCommunityOneDeviceMerged:
Class TestWalletOneDevice:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestCommunityMultipleDeviceMerged:
Class TestActivityMultipleDevicePRTwo:
Class TestActivityMultipleDevicePR:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestDeepLinksOneDevice:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
|
e2e failures are not related to the PR |
briansztamfater
force-pushed
the
fix/routes-fetch-fiat
branch
2 times, most recently
from
08f7100
to
b994b96
Compare
| valid-input? (not (or (string/blank? | ||
| (controlled-input/input-value | ||
| input-state)) | ||
| (<= (controlled-input/numeric-value | ||
| input-state) | ||
| 0) | ||
| (> (controlled-input/numeric-value | ||
| input-state) | ||
| current-limit))) | ||
| input-num-value (controlled-input/numeric-value input-state) | ||
| input-amount (controlled-input/input-value input-state) | ||
| confirm-disabled? (or (nil? route) | ||
| (empty? route) | ||
| (string/blank? (controlled-input/input-value | ||
| input-state)) | ||
| (<= input-num-value 0) | ||
| (> input-num-value current-limit)) | ||
| amount (if @crypto-currency? | ||
| input-amount | ||
| (number/remove-trailing-zeroes | ||
| (.toFixed (/ input-amount conversion-rate) | ||
| crypto-decimals))) | ||
| send-amount (rf/sub [:wallet/wallet-send-amount]) | ||
| amount-text (str (number/remove-trailing-zeroes | ||
| (.toFixed (js/parseFloat send-amount) | ||
| (min token-decimals 6))) | ||
| " " | ||
| token-symbol) | ||
| first-route (first route) | ||
| native-currency-symbol (when-not confirm-disabled? | ||
| (get-in first-route | ||
| [:from :native-currency-symbol])) | ||
| native-token (when native-currency-symbol | ||
| (rf/sub [:wallet/token-by-symbol | ||
| native-currency-symbol])) | ||
| fee-in-native-token (when-not confirm-disabled? | ||
| (send-utils/calculate-full-route-gas-fee route)) | ||
| fee-in-crypto-formatted (when fee-in-native-token | ||
| (utils/get-standard-crypto-format | ||
| native-token | ||
| fee-in-native-token)) | ||
| fee-in-fiat (when-not confirm-disabled? | ||
| (utils/calculate-token-fiat-value | ||
| {:currency fiat-currency | ||
| :balance fee-in-native-token | ||
| :token native-token})) | ||
| fee-formatted (when fee-in-fiat | ||
| (utils/get-standard-fiat-format | ||
| fee-in-crypto-formatted | ||
| currency-symbol | ||
| fee-in-fiat)) | ||
| show-select-asset-sheet #(rf/dispatch | ||
| [:show-bottom-sheet | ||
| {:content (fn [] | ||
| [select-asset-bottom-sheet | ||
| clear-input!])}]) | ||
| sender-network-values (rf/sub | ||
| [:wallet/wallet-send-sender-network-values]) | ||
| receiver-network-values (rf/sub | ||
| [:wallet/wallet-send-receiver-network-values]) | ||
| token-not-supported-in-receiver-networks? (every? #(= (:type %) :not-available) | ||
| (filter #(not= (:type %) :add) | ||
| receiver-network-values)) | ||
| suggested-routes (rf/sub [:wallet/wallet-send-suggested-routes]) | ||
| routes (when suggested-routes | ||
| (or (:best suggested-routes) [])) | ||
| no-routes-found? (and | ||
| (every-network-value-is-zero? | ||
| sender-network-values) | ||
| (not (nil? routes)) | ||
| (not loading-routes?) | ||
| (not token-not-supported-in-receiver-networks?)) | ||
| receiver-networks (rf/sub [:wallet/wallet-send-receiver-networks]) | ||
| receiver-preferred-networks (rf/sub | ||
| [:wallet/wallet-send-receiver-preferred-networks]) | ||
| receiver-preferred-networks-set (set receiver-preferred-networks) | ||
| sending-to-unpreferred-networks? (not (every? (fn [receiver-selected-network] | ||
| (contains? | ||
| receiver-preferred-networks-set | ||
| receiver-selected-network)) | ||
| receiver-networks))] | ||
| suggested-routes (rf/sub [:wallet/wallet-send-suggested-routes]) | ||
| routes (when suggested-routes | ||
| (or (:best suggested-routes) [])) | ||
| no-routes-found? (and | ||
| (every-network-value-is-zero? | ||
| sender-network-values) | ||
| (not (nil? routes)) | ||
| (not loading-routes?) | ||
| (not token-not-supported-in-receiver-networks?)) | ||
| receiver-networks (rf/sub [:wallet/wallet-send-receiver-networks]) | ||
| receiver-preferred-networks (rf/sub | ||
| [:wallet/wallet-send-receiver-preferred-networks]) | ||
| receiver-preferred-networks-set (set receiver-preferred-networks) | ||
| sending-to-unpreferred-networks? (not (every? (fn [receiver-selected-network] | ||
| (contains? | ||
| receiver-preferred-networks-set | ||
| receiver-selected-network)) | ||
| receiver-networks)) | ||
| input-error (controlled-input/input-error input-state)] |
There was a problem hiding this comment.
@OmarBasem
Sorry, I don't get it, do you mean that we have events, subs, utils and defs defined in the same file?
@ulisesmac I mean this long list of definitions which is a mix of event, subs, utils, and values, could introduce bugs in the future and could be harder to debug. We need to have more separation of concerns here
briansztamfater
force-pushed
the
fix/routes-fetch-fiat
branch
from
b994b96
to
0a70a8f
Compare
pavloburykh
force-pushed
the
fix/routes-fetch-fiat
branch
from
0a70a8f
to
882f7b7
Compare
briansztamfater
force-pushed
the
fix/routes-fetch-fiat
branch
from
882f7b7
to
cbbb9e9
Compare
|
Hey @briansztamfater! Thanks for the PR. I see that you have pushed recently some changes. Is that just rebasing or additional changes? |
Hey @pavloburykh, just rebasing, PR can be tested |
|
@briansztamfater thanks for the fixes. PR is ready for merge. |
Signed-off-by: Brian Sztamfater <brian@status.im>
briansztamfater
force-pushed
the
fix/routes-fetch-fiat
branch
from
cbbb9e9
to
60151b5
Compare
fixes #18561
fixes #19999
Summary
This PR fixes a bug that makes no routes being fetched when the users switch the input mode to display values in fiat and a valid amount in fiat is entered. Also fixed some minor but noticeable bugs that appeared after fixing the main bug:
Demo
fiatconversiondemo.mp4
Platforms
Areas that maybe impacted
Functional
Steps to test
status: ready