Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: wallet router v2 #20631

Merged
merged 23 commits into from
Jul 26, 2024
Merged

feat: wallet router v2 #20631

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
04db308
add v2 method
BalogunofAfrica Jul 2, 2024
0d3ad5e
rename v2 to get-suggested-route
BalogunofAfrica Jul 2, 2024
843f5bb
remove timestamp check on success
BalogunofAfrica Jul 2, 2024
ffca87d
handle async signal for suggestion
BalogunofAfrica Jul 2, 2024
ee37ba1
fix stop get suggested routes
BalogunofAfrica Jul 3, 2024
a93a4ce
address feedback
BalogunofAfrica Jul 4, 2024
fd144ed
rename get-suggest-route
BalogunofAfrica Jul 4, 2024
8bd613b
prefer lazy seq
BalogunofAfrica Jul 4, 2024
d32a34a
fix formatting
BalogunofAfrica Jul 4, 2024
b417ebe
update suggested routes success
BalogunofAfrica Jul 4, 2024
d1cf96e
refactor get-in calls in start-get-suggested-routes
BalogunofAfrica Jul 4, 2024
4854aec
move transformations to data store
BalogunofAfrica Jul 4, 2024
784ef8a
clean suggested routes immediately
BalogunofAfrica Jul 4, 2024
ad24374
fix lint
BalogunofAfrica Jul 4, 2024
a9fdc3b
pass precision as ar
BalogunofAfrica Jul 5, 2024
8551697
change test name
BalogunofAfrica Jul 22, 2024
8dca5ce
fix big number division error (issues 1,2)
BalogunofAfrica Jul 22, 2024
8b0e6c9
only trigger router fetch when there address (to/from)
BalogunofAfrica Jul 23, 2024
6b3afa9
check response data for error response when routes received via signal
BalogunofAfrica Jul 24, 2024
e43012a
update status-go
BalogunofAfrica Jul 24, 2024
b61cf72
fix: test failure
BalogunofAfrica Jul 24, 2024
537c706
update status go
BalogunofAfrica Jul 25, 2024
75b31ee
handle error message for generic errors
BalogunofAfrica Jul 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions src/status_im/common/signals/events.cljs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,9 @@
"wallet"
{:fx [[:dispatch [:wallet/signal-received event-js]]]}

"wallet.suggested.routes"
{:fx [[:dispatch [:wallet/handle-suggested-routes (transforms/js->clj event-js)]]]}

