[#17896] Raw balances using big number #17920
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ulisesmac
requested review from
vkjr,
briansztamfater,
smohamedjavid,
J-Son89,
OmarBasem and
mmilad75
ulisesmac
commented
Nov 15, 2023
Comment on lines
26
to
32
| (defn- calculate-raw-balance | ||
| [raw-balance decimals] | ||
| (if-let [n (utils.number/parse-int raw-balance nil)] | ||
| (/ n (Math/pow 10 (utils.number/parse-int decimals))) | ||
| 0)) |
There was a problem hiding this comment.
utils.money/token->unit is able to do the same and works with big numbers, so now it is being used
ulisesmac
commented
Nov 15, 2023
Comment on lines
-43
to
-52
| (* (total-token-value-in-all-chains token) | ||
| (-> token :market-values-per-currency :usd :price)))) |
There was a problem hiding this comment.
utils.money/crypto->fiat does the same but supports bignumbers, so now it's being used
ulisesmac
commented
Nov 15, 2023
| @@ -1,6 +1,7 @@ | |||
| (ns status-im2.contexts.wallet.common.utils | |||
There was a problem hiding this comment.
Besides using functions in utils.money, this file also had a refactor
ulisesmac
commented
Nov 15, 2023
Comment on lines
-105
to
+124
| (defn- fix-chain-id-keys | ||
| (defn- fix-balances-per-chain | ||
| [token] | ||
| (update token :balances-per-chain update-keys (comp utils.number/parse-int name))) | ||
| (-> token | ||
| (update :balances-per-chain update-vals #(update % :raw-balance money/bignumber)) | ||
| (update :balances-per-chain update-keys (comp utils.number/parse-int name)))) | ||
|
|
||
| (rf/reg-event-fx | ||
| :wallet/store-wallet-token | ||
| (fn [{:keys [db]} [raw-tokens-data]] | ||
| (let [tokens (-> raw-tokens-data | ||
| (update-keys name) | ||
| (update-vals #(cske/transform-keys csk/->kebab-case %)) | ||
| (update-vals #(mapv fix-chain-id-keys %))) | ||
| (update-vals #(mapv fix-balances-per-chain %))) |
There was a problem hiding this comment.
Here, when the token raw-balance is received it is being stored as bignumber instances
J-Son89
reviewed
Nov 15, 2023
ulisesmac
commented
Nov 15, 2023
Comment on lines
+89
to
+94
| (let [result (rf/sub [sub-name]) | ||
| balance-0x1 (money/bignumber 3250) | ||
| balance-0x2 (money/bignumber 2100)] | ||
|
|
||
| (is (= {"0x1" 3250 "0x2" 2100} | ||
| (rf/sub [sub-name]))))) | ||
| (is (money/equal-to balance-0x1 (get result "0x1"))) | ||
| (is (money/equal-to balance-0x2 (get result "0x2")))))) |
There was a problem hiding this comment.
This file is now testing against bignumber instances. the function utils.money/equal-to is needed to check equality between them.
Jenkins BuildsClick to see older builds (20)
|
ulisesmac
commented
Nov 15, 2023
Comment on lines
-207
to
+215
| (defn add | ||
| (defn- add* | ||
| [bn1 n2] | ||
| (.add ^js bn1 n2)) | ||
|
|
||
| (def add | ||
| "Add with defaults, this version is able to receive `nil` and takes them as 0." | ||
| (fnil add* (bignumber 0) (bignumber 0))) | ||
|
|
There was a problem hiding this comment.
Added a more flexible version of add.
I'd really like to get your feedback about this change.
Comparison between this version and the previous one:
(reduce money/add [])
;; Before:
;; Exception about null has no .add method
;; Now:
;; => (bignumber 0)
(reduce money/add [(money/bignumber 10)])
;; Before:
;; => (bignumber 10)
;; Now:
;; => (bignumber 10)
(reduce money/add [(money/bignumber 10) (money/bignumber 20) (money/bignumber 30)])
;; Before:
;; => (bignumber 60)
;; Now:
;; => (bignumber 60)
(money/add (money/bignumber 10))
;; Before:
;; Exception about no method plus() found in null
;; Now:
;; => (bignumber 10)
(money/add (money/bignumber 10) nil)
;; Before:
;; Exception about no method plus() found in null
;; Now:
;; => (bignumber 10)
(money/add nil (money/bignumber 10))
;; Before:
;; Exception about null has no .add method
;; Now:
;; => (bignumber 10)
(money/add nil nil)
;; Before:
;; Exception about null has no .add method
;; Now:
;; => (bignumber 0)
There was a problem hiding this comment.
This seems like a perfect solution to me for dealing with nil values as we should not allow the app to crash on nil values.
Thanks for comparsion @ulisesmac!
J-Son89
approved these changes
Nov 17, 2023
smohamedjavid
approved these changes
Nov 24, 2023
Comment on lines
+30
to
+37
| (map (comp :raw-balance val)) | ||
| (reduce money/add))) |
| total-units-in-all-chains (-> balances-per-chain | ||
| (total-raw-balance-in-all-chains) | ||
| (money/token->unit decimals))] | ||
| (money/crypto->fiat total-units-in-all-chains usd-price))) |
There was a problem hiding this comment.
Love the naming of the methods. Clean and neat. 🚀
Comment on lines
-207
to
+215
| (defn add | ||
| (defn- add* | ||
| [bn1 n2] | ||
| (.add ^js bn1 n2)) | ||
|
|
||
| (def add | ||
| "Add with defaults, this version is able to receive `nil` and takes them as 0." | ||
| (fnil add* (bignumber 0) (bignumber 0))) | ||
|
|
There was a problem hiding this comment.
This seems like a perfect solution to me for dealing with nil values as we should not allow the app to crash on nil values.
Thanks for comparsion @ulisesmac!
|
@ulisesmac can you please resolve conflicts and update it? |
ulisesmac
force-pushed
the
17896-raw-balances-big-number
branch
from
930bc66
to
4b5cb07
Compare
@churik No problem! done! |
|
@ulisesmac |
ulisesmac
force-pushed
the
17896-raw-balances-big-number
branch
from
4b5cb07
to
2fb8a5c
Compare
ulisesmac
commented
Dec 4, 2023
Comment on lines
-74
to
+78
| (let [tokens (map (fn [token] | ||
| (assoc token | ||
| :networks (utils/network-list token networks) | ||
| :total-balance (utils/total-token-value-in-all-chains token) | ||
| :total-balance-fiat (utils/calculate-balance token))) | ||
| (:tokens account)) | ||
|
|
||
| sorted-tokens | ||
| (sort-by :name compare tokens) | ||
| filtered-tokens | ||
| (filter #(or (string/starts-with? (string/lower-case (:name %)) | ||
| (string/lower-case query)) | ||
| (string/starts-with? (string/lower-case (:symbol %)) | ||
| (string/lower-case query))) | ||
| sorted-tokens)] | ||
| (let [tokens (map (fn [token] | ||
| (assoc token | ||
| :networks (utils/network-list token networks) | ||
| :total-balance (utils/total-token-units-in-all-chains token) | ||
| :total-balance-fiat 0)) |
There was a problem hiding this comment.
@churik
For the wallet flow:
- Select an account
- Select Send button
- Write an address (0x123..)
- Select an address
the fiat balance is being returned as zero as part of this PR.
The reason is that the value has a bug in develop, and its calculation was perfomed using code that I erased in this PR, so instead of fixing that bug here, I created an issue:
to properly address it. And set it as zero for this PR.
cc @briansztamfater
|
@ulisesmac as you mentioned that balance is 0 now (I checked it) I do not think that it is testable right now, is it true? I'm blocking PR for testing until the issue is resolved, let me know when it is ready, thanks! |
|
@ulisesmac Nightly: FILE.2023-12-06.17.49.05.mp4PR: IMG_1884.MP4 |
62% of end-end tests have passedNot executed tests (1)Failed tests (14)Click to expandClass TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityMultipleDeviceMerged:
Class TestCommunityOneDeviceMerged:
Expected to fail tests (4)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityOneDeviceMerged:
Passed tests (30)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestActivityMultipleDevicePRTwo:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestDeepLinksOneDevice:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestActivityMultipleDevicePR:
Class TestCommunityOneDeviceMerged:
|
Thanks @churik Will check it and fix it 👍 |
ulisesmac
force-pushed
the
17896-raw-balances-big-number
branch
from
2fb8a5c
to
866f5d1
Compare
ulisesmac
force-pushed
the
17896-raw-balances-big-number
branch
from
556036f
to
8b0d40b
Compare
|
@churik Sorry for the delay. We had a problem showing the balances, but now it's fixed! About what I said related to balances:
To test it, we have two options:
A random address I found in etherscan: is less than the total balance in this branch: since, as I said, the tokens being more than 1000 are not calculated correctly. This is just a small example to show the difference in calculations. |
|
Another account to test: https://etherscan.io/address/0x7bdf634f898ad124a9adb7a1aff95a95aa864cbb Just keeping here for the history, that only |
78% of end-end tests have passedFailed tests (3)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestCommunityOneDeviceMerged:
Expected to fail tests (8)Click to expandClass TestCommunityMultipleDeviceMergedTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityMultipleDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestCommunityOneDeviceMerged:
Passed tests (38)Click to expandClass TestCommunityMultipleDeviceMergedTwo:
Class TestDeepLinksOneDevice:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityMultipleDeviceMerged:
Class TestActivityMultipleDevicePR:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestActivityMultipleDevicePRTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestCommunityOneDeviceMerged:
|
yevh-berdnyk
pushed a commit
that referenced
this pull request
Dec 8, 2023
* Add `utils.money/add` version able to work with `nil`s * Store token raw-balance as big-number * Improve account balance calculations using `utils.money` existing functions * Update `:wallet/tokens-filtered` sub * Make `prettify-balance` able to work with bignumbers
@OmarBasem Since you are going to modify that code again, this might interest you. I think it's due to two reasons:
In this PR I'm using bignumbers for the total balance calculation -it may give you a hint-, also testing with more than 1000 units per token can help. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
fixes #17896
Summary
The previous implementation was using js/parseInt to process the raw-balances, this PR stores them as big-number instances in re-frame.
This had some problems, if we had 100 or more units of a token, the calculations resulted in a
0, now using bignumbers that issue has been fixed.Changes
utils.money/addable to receivenilvaluesutils.moneyfunctions.Testing notes
The app should work as expected. Previously, if we had 100 or more token units, the result listed in the account cards in the wallet home page was
$0.00. Now it shows the proper value.Platforms
Areas that maybe impacted
Wallet home page in the account cards.
Steps to test
status: ready