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 <eric@status.im>
This commit is contained in:
yenda 2019-09-24 18:45:33 +02:00
parent bf6b328996
commit c047b588ed
No known key found for this signature in database
GPG Key ID: 0095623C0069DCE6
4 changed files with 28 additions and 15 deletions

View File

@ -5,7 +5,8 @@
[status-im.transport.message.contact :as message.contact] [status-im.transport.message.contact :as message.contact]
[status-im.transport.message.protocol :as protocol] [status-im.transport.message.protocol :as protocol]
[status-im.utils.fx :as fx] [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}] (fx/defn multiaccount-update-message [{:keys [db] :as cofx}]
(let [multiaccount (:multiaccount db) (let [multiaccount (:multiaccount db)
@ -53,11 +54,15 @@
:params ["multiaccount" (types/serialize new-multiaccount)] :params ["multiaccount" (types/serialize new-multiaccount)]
:on-success on-success}]} :on-success on-success}]}
{:keys [name photo-path prefered-name]} new-multiaccount-fields] {:keys [name photo-path prefered-name]} new-multiaccount-fields]
(if (or name photo-path prefered-name) (if (empty? current-multiaccount)
(fx/merge cofx ;; NOTE: this should never happen, but if it does this is a critical error
fx ;; and it is better to crash than risk having an unstable state
(send-multiaccount-update)) (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"))
fx))) (if (or name photo-path prefered-name)
(fx/merge cofx
fx
(send-multiaccount-update))
fx))))
(fx/defn clean-seed-phrase (fx/defn clean-seed-phrase
"A helper function that removes seed phrase from storage." "A helper function that removes seed phrase from storage."
@ -72,8 +77,12 @@
settings settings
{:keys [on-success] :or {on-success #()}}] {:keys [on-success] :or {on-success #()}}]
(let [new-multiaccount (assoc multiaccount :settings settings)] (let [new-multiaccount (assoc multiaccount :settings settings)]
{:db (assoc db :multiaccount new-multiaccount) (if (empty? multiaccount)
::json-rpc/call ;; NOTE: this should never happen, but if it does this is a critical error
[{:method "settings_saveConfig" ;; and it is better to crash than risk having an unstable state
:params ["multiaccount" (types/serialize new-multiaccount)] (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"))
:on-success on-success}]})) {:db (assoc db :multiaccount new-multiaccount)
::json-rpc/call
[{:method "settings_saveConfig"
:params ["multiaccount" (types/serialize new-multiaccount)]
:on-success on-success}]})))

View File

@ -33,7 +33,7 @@
(if (get merged-fx k) (if (get merged-fx k)
(if (mergeable-keys k) (if (mergeable-keys k)
(update merged-fx k into v) (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}))) (reduced {:merging-fx-with-common-keys k})))
(assoc merged-fx k v)))) (assoc merged-fx k v))))
fx fx

View File

@ -19,7 +19,7 @@
{:random-id-generator (constantly "some-id") {:random-id-generator (constantly "some-id")
:db {:bootnodes/manage new-bootnode :db {:bootnodes/manage new-bootnode
:network "mainnet_rpc" :network "mainnet_rpc"
:multiaccount {}}})] :multiaccount {:not-empty "would throw an error if was empty"}}})]
(is (= expected (get-in actual [:db :multiaccount :bootnodes]))))) (is (= expected (get-in actual [:db :multiaccount :bootnodes])))))
(testing "adding an existing bootnode" (testing "adding an existing bootnode"
(let [new-bootnode {:id {:value "a"} (let [new-bootnode {:id {:value "a"}

View File

@ -4,10 +4,14 @@
[status-im.multiaccounts.update.core :as multiaccounts.update])) [status-im.multiaccounts.update.core :as multiaccounts.update]))
(deftest test-multiaccount-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)))] json-rpc (into #{} (map :method (::json-rpc/call efx)))]
(is (json-rpc "settings_saveConfig")) (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 (deftest test-clean-seed-phrase
(let [efx (multiaccounts.update/clean-seed-phrase {:db {:multiaccount {:mnemonic "lalalala"}}}) (let [efx (multiaccounts.update/clean-seed-phrase {:db {:multiaccount {:mnemonic "lalalala"}}})