"envelope.sent"
(messages.transport/update-envelopes-status
cofx
Expand Down
2 changes: 1 addition & 1 deletion src/status_im/contexts/wallet/bridge/input_amount/view.cljs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@

(defn view
[]
(hot-reload/use-safe-unmount #(rf/dispatch [:wallet/clean-routes-calculation]))
(hot-reload/use-safe-unmount #(rf/dispatch [:wallet/stop-get-suggested-routes]))
[rn/view {:style style/bridge-send-wrapper}
[input-amount/view
{:current-screen-id :screen/wallet.bridge-input-amount
Expand Down
75 changes: 75 additions & 0 deletions src/status_im/contexts/wallet/data_store.cljs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
[clojure.string :as string]
[status-im.constants :as constants]
[status-im.contexts.wallet.common.utils.networks :as network-utils]
[status-im.contexts.wallet.send.utils :as send-utils]
[utils.collection :as utils.collection]
[utils.money :as money]
[utils.number :as utils.number]
Expand Down Expand Up @@ -188,3 +189,77 @@
:removed-account-addresses removed-account-addresses
:updated-keypairs-by-id updated-keypairs-by-id
:updated-accounts-by-address updated-accounts-by-address}))

(defn- rename-keys-to-kebab-case
[m]
(set/rename-keys m (zipmap (keys m) (map transforms/->kebab-case-keyword (keys m)))))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this implementation should live in utils.transforms.

But the implementation can be faster and simpler.

(update-keys m ->kebab-case-keyword)

I would say we don't even need this utility function because the version with update-keys is quite nice already.

I checked on a benchmark and rename-keys is significantly slower, unless the map of key transformations can be stored statically. But the version with update-keys don't suffer from this problem, so it's equally fast in the specific case and faster in the general one.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc @ulisesmac


(defn rpc->suggested-routes
Copy link
Contributor

@ilmotta ilmotta Jul 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see this function as disguised as something more general. If I simply rename the bindings, the function can be readily moved to utils.transforms and be reusable across the code. So I think it doesn't belong in this namespace actually.

Also note that I used reduce-kv and directly used update-keys.

(defn recursive-kebab-case-keyword
  [coll]
  (cond
    (map? coll)
    (reduce-kv (fn [m k v]
                 (assoc m k (recursive-kebab-case-keyword v)))
               {}
               (update-keys coll ->kebab-case-keyword))

    (vector? coll)
    (map recursive-kebab-case-keyword coll)

    :else coll))

This solution won't work with other types of data, like sets. Also the check with vector? is too rigid. We're running on the risk of not transforming certain keys if the value is a lazy list for example.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ilmotta I already tried rewriting the cske/transform-keys function to do less work, although I've got better results I wasn't sure it was the best approach. For example, that function is adding metadata with the previous values, if we skip those operations, the transformations are faster, although the main problem is using a Clojure walker.

We could try exploring a performant solution, but I honestly think it's going to be expensive to catch all cases

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it @ulisesmac 👍🏼 I benchmarked the recursive implementation from @BalogunofAfrica in this PR and it performs significantly better than cske/transform-keys, although of course it's of limited usage. But cske/transform-keys I think shouldn't be used.

In fact I think we shouldn't be doing recursive key transformations even with our own code because the hand-rolled code with raw maps (not using rename-keys or update-keys) to transform keys is considerably faster as far as benchmarked. I thought we had an agreement about this, but then found we're using in the wallet context. That's okay, I was just surprised :)

Also it's not like we need to rename keys all the time from status-go, usually we're careful with that nowadays because it's a breaking change. So we might be optimizing too much for ergonomics in a more sensitive part of the stack.

I'm copying @cammellos and @flexsurfer just to see if they are okay with this. If not we should probably discuss on Discord and not here because there are already many comments.

Thanks for the insights as usual @ulisesmac

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry for the late reply, I would be leaning towards avoiding dynamic transformation of keys, we had to remove them in at least 2/3 instances on performance grounds, so I would be wary of using them, albeit of course they provide a great dev experience

[suggested-routes]
(cond
(map? suggested-routes)
(into {}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's known that processing maps as tuples is slow. Almost always the transformation can be done with reduce-kv, which is significantly faster. It also leads to simpler code. Please, check my previous comment where reduce-kv is used.

(for [[k v] (rename-keys-to-kebab-case suggested-routes)]
[k (rpc->suggested-routes v)]))

(vector? suggested-routes)
(map rpc->suggested-routes suggested-routes)

:else suggested-routes))

(def ^:private precision 6)

(defn new->old-route-path
[new-path]
(let [bonder-fees (:tx-bonder-fees new-path)
token-fees (+ (money/wei->ether bonder-fees)
(money/wei->ether
(:tx-token-fees new-path)))]
{:from (:from-chain new-path)
:amount-in-locked (:amount-in-locked new-path)
:amount-in (:amount-in new-path)
:max-amount-in "0x0"
:gas-fees {:gas-price "0"
:base-fee (send-utils/convert-to-gwei (:tx-base-fee
new-path)
precision)
:max-priority-fee-per-gas (send-utils/convert-to-gwei (:tx-priority-fee
new-path)
precision)
:max-fee-per-gas-low (send-utils/convert-to-gwei
(get-in
new-path
[:suggested-levels-for-max-fees-per-gas
:low])
precision)
:max-fee-per-gas-medium (send-utils/convert-to-gwei
(get-in
new-path
[:suggested-levels-for-max-fees-per-gas
:medium])
precision)
:max-fee-per-gas-high (send-utils/convert-to-gwei
(get-in
new-path
[:suggested-levels-for-max-fees-per-gas
:high])
precision)
:l-1-gas-fee (send-utils/convert-to-gwei (:tx-l-1-fee
new-path)
precision)
:eip-1559-enabled true}
:bridge-name (:processor-name new-path)
:amount-out (:amount-out new-path)
:approval-contract-address (:approval-contract-address new-path)
:approval-required (:approval-required new-path)
:estimated-time (:estimated-time new-path)
:approval-gas-fees (* (money/wei->ether (get-in new-path
[:suggested-levels-for-max-fees-per-gas
:medium]))
(:approval-gas-amount new-path))
:to (:to-chain new-path)
:bonder-fees bonder-fees
:approval-amount-required (:approval-amount-required new-path)
;; :cost () ;; tbd not used on desktop
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Double comment ;;

:token-fees token-fees
:gas-amount (:tx-gas-amount new-path)}))
Loading