From c047b588edb0cd04205b562b3d52437f4d1b331a Mon Sep 17 00:00:00 2001 From: yenda Date: Tue, 24 Sep 2019 18:45:33 +0200 Subject: [PATCH] throw errors on dangerous behaviours - if multiaccount settings are saved on top of an empty map or nil, this means something went wrong, the state of the app is unstable, and actually saving will result in loss of data. It should never happen, but if it does, throw and error and abort. - sometimes two fxs are merged when they shouldn't, this is caused by bugs and should never happen, but if it does, throw an error with arguments for both effects to help localize the error Signed-off-by: yenda --- src/status_im/multiaccounts/update/core.cljs | 31 ++++++++++++------- src/status_im/utils/fx.cljs | 2 +- test/cljs/status_im/test/models/bootnode.cljs | 2 +- .../test/multiaccounts/update/core.cljs | 8 +++-- 4 files changed, 28 insertions(+), 15 deletions(-) diff --git a/src/status_im/multiaccounts/update/core.cljs b/src/status_im/multiaccounts/update/core.cljs index 82ccd6189a..8a2bc7cf7a 100644 --- a/src/status_im/multiaccounts/update/core.cljs +++ b/src/status_im/multiaccounts/update/core.cljs @@ -5,7 +5,8 @@ [status-im.transport.message.contact :as message.contact] [status-im.transport.message.protocol :as protocol] [status-im.utils.fx :as fx] - [status-im.utils.types :as types])) + [status-im.utils.types :as types] + [taoensso.timbre :as log])) (fx/defn multiaccount-update-message [{:keys [db] :as cofx}] (let [multiaccount (:multiaccount db) @@ -53,11 +54,15 @@ :params ["multiaccount" (types/serialize new-multiaccount)] :on-success on-success}]} {:keys [name photo-path prefered-name]} new-multiaccount-fields] - (if (or name photo-path prefered-name) - (fx/merge cofx - fx - (send-multiaccount-update)) - fx))) + (if (empty? current-multiaccount) + ;; NOTE: this should never happen, but if it does this is a critical error + ;; and it is better to crash than risk having an unstable state + (throw (js/Error. "Please shake the phone to report this error and restart the app. multiaccount is currently empty, which means something went wrong when trying to update it with")) + (if (or name photo-path prefered-name) + (fx/merge cofx + fx + (send-multiaccount-update)) + fx)))) (fx/defn clean-seed-phrase "A helper function that removes seed phrase from storage." @@ -72,8 +77,12 @@ settings {:keys [on-success] :or {on-success #()}}] (let [new-multiaccount (assoc multiaccount :settings settings)] - {:db (assoc db :multiaccount new-multiaccount) - ::json-rpc/call - [{:method "settings_saveConfig" - :params ["multiaccount" (types/serialize new-multiaccount)] - :on-success on-success}]})) + (if (empty? multiaccount) + ;; NOTE: this should never happen, but if it does this is a critical error + ;; and it is better to crash than risk having an unstable state + (throw (js/Error. "Please shake the phone to report this error and restart the app. multiaccount is currently empty, which means something went wrong when trying to update settings")) + {:db (assoc db :multiaccount new-multiaccount) + ::json-rpc/call + [{:method "settings_saveConfig" + :params ["multiaccount" (types/serialize new-multiaccount)] + :on-success on-success}]}))) diff --git a/src/status_im/utils/fx.cljs b/src/status_im/utils/fx.cljs index f0792b96a5..c6473f0667 100644 --- a/src/status_im/utils/fx.cljs +++ b/src/status_im/utils/fx.cljs @@ -33,7 +33,7 @@ (if (get merged-fx k) (if (mergeable-keys k) (update merged-fx k into v) - (do (log/error "Merging fx with common-key: " k v) + (do (log/error "Merging fx with common-key: " k v (get merged-fx k)) (reduced {:merging-fx-with-common-keys k}))) (assoc merged-fx k v)))) fx diff --git a/test/cljs/status_im/test/models/bootnode.cljs b/test/cljs/status_im/test/models/bootnode.cljs index f1d0065c42..5b96b9af3b 100644 --- a/test/cljs/status_im/test/models/bootnode.cljs +++ b/test/cljs/status_im/test/models/bootnode.cljs @@ -19,7 +19,7 @@ {:random-id-generator (constantly "some-id") :db {:bootnodes/manage new-bootnode :network "mainnet_rpc" - :multiaccount {}}})] + :multiaccount {:not-empty "would throw an error if was empty"}}})] (is (= expected (get-in actual [:db :multiaccount :bootnodes]))))) (testing "adding an existing bootnode" (let [new-bootnode {:id {:value "a"} diff --git a/test/cljs/status_im/test/multiaccounts/update/core.cljs b/test/cljs/status_im/test/multiaccounts/update/core.cljs index 9223b12f74..d78599fb9d 100644 --- a/test/cljs/status_im/test/multiaccounts/update/core.cljs +++ b/test/cljs/status_im/test/multiaccounts/update/core.cljs @@ -4,10 +4,14 @@ [status-im.multiaccounts.update.core :as multiaccounts.update])) (deftest test-multiaccount-update - (let [efx (multiaccounts.update/multiaccount-update {:db {:multiaccount {}}} nil {}) + ;;TODO this test case actually shows that we are doing a needless rpc call when + ;;there is no changes, but it is an edge case that shouldn't really happen + (let [efx (multiaccounts.update/multiaccount-update + {:db {:multiaccount {:not-empty "would throw an error if was empty"}}} + nil {}) json-rpc (into #{} (map :method (::json-rpc/call efx)))] (is (json-rpc "settings_saveConfig")) - (is (= (get-in efx [:db :multiaccount]) {})))) + (is (= (get-in efx [:db :multiaccount]) {:not-empty "would throw an error if was empty"})))) (deftest test-clean-seed-phrase (let [efx (multiaccounts.update/clean-seed-phrase {:db {:multiaccount {:mnemonic "lalalala"}}})