feat: wallet router v2 #20631
feat: wallet router v2 #20631
Conversation
BalogunofAfrica
requested review from
ilmotta,
vkjr,
J-Son89,
OmarBasem and
ulisesmac
| @@ -24,79 +24,76 @@ | |||
| {:db (assoc-in db [:wallet :ui :send :select-address-tab] tab)})) | |||
|
|
|||
| (rf/reg-event-fx :wallet/suggested-routes-success | |||
| (fn [{:keys [db]} [suggested-routes timestamp]] | |||
| (when (= (get-in db [:wallet :ui :send :suggested-routes-call-timestamp]) timestamp) | |||
There was a problem hiding this comment.
Check was removed because the call to the endpoint and where it is successfully (signals) handled has changed
There was a problem hiding this comment.
The asynchronous endpoint would override the current call if called again while the request is pending.
So, I don't think this timestamp check is useful in this context
There was a problem hiding this comment.
With this asynchronous code. Should we still debounce the calls, since the async code on status go uses a queue to resolve this and current running requests are overridden till there is a result.
cc: @J-Son89
There was a problem hiding this comment.
@briansztamfater, @vkjr probably know better.
There was a problem hiding this comment.
The asynchronous endpoint would override the current call if called again while the request is pending.
So, I don't think this timestamp check is useful in this context
I think we can remove this check 👍
There was a problem hiding this comment.
How about the debounce, do you think it is safe to remove? cc: @briansztamfater @vkjr
There was a problem hiding this comment.
@BalogunofAfrica, I think we should leave it. The debounce is there to prevent sending multiple events when user changes something on token input screen, for example when user types in the amount to send. We do not want to invoke routes recalculation on each change, only when user stopped for a bit)
Even if it is resolved on status-go side, we don't have to rely on that implementation, it can change one day without us knowing and we will get unexpected performance issue)
| :error error | ||
| :params params}))}]}))) | ||
|
|
||
| (rf/reg-event-fx :wallet/stop-get-suggested-routes |
There was a problem hiding this comment.
Still haven't figured out where this would be useful
There was a problem hiding this comment.
Replaced clean calculation with this endpoint
There was a problem hiding this comment.
Still haven't figured out where this would be useful
You used this event on unmount if I'm not mistaken. That seems good to me because it avoids needless computation of processing a signal that's not needed anymore.
There was a problem hiding this comment.
If we have :wallet/stop-get-suggested-routes, wdyt of refactoring the other event to :wallet/start-get-suggested-routes for consistency?
Jenkins BuildsClick to see older builds (102)
|
| {:event :wallet/stop-get-suggested-routes | ||
| :error error}))}]})) | ||
|
|
||
| (defn- transform-new-to-old-path |
There was a problem hiding this comment.
Maps the v2 paths to the (old) router paths.
Destop's implementation https://github.com/status-im/status-desktop/pull/15344/files#diff-1da937d55df9cf5890ba3e3081a0a5dc254fefdf76b5b2f7f8b408183eaac940R12-R74
BalogunofAfrica
force-pushed
the
feat/router-v2
branch
from
09df89e
to
b6a0d39
Compare
| :to (:to-chain new-path) | ||
| :bonder-fees bonder-fees | ||
| :approval-amount-required (:approval-amount-required new-path) | ||
| ;; :cost () ;; tbd not used on desktop |
There was a problem hiding this comment.
Checked and we don't seem to use this, same as desktop
BalogunofAfrica
force-pushed
the
feat/router-v2
branch
from
b6a0d39
to
492d326
Compare
| token-networks (:networks token) | ||
| token-networks-ids (when token-networks (mapv #(:chain-id %) token-networks)) | ||
| from-network-amounts-by-chain (send-utils/network-amounts-by-chain {:route chosen-route | ||
| :token-decimals |
There was a problem hiding this comment.
This part of the code is weirdly formatted because it's too right-aligned. One idea is to put the map on the next line after send-utils/network-amounts-by-chain. This will be an improvement because each key pair will be on the same line. Same idea for the other call to send-utils/network-amounts-by-chain.
| (send-utils/network-links chosen-route | ||
| sender-network-values | ||
| receiver-network-values))] | ||
| {:db (-> db |
There was a problem hiding this comment.
We can reduce the amount of repetition in this long chain of assocs.
{:db (update-in db
[:wallet :ui :send]
assoc
:suggested-routes suggested-routes-data
:route chosen-route
:from-values-by-chain from-network-values-for-ui
:to-values-by-chain to-network-values-for-ui
:sender-network-values sender-network-values
:receiver-network-values receiver-network-values
:network-links network-links
:loading-suggested-routes? false)}This is a safe change, but because this large event has no unit tests, it's worth doing a manual check whenever the event handler is refactored.
| (assoc-in [:wallet :ui :send :loading-suggested-routes?] false))})))) | ||
| (fn [{:keys [db]} [suggested-routes-data]] | ||
| (let [chosen-route (:best suggested-routes-data) | ||
| token (get-in db [:wallet :ui :send :token]) |
There was a problem hiding this comment.
We are calling get-in on the same nested path many times. This is inefficient. Better is to call get-in once with path [:wallet :ui :send] and destructure.
If you implement this suggestion, you may wonder, like me, what to do with the default values of disabled-from-chain-ids and from-locked-amounts while destructuring. They don't seem to be necessary because in Clojure nil punning is extensively supported on data structures.
disabled-from-chain-idswill be used first as in(set disabled-chain-ids). The default value ofdisabled-from-chain-idsis[].(set [])is equal to(set nil).from-locked-amountsdefault value is{}and is first used like(get from-locked-amounts chain-id amount), butgeton nil is perfectly correct, hence(get nil chain-id amount)will returnamountas expected.
| native-token? (and token (= token-display-name "ETH")) | ||
| routes-available? (pos? (count chosen-route)) | ||
| token-networks (:networks token) | ||
| token-networks-ids (when token-networks (mapv #(:chain-id %) token-networks)) |
There was a problem hiding this comment.
mapv is unnecessary here and possibly in other places in this namespace. Immediately later this value is actually converted to a set. When I read mapv anywhere, I need to think why the developer decided to use the non-lazy version. If there's no technical reason, it becomes a mental overhead because in Clojure we mostly want to use the lazy versions, unless we need the performance characteristics of a vector, for example to access by index efficiently. The lazy versions also play nice when we remove the last collection argument in case we want to use them as transducers.
| :disabledToChainIDs disabled-to-chain-ids | ||
| :gasFeeMode gas-rates | ||
| :fromLockedAmount from-locked-amount} | ||
| rest-keys)]] |
There was a problem hiding this comment.
I think the code is more complicated than it needs to be. rest-keys is unnecessary because you know the keys you need from it and rest-keys is not reused anywhere else. Thus, you could remove the binding rest-keys and add the keys directly to the default map.
params [{:uuid (str (random-uuid))
:sendType send-type
:addrFrom from-address
:addrTo to-address
:amountIn amount-in
:amountOut amount-out
:tokenID token-id
:toTokenID to-token-id
:disabledFromChainIDs disabled-from-chain-ids
:disabledToChainIDs disabled-to-chain-ids
:gasFeeMode gas-rates
:fromLockedAmount from-locked-amount
:username (:username args)
:publicKey (:publicKey args)
:packID (:packID args)}]| {:event :wallet/stop-get-suggested-routes | ||
| :error error}))}]})) | ||
|
|
||
| (defn- transform-new-to-old-path |
There was a problem hiding this comment.
It's conventional to name such a function with an arrow, like new->old-path or new-path->old-path.
|
|
||
| (defn- transform-new-to-old-path | ||
| [new-path] | ||
| (let [unit (-> (get-in new-path [:from-token :symbol]) |
There was a problem hiding this comment.
This function could be moved to a data store namespace and unit tested, at least one happy path test.
| (defn- transform-new-to-old-path | ||
| [new-path] | ||
| (let [unit (-> (get-in new-path [:from-token :symbol]) | ||
| clojure.string/lower-case |
There was a problem hiding this comment.
clojure.string has been already imported as string.
| :error error | ||
| :params params}))}]}))) | ||
|
|
||
| (rf/reg-event-fx :wallet/stop-get-suggested-routes |
There was a problem hiding this comment.
Still haven't figured out where this would be useful
You used this event on unmount if I'm not mistaken. That seems good to me because it avoids needless computation of processing a signal that's not needed anymore.
|
|
||
| (rf/reg-event-fx :wallet/stop-get-suggested-routes | ||
| (fn [] | ||
| {:json-rpc/call [{:method "wallet_stopSuggestedRoutesV2AsyncCalcualtion" |
There was a problem hiding this comment.
There's a typo in the word calculation.
There was a problem hiding this comment.
Yeah, the mistake is from how the API was named in status-go
https://github.com/status-im/status-go/blob/develop/services/wallet/api.go#L519-L523
| :tx/collectible-erc-1155 constants/send-type-erc-1155-transfer | ||
| :tx/bridge constants/send-type-bridge | ||
| constants/send-type-transfer) | ||
| send-type (case transaction-type |
There was a problem hiding this comment.
I know this is status-go domain, but I am wondering why this parameter is called SendType when some values from this enum specify types of operations that doesn't involve sending (ENSRegister, StickersBuy, Swap, Bridge, etc). IMO, TransactionType is still a better name.
| convert-to-gwei #(-> % | ||
| money/wei->gwei | ||
| (money/with-precision 6) | ||
| (str)) |
There was a problem hiding this comment.
Maybe move this to a utils file?
BalogunofAfrica
force-pushed
the
feat/router-v2
branch
from
492d326
to
5b76831
Compare
| {:json-rpc/call [{:method "wallet_stopSuggestedRoutesV2AsyncCalcualtion" | ||
| :params [] | ||
| :on-success (fn [] | ||
| (rf/dispatch [:wallet/clean-routes-calculation])) |
There was a problem hiding this comment.
I'm not sure we need to rely on status-go call result to clean our local re-frame db? Or there is a reason?
There was a problem hiding this comment.
The endpoint is there to stop the asynchronous calculation from status-go so we do not get new signals with suggested routes.
There was a problem hiding this comment.
Yes, but I think that we can call :wallet/clean-routes-calculation independently because it cleans up our local re-frame db and we'd better do it anyway, even if stopSuggestedRoutes return error, wdyt?
BalogunofAfrica
force-pushed
the
feat/router-v2
branch
2 times, most recently
from
0d33931
to
dfccba1
Compare
| [n] | ||
| (-> n | ||
| money/wei->gwei | ||
| (money/with-precision 6) |
There was a problem hiding this comment.
Where is this 6 coming from? Should it be defined in a constant?
There was a problem hiding this comment.
Or I'd suggest to have a precision as an argument
BalogunofAfrica
force-pushed
the
feat/router-v2
branch
from
dfccba1
to
a613d43
Compare
BalogunofAfrica
force-pushed
the
feat/router-v2
branch
from
a613d43
to
e43c5fb
Compare
BalogunofAfrica
force-pushed
the
feat/router-v2
branch
from
6c5ce99
to
75b31ee
Compare
status-im/status-go@2bbdb35...2bbdb35 * add v2 method * rename v2 to get-suggested-route * remove timestamp check on success * handle async signal for suggestion * fix stop get suggested routes * address feedback * rename get-suggest-route * prefer lazy seq * fix formatting * update suggested routes success * refactor get-in calls in start-get-suggested-routes * move transformations to data store * clean suggested routes immediately * fix lint * pass precision as ar * change test name * fix big number division error (issues 1,2) * only trigger router fetch when there address (to/from) * check response data for error response when routes received via signal * update status-go * fix: test failure * update status go * handle error message for generic errors
Revisions from develop: - 59ceddb develop origin/develop fix(wallet): fix bridge transactions (#20902) - 99ccbc3 Cover wallet send events with tests Part 2 #20411 #20533 (#20721) - 8c2d539 Enabling WalletConnect feature flag (#20906) - 67c83b1 fix(wallet): remove edit routes button in bridging (#20874) - 11a84ba feat(wallet): disable complex routing (#20901) - 1f5bb57 chore(wallet): disable bridging on unsupported tokens (#20846) - 4586f80 Add toggle in advanced settings for mobile data - 55c620e fix: create password for small screen (#20645) - 525609f Wallet Activity: transactions are not sorted by time #20808 (#20862) - 9065395 chore(settings): Disable telemetry option (#20881) - d27ab75 fix_:display group message using the new ui (#20787) - c6a1db6 ci: enable split apks & build only for arm64-v8a (#20683) - 73777e0 Ensure keycard account can send transaction after upgrading from v1 to v2 #20552 (#20845) - a6d3fc3 [#20524] fix: the missed keypairs are shown in the key pair list screen (#20888) - a671c70 fix broken screen and navigation when syncing fails (#20887) - a45991b 🥅 Filter connected dapps based on testnet mode, reject proposals and requests gracefully (#20799) - 2e9fa22 feat: wallet router v2 (#20631) - 737d8c4 rename sub to fix error when requesting to join community (#20868) - 3aa7e10 Sync process is blocked on Enabled notifications screen (#20883) - c1d2d44 perf: Fix app freeze after login (#20729) - 0fed811 e2e: updated testnet switching and added one test into smoke - 53c35cb fix(wallet): Linear gradient exception on invalid colors for watched account cards (#20854) - be82365 chore(settings)_: Remove testnet toggle from legacy advanced settings (#20875) - eae8a65 feat(wallet)_: Add beta info box in activity tab (#20873) - fe54a25 fix: not clearing network & web3-wallet on logout (#20886) - 15a4219 Reject wallet-connect request by dragging the modal down (#20763) (#20836) - 2ffbdac WalletConnect show expired toast (#20857) - 402eb83 fix Issue with scrolling WalletConnect transaction on Android (#20867) - ff88049 Fix WalletConnect header alignment on Android (#20860) - cee2124 WalletConnect no internet edge-cases (#20826) - 60ad7c8 chore(tests): New match-strict? cljs.test directive (#20825) - 4989c92 fix_: Adding own address as saved addresses (#20839)
fixes #20575
Summary
Integrates the new routes suggestion API implemented on status-go here status-im/status-go#5426 and integrated on desktop here status-im/status-desktop#15344.
We now make use of the async endpoint and receive the suggestions as signals. The request of the old router is mapped onto the new one, and the response of the new router is mapped onto the old one, specifically the path array(s) which is what we get in both the
BestandCandidatesfields.Simulator.Screen.Recording.-.iPhone.11.Pro.-.2024-07-03.at.00.02.41.mp4
Documentation change PR (review please): https://github.com/status-im/status.im/pull/xxx
Review notes
Testing notes
Platforms
Areas that maybe impacted
Functional
Steps to test
Before and after screenshots comparison
UI behaviour remains the same
status: ready
Wallet send flow
Done
Risk
Described potential risks and worst case scenarios.
Tick one: