Wallet: token input conversion #18539
Conversation
OmarBasem
force-pushed
the
wallet/fix-conversion
branch
from
ed5fc83
to
a72bbc8
Compare
OmarBasem
requested review from
shivekkhurana,
briansztamfater,
smohamedjavid,
J-Son89,
mmilad75 and
ulisesmac
Jenkins BuildsClick to see older builds (32)
|
92% of end-end tests have passedFailed tests (1)Click to expandClass TestActivityCenterContactRequestMultipleDevicePR:
Expected to fail tests (3)Click to expandClass TestCommunityMultipleDeviceMergedTwo:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityOneDeviceMerged:
Passed tests (44)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestCommunityMultipleDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestDeepLinksOneDevice:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestCommunityOneDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestActivityMultipleDevicePRTwo:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestActivityMultipleDevicePR:
|
| @@ -14,11 +14,13 @@ | |||
| [reagent.core :as reagent])) | |||
|
|
|||
| (defn calc-value | |||
| [crypto? currency token value conversion] | |||
| [crypto? currency token value conversion crypto-decimals] | |||
There was a problem hiding this comment.
We should avoid functions with 3+ positional args, better to use a map imo
| (let [num-value (if (string? value) (parse-double (or value "0")) value)] | ||
| (if crypto? | ||
| (str (get common/currency-label currency) (.toFixed (* num-value conversion) 2)) | ||
| (str (.toFixed (/ num-value conversion) 2) " " (string/upper-case (or (clj->js token) "")))))) | ||
| (str (.toFixed (/ num-value conversion) (or crypto-decimals 2)) |
There was a problem hiding this comment.
Imo it would be nice to separate the two functions of the if,
e. calc-crypto and calc-fiat
So it's
(if crypto?
(calc-crypto ...)
(calc-fiat ...))
And just reads nicely 👌
|
@OmarBasem thanks for the PR. Could you please provide some details on this fix?
Can you show with screenshot examples how it was before the fix and how it is now? So far it is not clear for me what has been changed in terms of this fix. Thank you! |
|
Hi @pavloburykh, Both fixes are for the same component in the video.
If you try to send a token you would notice the conversion rate when you switch fiat/crypto on that screen is currently incorrect.
Currently if you choose to send ETH for example you would see on that screen that ETH is shown using so many decimal places. That current behaviour incorrect. If something is still unclear please let me know |
Thanks for the answer @OmarBasem! Just curious why would we want to limit decimals amount on send token screens? In this case - it will not show the accurate amount of tokens. Where do we get the requirements on how many decimals we should show? For example, in this PR we show 5 decimals for ETH and 2 decimals for DAI though both tokens may have 18 decimals. Sorry, maybe I have missed some discussion on this topic, would appreciate if you share the links if such exist. I need this info to be sure in expected result otherwise my testing can not be valid. The Designs are showing 2 decimals for ETH which is completely weird. |
|
@OmarBasem meanwhile I have faced the issue and I am not sure if it is PR related. I cannot reproduce it on nightly. ISSUE 1 Cannot read property 'eq' of null error when selecting token for sending (Android)So far reproducing only on Android (Samsung Galaxy A52, Android 12) Steps:
Actual result: Cannot read property 'eq' of null error appears. It does not happen every time. telegram-cloud-document-2-5282794599960559219.mp4Expected result: no error |
@OmarBasem UPDATE: I have found this info status-im/status-desktop#8640 Will consider it to be a source of truth. |
@pavloburykh Yeah that's the information. Btw, this information comes from status-go so you don't have to worry about math details. Regarding issue 1, is this issue reproducible on develop cause we are facing a similar issue on develop in debug mode (a crash after selecting token which doesn't happen everytime) and there is currently another PR that should fix it #18532 |
|
@OmarBasem thank you for clarification. PR is ready to be merged. |
OmarBasem
force-pushed
the
wallet/fix-conversion
branch
from
ee25873
to
dc0db61
Compare
OmarBasem
force-pushed
the
wallet/fix-conversion
branch
3 times, most recently
from
79c789f
to
6d8e0db
Compare
OmarBasem
force-pushed
the
wallet/fix-conversion
branch
from
6d8e0db
to
2c60662
Compare
Wallet: token input conversion
| (defn get-crypto-decimals-count | ||
| [{:keys [market-values-per-currency]}] | ||
| (let [price (get-in market-values-per-currency [:usd :price]) | ||
| one-cent-value (if (pos? price) (/ 0.01 price) 0)] |
There was a problem hiding this comment.
Should price be a big number?
Cc @ulisesmac
There was a problem hiding this comment.
Yeah, I think price should be.
JS numbers' implementation sometimes have an unexpected behaviour. since we are dealing with money, better to be safe 👍
There was a problem hiding this comment.
Yeah, I think price should be.
JS numbers' implementation sometimes have an unexpected behaviour. since we are dealing with money, better to be safe 👍
fixes: #18499, #18538
This PR fixes the coversion rate from crypto to fiat used in the token input component.
Also fixes the crypto decimal places used in the token input component.
Screen_Recording_20240117_161258_Status.mp4