Implement getSuggestedRoutes endpoint in the Send Flow for wallet #18104
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Jenkins BuildsClick to see older builds (64)
|
briansztamfater
force-pushed
the
feat/suggested-routes
branch
3 times, most recently
from
d281f5f to
abec7a7
Compare
briansztamfater
commented
Dec 6, 2023
Comment on lines
+147
to
+149
| (cond loading-suggested-routes? | ||
| [quo/text "Loading routes"] | ||
| (and (not loading-suggested-routes?) route) | ||
| [quo/text "Route found"] | ||
| (and (not loading-suggested-routes?) (nil? route)) | ||
| [quo/text "Route not found"])] |
There was a problem hiding this comment.
This is temporary, should be addressed in #16929
briansztamfater
force-pushed
the
feat/suggested-routes
branch
2 times, most recently
from
5eceecf to
fa46e05
Compare
J-Son89
reviewed
Dec 7, 2023
| @@ -390,3 +390,15 @@ | |||
| (def ^:const account-default-customization-color :blue) | |||
|
|
|||
| (def ^:const wallet-account-name-max-length 20) | |||
|
|
|||
| (def ^:const gas-rate-low 0) | |||
J-Son89
reviewed
Dec 7, 2023
J-Son89
reviewed
Dec 7, 2023
J-Son89
reviewed
Dec 7, 2023
J-Son89
reviewed
Dec 7, 2023
J-Son89
reviewed
Dec 7, 2023
src/utils/money.cljs
Outdated
| @@ -205,10 +205,14 @@ | |||
| (with-precision 2) | |||
| str)) | |||
|
|
|||
| (defn add | |||
| (defn- add* | |||
| [bn1 n2] | |||
| (.add ^js bn1 n2)) | |||
|
|
|||
There was a problem hiding this comment.
not for now, but these methods should probably all be covered in tests. We can add this as part of the post 1.27 workset 👍
There was a problem hiding this comment.
I added it (and explained why) in a previous PR.
I can add some tests in a following PR @J-Son89
There was a problem hiding this comment.
@ulisesmac - I was making no comment about the added function, I understand the use case. My point is more that we are using a bunch of (afaik) untested utilities that handle numbers. Tests would be great but we can wait until post 1.27 as let's focus on the delivering those features to the best quality as possible :)
There was a problem hiding this comment.
Oh yes, that's true!, we are relying on the functions in this namespace. They need to be tested.
J-Son89
reviewed
Dec 7, 2023
| @@ -48,7 +48,7 @@ | |||
| :<- [:wallet/balances] | |||
| :<- [:wallet/tokens-loading?] | |||
| (fn [[accounts balances tokens-loading?]] | |||
| (mapv (fn [{:keys [color address type] :as account}] | |||
| (mapv (fn [{:keys [color address] :as account}] | |||
There was a problem hiding this comment.
why did you remove type? it's needed on line 54?
There was a problem hiding this comment.
Maybe we should destructure this map in another way, so that the app doesn't compile if the var doesn't exist.
(mapv (fn [{:keys [color address]
account-type :type
:as account}]
,,,
;;line 54:
:type (if (= account-type :watch) :watch-only :empty)
,,,)
[,,,])currently the app is compiling but in the line 54: (= type :watch) is always going to be false.
Just to give a little more of context on it in case it's needed:
(= type :watch)
;; => false
type
;; => #object[cljs$core$type]
J-Son89
reviewed
Dec 7, 2023
J-Son89
reviewed
Dec 7, 2023
J-Son89
reviewed
Dec 7, 2023
J-Son89
reviewed
Dec 7, 2023
| token-id (:symbol token) | ||
| network-preferences [constants/mainnet-chain-id] ; TODO: don't hardcode network preferences | ||
| gas-rates constants/gas-rate-low | ||
| amount-in (money/mul (money/bignumber amount) |
There was a problem hiding this comment.
might be better to make amount-in a helper fn?
(defn amount-in-hex [amount token-decimal]
(money/to-hex-in (money/mul (money/bignumber amount) (money/from-decimal token-decimal)))
There was a problem hiding this comment.
I think the same, as a named function it'll be clearer what these calculations mean 👍
J-Son89
reviewed
Dec 7, 2023
J-Son89
approved these changes
Dec 7, 2023
J-Son89
reviewed
Dec 7, 2023
OmarBasem
reviewed
Dec 15, 2023
Comment on lines
+249
to
+250
|
|
||
| (defn amount-in-hex |
There was a problem hiding this comment.
A test can be added for this function
58% of end-end tests have passedFailed tests (15)Click to expandClass TestGroupChatMultipleDeviceMergedNewUI:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestCommunityOneDeviceMerged:
Expected to fail tests (5)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestCommunityMultipleDeviceMerged:
Class TestCommunityOneDeviceMerged:
Passed tests (28)Click to expandClass TestActivityMultipleDevicePR:
Class TestDeepLinksOneDevice:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestCommunityMultipleDeviceMerged:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestActivityMultipleDevicePRTwo:
Class TestCommunityOneDeviceMerged:
|
75% of end-end tests have passedFailed tests (5)Click to expandClass TestCommunityOneDeviceMerged:
Class TestActivityMultipleDevicePRTwo:
Class TestActivityMultipleDevicePR:
Expected to fail tests (7)Click to expandClass TestCommunityOneDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestCommunityMultipleDeviceMerged:
Passed tests (36)Click to expandClass TestCommunityOneDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestDeepLinksOneDevice:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestCommunityMultipleDeviceMerged:
Class TestActivityCenterContactRequestMultipleDevicePR:
|
briansztamfater
force-pushed
the
feat/suggested-routes
branch
from
1d63ee8 to
21dcda2
Compare
J-Son89
force-pushed
the
feat/suggested-routes
branch
from
21dcda2 to
dc3bf43
Compare
100% of end-end tests have passedPassed tests (5)Click to expandClass TestActivityMultipleDevicePRTwo:
Class TestCommunityOneDeviceMerged:
Class TestActivityMultipleDevicePR:
|
J-Son89
force-pushed
the
feat/suggested-routes
branch
from
dc3bf43 to
05fd97e
Compare
|
Hey. @briansztamfater the routes are not found in my cases. But I have some real tokens on mainnet, optimizm, arbitrium networks. What I am doing wrong? Actual result:route.mp4Expected result:Route is found Logs: |
|
Question 1: Should this feature work in case of founding the route for testnet tokens? For now it does not work Steps:
Actual result:The route is not found goerli.mp4Expected result:Should the rout be found? |
|
Hi @VolodLytvynenko, that should at least work. This is the happy path for finding the best route for a transaction. I tested yesterday evening and it was working okay for me. Is it working on mainnet? |
@J-Son89 Yes, I tried it on the Mainnet network without any success to get the route, even when entering the minimum amount. 
|
|
@J-Son89 Have I understood the requirements correctly regarding route finding within the scope of this PR? The route should always be found if the user has enough assets on all networks. For instance: However, in the case of entering an amount higher than available, for example, '3.0001 ETH,' the route should not be found. Is there a minimum amount for a particular asset that determines whether a route should or should not be found? Or in the scope of this PR would be enough to see the route is found at least once to achieve the happy path? |
Hey @VolodLytvynenko - i.e It is mostly just for Ethereum tokens (other tokens should work too), It is only a single token transaction, i.e you are sending eth so you will then pay gas in eth and not in other tokens. The amount entered is in crypto, we will handle the fiat conversion in another issue too. |
|
@briansztamfater @J-Son89 No issues from my side. The happy flow works. Follow ups I will add separately Ready to be merged |
Signed-off-by: Brian Sztamfater <brian@status.im>
J-Son89
force-pushed
the
feat/suggested-routes
branch
from
05fd97e to
e541a2a
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
fixes #18201
Summary
This PR includes token amount selection screen into the Send flow and implements getSuggestedRoutes endpoint, which based on the response can enable the Confirm button to proceed to Transaction Confirmation Screen (to be implemented in a follow up PR)
suggestedroutesdemo.mp4
Platforms
Areas that maybe impacted
Functional
Steps to test
status: ready