Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix: ensure clean-up state after send transaction confirmation #21282

Merged
merged 6 commits into from
Sep 25, 2024
Merged

fix: ensure clean-up state after send transaction confirmation #21282

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
0278bd4
fix_: ensure we stop receiving suggest route signals after we've conf…
seanstrom Sep 18, 2024
c534ae7
fix_: ensure we don't throw if bignumber fails to compare, instead re…
seanstrom Sep 18, 2024
581c511
chore: make money equal-to function a little safer
seanstrom Sep 19, 2024
d91568c
fix: avoid saving special ui state into reframe and re-calculate ui s…
seanstrom Sep 20, 2024
e2c98aa
chore: add extra checks for bignumber instances
seanstrom Sep 20, 2024
fb8e202
chore: refactor ->bignumbers to use not use varargs
seanstrom Sep 24, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 5 additions & 9 deletions src/status_im/contexts/wallet/send/events.cljs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,23 +43,19 @@
:token-decimals token-decimals
:native-token? native-token?
:receiver? false})
from-network-values-for-ui (send-utils/network-values-for-ui
from-network-amounts-by-chain)
to-network-amounts-by-chain (send-utils/network-amounts-by-chain
{:route chosen-route
:token-decimals token-decimals
:native-token? native-token?
:receiver? true})
to-network-values-for-ui (send-utils/network-values-for-ui
to-network-amounts-by-chain)
sender-possible-chain-ids (map :chain-id sender-network-values)
sender-network-values (if routes-available?
(send-utils/network-amounts
{:network-values
(if (= tx-type :tx/bridge)
from-network-values-for-ui
from-network-amounts-by-chain
(send-utils/add-zero-values-to-network-values
from-network-values-for-ui
from-network-amounts-by-chain
sender-possible-chain-ids))
:disabled-chain-ids disabled-from-chain-ids
:receiver-networks receiver-networks
Expand All @@ -71,7 +67,7 @@
sender-network-values))
receiver-network-values (if routes-available?
(send-utils/network-amounts
{:network-values to-network-values-for-ui
{:network-values to-network-amounts-by-chain
:disabled-chain-ids disabled-from-chain-ids
:receiver-networks receiver-networks
:token-networks-ids token-networks-ids
Expand All @@ -91,8 +87,8 @@
assoc
:suggested-routes suggested-routes-data
:route chosen-route
:from-values-by-chain from-network-values-for-ui
:to-values-by-chain to-network-values-for-ui
:from-values-by-chain from-network-amounts-by-chain
:to-values-by-chain to-network-amounts-by-chain
Comment on lines -94 to +91
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's where we were storing the UI strings inside the re-frame state. Now we'll store the original data and do the conversions inside the UI components.

:sender-network-values sender-network-values
:receiver-network-values receiver-network-values
:network-links network-links
Expand Down
2 changes: 2 additions & 0 deletions src/status_im/contexts/wallet/send/send_amount/view.cljs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,13 @@
(:require
[quo.theme]
[status-im.contexts.wallet.send.input-amount.view :as input-amount]
[status-im.setup.hot-reload :as hot-reload]
[utils.i18n :as i18n]
[utils.re-frame :as rf]))

(defn view
[]
(hot-reload/use-safe-unmount #(rf/dispatch [:wallet/stop-get-suggested-routes]))
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here we're configuring the wallet send screen to dispatch the :wallet/stop-get-suggested-routes event when unmounting the component. Without this, it seems that we'll continue to receive suggested route signals, which can cause some glitches because we're re-processing stale wallet send UI state.

[input-amount/view
{:current-screen-id :screen/wallet.send-input-amount
:button-one-label (i18n/label :t/review-send)
Expand Down
3 changes: 2 additions & 1 deletion src/status_im/contexts/wallet/send/transaction_confirmation/view.cljs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
[status-im.common.standard-authentication.core :as standard-auth]
[status-im.contexts.wallet.common.utils :as utils]
[status-im.contexts.wallet.send.transaction-confirmation.style :as style]
[status-im.contexts.wallet.send.utils :as send-utils]
[utils.i18n :as i18n]
[utils.re-frame :as rf]
[utils.security.core :as security]))
Expand Down Expand Up @@ -142,7 +143,7 @@
[quo/summary-info
{:type summary-info-type
:networks? true
:values network-values
:values (send-utils/network-values-for-ui network-values)
Comment on lines -145 to +146
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's one place we were using the UI strings inside network-values, because summary-info will use the string "<0.01" to determine if some UI should be shown. Not sure what the original intent of that code would be, but perhaps that code could be changed too.

:account-props (cond-> account-props
(and account-to? (not bridge-tx?))
(assoc
Expand Down
3 changes: 2 additions & 1 deletion src/status_im/contexts/wallet/swap/swap_confirmation/view.cljs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
[status-im.common.floating-button-page.view :as floating-button-page]
[status-im.common.standard-authentication.core :as standard-auth]
[status-im.constants :as constants]
[status-im.contexts.wallet.send.utils :as send-utils]
[status-im.contexts.wallet.swap.swap-confirmation.style :as style]
[utils.address :as address-utils]
[utils.i18n :as i18n]
Expand Down Expand Up @@ -76,7 +77,7 @@
[quo/summary-info
{:type :token
:networks? true
:values network-values
:values (send-utils/network-values-for-ui network-values)
:token-props {:token token-symbol
:label (str amount " " token-symbol)
:address (address-utils/get-shortened-compressed-key token-address)
Expand Down
40 changes: 26 additions & 14 deletions src/utils/money.cljs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,25 +42,36 @@
[x]
(instance? BigNumber x))

(defn ->bignumber
[n]
(if (bignumber? n) n (bignumber n)))

(defn ->bignumbers
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function is only used for 1 pair of inputs at a time. Also, for a low-level construct to build instances, I think we should skip the overhead of varargs and processing nums twice with map and every?.

Because ->bignumber (singular) conveniently returns a BigNumber instance or nil, I think we can skip completely the extra ->bignumbers (plural) function because we can create a vector on the fly, like this:

;; PR implementation
(defn less-than
  [n1 n2]
  (when-let [[^js bn1 ^js bn2] (->bignumbers n1 n2)]
    (.lessThan ^js bn1 bn2)))

;; Suggestion
(defn less-than
  [n1 n2]
  (when-let [[^js bn1 ^js bn2] [(->bignumber n1) (->bignumber n2)]]
    (.lessThan bn1 bn2)))

The suggestion is still as readable, if not more due to the elimination of the extra indirection.

In terms of benchmarks, this simple change can reduce the time by almost 50% in my tests of less-than. Our conversion of numbers is relatively slow already because we always normalize numbers with money/normalize, which is very odd to be a default. I actually found the original commit that introduced normalization by default and it really makes no sense anymore 06650aecbb because this was added to conveniently allow to add commas in gas inputs, but the vast majority of cases shouldn't need the slow regex processing we do to instantiate bignums.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to be clear @seanstrom, I wouldn't touch the normalization part in this PR. I just wanted to illustrate that creating BigNumber instances is already slow, but this is a part of the code where we should make things fast.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ilmotta Okay sounds good, I'll try refactoring the code to not use varargs and avoid doing multiple passes on list of bignum inputs.

One thing to note is that this code:

(defn less-than
  [n1 n2]
  (when-let [[^js bn1 ^js bn2] [(->bignumber n1) (->bignumber n2)]]
    (.lessThan bn1 bn2)))

Might not work the same with when-let because we're always returning a vector, and we want to coerce/check if each number is a big number (I think).
Initially ->bignumbers was a function that used two when-let expressions check if both numbers were bignumbers, but I ended up refactoring to varargs stuff. Though maybe that function is doing too much, and refactoring that function to use transducers might not be worth it either. So I'll reverting back to the double when-let like this:

(defn ->bignumbers
  [n1 n2]
  (when-let [bn1 (->bignumber n1)]
    (when-let [bn2 (->bignumber n2)]
      (when (and (bignumber? bn1) (bignumber? bn2))
        [bn1 bn2]))))

This way we still return the vector of numbers when both numbers are valid big numbers. Thoughts?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On a side note, maybe we can update that normalising code to check if the input is a string? Because if the input is not a string I don't think it will have commas present when coerced to a string. That way we may be able to avoid the extra normalising logic for numeric inputs. 💭

Copy link
Contributor

@ilmotta ilmotta Sep 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch @seanstrom, definitely my snippet wouldn't work and I didn't test *properly the snippet I shared. Your new snippet seems correct 👍🏼 What I like about using an optimized arity-2 implementation like what we are doing now is that we can always add arity-x in a backwards compatible way, similar to how cljs.core/+ works.

On a side note, maybe we can update that normalising code to check if the input is a string?

I had the impression (no evidence now) that most values we pass to ->bignumber are strings, so maybe this wouldn't be too effective?

From that original old commit I shared, I think the solution wasn't the best because cleaning up user input is a distant concern for a utility such as money instantiating bignums.

In the few cases where a bignum has to be created from user input, then the UI code can do the clean-up/normalization explicitly. This would be the best I think, because in all other cases bignums should be instantiated from proper bignum strings sent from the backend, and those shouldn't have formatting chars like ,.

But probably safer if my suggestion or yours is applied in a separate PR?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup yup I think it's better to tweak the normalising logic in a separate pr 👍
And yeah the two-arity refactor is very nice, thank you 🙌

[n1 n2]
(when-let [bn1 (->bignumber n1)]
(when-let [bn2 (->bignumber n2)]
(when (and (bignumber? bn1) (bignumber? bn2))
[bn1 bn2]))))

(defn greater-than-or-equals
[^js bn1 ^js bn2]
(when (bignumber? bn1)
[^js n1 ^js n2]
(when-let [[^js bn1 ^js bn2] (->bignumbers n1 n2)]
(.greaterThanOrEqualTo bn1 bn2)))

(defn greater-than
[bn1 bn2]
(when (bignumber? bn1)
[n1 n2]
(when-let [[^js bn1 ^js bn2] (->bignumbers n1 n2)]
(.greaterThan ^js bn1 bn2)))

(defn less-than
[bn1 bn2]
(when (bignumber? bn1)
[n1 n2]
(when-let [[^js bn1 ^js bn2] (->bignumbers n1 n2)]
(.lessThan ^js bn1 bn2)))

(defn equal-to
[n1 n2]
(when-let [^js bn1 (bignumber n1)]
(.eq ^js bn1 n2)))
(when-let [[^js bn1 ^js bn2] (->bignumbers n1 n2)]
(.eq ^js bn1 bn2)))

(extend-type BigNumber
IEquiv
Expand All @@ -79,8 +90,9 @@
:else 0)))

(defn sub
[bn1 bn2]
(.sub ^js bn1 bn2))
[n1 n2]
(when-let [[^js bn1 ^js bn2] (->bignumbers n1 n2)]
(.sub ^js bn1 bn2)))

(defn valid?
[^js bn]
Expand Down Expand Up @@ -125,7 +137,7 @@

(defn to-number
[^js bn]
(when bn
(when (bignumber? bn)
(.toNumber bn)))

(defn to-string
Expand Down Expand Up @@ -158,7 +170,7 @@

(defn ether->wei
[^js bn]
(when bn
(when (bignumber? bn)
(.times bn ^js (bignumber 1e18))))

(defn token->unit
Expand Down Expand Up @@ -228,7 +240,7 @@
(defn sufficient-funds?
[^js amount ^js balance]
(when (and amount balance)
(.greaterThanOrEqualTo balance amount)))
(greater-than-or-equals balance amount)))

(defn fiat-amount-value
[amount-str from to prices]
Expand Down Expand Up @@ -275,7 +287,7 @@

(defn absolute-value
[bn]
(when bn
(when (bignumber? bn)
(.absoluteValue ^js bn)))

(defn format-amount
Expand Down