From 1c83916fc35b7e9281d9ad560221e7375f3ded70 Mon Sep 17 00:00:00 2001 From: Icaro Motta Date: Mon, 1 Jul 2024 18:38:10 -0300 Subject: [PATCH] fix: Make BigNumber comparator safe against unsupported data types (#20614) BigNumber instances are not comparable to other data types, except numbers and strings (supported directly by the library). This commit makes = safe on bignumbers (no exceptions should be thrown), even when comparing a bignumber with a nil value. There's another important benefit from having a custom comparator: performance, because re-frame will be able to correctly compare data with bignumbers and thus reuse cached subscriptions and also Reagent will be able to skip processing when props are equal. The IComparable interface was not changed. Similarly to core CLJS data types, sorting heterogeneous collections throws (ignore nils). ```clojure ;; This throws with message: cannot compare 10 to foo (sort ["foo" 10]) ``` We could make sorting work with heterogeneous collections, but then we would need to extend many core data types to play nice with BigNumber and that sounds more like a hack, and it's also not a problem we have nowadays. --- src/utils/money.cljs | 6 ++++- src/utils/money_test.cljs | 55 ++++++++++++++++++++++++++------------- 2 files changed, 42 insertions(+), 19 deletions(-) diff --git a/src/utils/money.cljs b/src/utils/money.cljs index cf41537e60..a160ec065c 100644 --- a/src/utils/money.cljs +++ b/src/utils/money.cljs @@ -61,7 +61,11 @@ (extend-type BigNumber IEquiv (-equiv [this other] - (equal-to this other)) + (if (or (number? other) + (string? other) + (instance? BigNumber other)) + (equal-to this other) + false)) IComparable (-compare [this other] diff --git a/src/utils/money_test.cljs b/src/utils/money_test.cljs index 97e2cf68d3..93ab5eb9f2 100644 --- a/src/utils/money_test.cljs +++ b/src/utils/money_test.cljs @@ -4,26 +4,45 @@ [utils.money :as money])) (deftest comparable-test - (is (= [(money/bignumber -4) - (money/bignumber 0) - (money/bignumber 1) - (money/bignumber 1.1) - (money/bignumber 2.1)] - (sort [(money/bignumber 0) - (money/bignumber 2.1) - (money/bignumber -4) - (money/bignumber 1.1) - (money/bignumber 1)])))) + (testing "sorts bignumbers (and nils)" + (is (= [nil + nil + (money/bignumber -4) + (money/bignumber 0) + (money/bignumber 1) + (money/bignumber 1.1) + (money/bignumber 2.1)] + (sort [(money/bignumber 0) + nil + (money/bignumber 2.1) + (money/bignumber -4) + (money/bignumber 1.1) + (money/bignumber nil) + (money/bignumber 1)])))) + + (testing "throws when comparing non-bignumbers with bignumbers" + (is (thrown? js/Error (sort [(money/bignumber 42) ""]))))) (deftest equivalence-test - (is (= (money/bignumber 0) - (money/bignumber 0))) - (is (= (money/bignumber -1) - (money/bignumber -1))) - (is (not (= (money/bignumber 10) - (money/bignumber -10)))) - (is (match? {:a {:b {:c (money/bignumber 42)}}} - {:a {:b {:c (money/bignumber 42)}}}))) + (testing "equivalence with numbers and strings" + (is (= (money/bignumber 42) 42)) + (is (= (money/bignumber 42) 42.0)) + (is (= (money/bignumber 42) "42")) + (is (= (money/bignumber 42) "42.0")) + (is (not= (money/bignumber 42) :42)) + (is (not= (money/bignumber 42) {:x 42}))) + + (testing "bignumbers are never equivalent to nil" + ;; The native `.eq` function throws with nil values, but we gracefully handle this. + (is (false? (= (money/bignumber 0) nil))) + (is (false? (= (money/bignumber 10) (money/bignumber nil))))) + + (testing "equivalence of actual bignumbers" + (is (= (money/bignumber 0) (money/bignumber 0))) + (is (= (money/bignumber -1) (money/bignumber -1))) + (is (not= (money/bignumber 10) (money/bignumber -10))) + (is (match? {:a {:b {:c (money/bignumber 42)}}} + {:a {:b {:c (money/bignumber 42)}}})))) (deftest wei->ether-test (testing "Numeric input, 15 significant digits"