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.

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?

@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?

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.

@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.

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.

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.

Here we're adding some additional checks for big number instances when using our big number utility functions. Let me what you think 🙏

I love this approach!

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.

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.

@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?

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. 💭

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?

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 🙌