fix: ensure clean-up state after send transaction confirmation

[seanstrom]

partially fixes #21251

Summary

• This PR attempts to resolve some issues that have been occurring after sending a transaction.
  • BigNumber errors for "<0.01"
    • It seems we're storing this string "<0.01" for some wallet ui state, but that state can be accidentally used with BigNumber calculations, which can exceptions.
    • Here's where we inject this string into the ui state:

      status-mobile/src/status_im/contexts/wallet/send/utils.cljs

      Line 67 in 6966432

      (assoc acc k (if (money/equal-to v 0) "<0.01" v)))

    • And here's where we are referencing that value:

      status-mobile/src/quo/components/wallet/summary_info/view.cljs

      Lines 33 to 40 in 2f3d3fc

      	(let [{:keys [ethereum optimism arbitrum]} values
      	show-optimism? (and optimism
      	(or (pos? (:amount optimism))
      	(= (:amount optimism) "<0.01")))
      	show-arbitrum? (and arbitrum
      	(or (pos? (:amount arbitrum))
      	(= (:amount arbitrum) "<0.01")))]
      	[rn/view

  • Suggest Routes being received after the send transaction is already confirmed
    • It seems like we weren't configuring the wallet send screen to finish requesting the suggested routes data after the user has confirmed the transaction. And as we would continue to receive more suggested route results, the calculations would run with the existing wallet ui state, and that would eventually do some BigNumber comparisons with the string "<0.01" which would cause things to throw.
• Ideally, we would refactor the code in a way that avoids saving "<0.01" into the database, perhaps we can save an separate keyword like :less-than-something to indicate the same thing. Maybe this would avoid potential number comparison issues in the future.

Testing notes

Platforms

• Android
• iOS

Areas that maybe impacted

Functional

• wallet / transactions

Steps to test

• Open Status
• Login as user with funds
• Navigate to wallet home screen
• Open account with funds
• Press the "send" button to start a transaction
• Choose an account for receiving the transaction
• Input an amount of funds below 0.01, like 0.001
• Review and Confirm the transaction

Before and after screenshots comparison

Before Changes

Screen.Recording.2024-09-19.at.08.11.47.mov

After Changes

Screen.Recording.2024-09-19.at.08.10.34.mov

status: ready

[status-im-auto]

Jenkins Builds

Click to see older builds (4)

❔	Commit	#️⃣	Finished (UTC)	Duration	Platform	Result
✔️	df44fb6	#1	2024-09-18 15:16:22	~4 min	tests	📄log
✔️	df44fb6	#1	2024-09-18 15:20:05	~8 min	android-e2e	🤖apk 📲
✔️	df44fb6	#1	2024-09-18 15:20:30	~8 min	android	🤖apk 📲
✔️	df44fb6	#1	2024-09-18 15:25:21	~13 min	ios	📱ipa 📲

❔	Commit	#️⃣	Finished (UTC)	Duration	Platform	Result
✔️	d88e07a	#2	2024-09-19 07:12:58	~4 min	tests	📄log
✔️	d88e07a	#2	2024-09-19 07:16:16	~7 min	android-e2e	🤖apk 📲
✔️	d88e07a	#2	2024-09-19 07:17:40	~9 min	android	🤖apk 📲
✔️	d88e07a	#2	2024-09-19 07:19:05	~10 min	ios	📱ipa 📲
✔️	a7c0eab	#3	2024-09-19 16:03:13	~4 min	tests	📄log
✔️	a7c0eab	#3	2024-09-19 16:06:19	~7 min	android-e2e	🤖apk 📲
✔️	a7c0eab	#3	2024-09-19 16:06:49	~7 min	android	🤖apk 📲
✔️	a7c0eab	#3	2024-09-19 16:08:17	~9 min	ios	📱ipa 📲

[seanstrom]

Self Review 📑

[seanstrom]

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.

[seanstrom]

Here we're attempting to prevent exceptions from being thrown because of the BigNumber library failing to do a comparison. Perhaps we should not catch all exceptions here, but I think it's safer to assume that any failure would mean the comparison failed, which would mean the two things are not equal.

But maybe there's a better way to handle these exceptions, thoughts?

[vkjr]

@seanstrom, I think that we can avoid try/catch which is not common in our codebase by checking both arguments with (bignumber?) before calling .eq, wdyt?

[briansztamfater]

I am not sure what is the best approach, but I've recently updated greater-than less-than and greater-than-or-equals functions with a bignumber? check. The check was only needed on the first argument to avoid the error. We could use the same approach on equal-to for consistency.

[vkjr]

@briansztamfater, I also think that everything should be wrapped in checks, including second argument, because for equal-to second argument causes exception. I wrote an example down below.

[shivekkhurana]

Hi, this issue comment was made into a separate issue and assigned to @vkjr
#21251

@seanstrom your changes look good, if @vkjr has nothing against it we can go forward and use this PR.

[seanstrom]

@shivekkhurana thanks I've updated the PR with the link to the issue 👍

@vkjr based on some of the exceptions I saw when debugging, it could be still useful to refactor the code to avoid saving the "<0.01" into the re-frame database. This PR does not change any of that code, it sorta prevents the exceptions from happening as much, but it could be better to re-structure things. Let me know what you think 🙏

[vkjr]

@seanstrom, thanks for the PR, it will stop us from having exception. And I will investigate how that "<0.01" makes its way to that function, it shouldn't happen)
cc @shivekkhurana

[vkjr]

As @seanstrom correctly assumed, the fail happens because re-frame compares previous and new db value and in previous value stored BigNumber, whereas new values is a string that can't be converted to digit. As a result equal-to called with incorrect string as an argument and throws exception.

Throwing exception is expected and covered by tests by @ilmotta here, but I think that overall BigNumber is too fragile and we shouldn't allow it to throw exception. Because result of (equal-to some-bignumber "invalid-string") can easily be false and that will be logically correct.

So my suggestion is to wrap all functions in utils/money with extra checks to avoid any unexpected exceptions. Because as a developer I want to pass random staff into functions and be sure they will survive)
Something like this:

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

Wdyt, @seanstrom @ilmotta @shivekkhurana ?

[ilmotta]

Ideally, we would refactor the code in a way that avoids saving "<0.01" into the database, perhaps we can save an separate keyword like :less-than-something to indicate the same thing. Maybe this would avoid potential number comparison issues in the future.

Thanks a lot for helping with this bug @seanstrom. I was kind of expecting us to fix the root cause to avoid further problems. Even the names to-network-values-for-ui and from-network-values-for-ui are good hints that they shouldn't be stored in the app-db. Perhaps not too risky to fix the problem because the values are only used by one event which is dispatched from only one place after a signal arrives. The event also has one unit test, that should help a bit.

But of course, could be a follow-up PR, but would be nice if we never stored formatted data in the app-db, unless there's a good reason, like a performance concern where it's very expensive to recompute in subs and the computed data is read heavy much more than write heavy in the UI.

[ilmotta]

So my suggestion is to wrap all functions in utils/money with extra checks to avoid any unexpected exceptions. Because as a developer I want to pass random staff into functions and be sure they will survive) Something like this:

I fully agree @vkjr. Throwing an exception is quite bad for users because they bubble up in the whole stack. The bignumber library can throw for various reasons, probably not only because the inputs are not instances of bignumbers. It would be a good idea to take a look at the docs or implementation to see the possible causes of errors because we may need to wrap in a try/catch in some cases.

The other thing we should do ideally is to have a proper layer (data store ns) that always decodes data from status-go into the proper data types for Clojure. If a string arrives and it's supposed to be a bignum, it shouldn't flow throughout the system for too long as a raw string because eventually it can easily reach code that thinks it's a proper number. I've personally faced this problem multiple times in a previous Clojure project.

We extended BigNumber with IEquiv and IComparable, which should help us store them in the app-db and nicely manipulate them. More importantly, it means that if we store bignums in the app-db, re-frame will do the equalities correctly (something we were doing wrong some time ago and caused all subs caches to be invalidated whenever they had instances of bignums)

status-mobile/src/utils/money.cljs

Line 65 in 1190fc8

(extend-type BigNumber