chore: disable non selectable bridge assets #20846
Conversation
J-Son89
force-pushed
the
jc/disable-bridges
branch
from
08bbbb5
to
f966429
Compare
| :stack-id :screen/wallet.accounts | ||
| :start-flow? true} | ||
| {:token-symbol token-symbol | ||
| :bridge-disabled? (not (send-utils/bridgeable? token-symbol)) |
There was a problem hiding this comment.
@ulisesmac - should this be done in a subscription instead? wdyt? 🤔
There was a problem hiding this comment.
If I'm not mistaken, the sub :wallet/current-viewing-account-tokens-filtered (used in this view) depends on token-symbol already adds this boolean
We just need to de-structure token-data and use it
Quick thoughts: Why do we not hide the option to bridge instead of disabling it?
There was a problem hiding this comment.
@J-Son89 Both are fine but I'd prefer having it inside the subscription.
If it exists in the subscription, it'll be easier to be reused and the logic would be centralized.
There was a problem hiding this comment.
:wallet/current-viewing-account-tokens-filtere does, but it won't work for the home page.
I figured disabling is better as when the list is empty at least the user will see something on the page of assets to select / it doesn't look like assets are disappearing then.
J-Son89
requested review from
vkjr,
smohamedjavid,
OmarBasem,
BalogunofAfrica and
mmilad75
Jenkins BuildsClick to see older builds (37)
|
src/status_im/constants.cljs
Outdated
| @@ -558,6 +558,8 @@ | |||
| (def ^:const bridge-name-erc-1155-transfer "ERC1155Transfer") | |||
| (def ^:const bridge-name-hop "Hop") | |||
|
|
|||
| (def ^:const bridge-assets ["ETH" "USDT" "USDC" "DAI"]) | |||
There was a problem hiding this comment.
It's good to use set here as the data(token symbols) will be distinct.
Additionally, are we planning to add these supported tokens as well?
status-im/status-desktop#15697 (comment)
| (def ^:const bridge-assets ["ETH" "USDT" "USDC" "DAI"]) | |
| (def ^:const bridge-assets #{"ETH" "USDT" "USDC" "USDC.e" "DAI" "HOP" "SNX" "sUSD" "rETH" "MAGIC"}) |
There was a problem hiding this comment.
I think we do not need these tokens just yet as they are unstable for the moment.
cc @churik
There was a problem hiding this comment.
Is this the default list of bridge assets? (if so, could be renamed to default-bridge-assets)
|
|
||
| (defn bridgeable? | ||
| [token-symbol] | ||
| (some #(= % token-symbol) constants/bridge-assets)) |
There was a problem hiding this comment.
Perf suggestion: If we update the constants/bridge-assets to set, then updating this some to contains? will be better.
| (some #(= % token-symbol) constants/bridge-assets)) | |
| (contains? constants/bridge-assets token-symbol)) |
| :stack-id :screen/wallet.accounts | ||
| :start-flow? true} | ||
| {:token-symbol token-symbol | ||
| :bridge-disabled? (not (send-utils/bridgeable? token-symbol)) |
There was a problem hiding this comment.
If I'm not mistaken, the sub :wallet/current-viewing-account-tokens-filtered (used in this view) depends on token-symbol already adds this boolean
We just need to de-structure token-data and use it
Quick thoughts: Why do we not hide the option to bridge instead of disabling it?
| :state (cond | ||
| disabled? :disabled | ||
| (= preselected-token-symbol token-symbol) | ||
| :selected | ||
| :else nil)}])) |
There was a problem hiding this comment.
cond doesn't need an :else clause, it returns nil when nothing matches.
| :stack-id :screen/wallet.accounts | ||
| :start-flow? true} | ||
| {:token-symbol token-symbol | ||
| :bridge-disabled? (not (send-utils/bridgeable? token-symbol)) |
There was a problem hiding this comment.
@J-Son89 Both are fine but I'd prefer having it inside the subscription.
If it exists in the subscription, it'll be easier to be reused and the logic would be centralized.
|
Hey, @J-Son89 we don't want to support bridging all tokens that Hop + Celer support for now? |
|
@IvanBelyakoff they are not working, and there is no confidence when Celer will start working inside our app. I don't think that it might happen any time soon (i.e. in couple of weeks) and we should release the app sooner (referring to this conversation ) Hopefully in the future when we can add more providers, we can expand the list. |
|
@J-Son89 ping me when it is ready to test |
| @@ -411,6 +411,7 @@ | |||
| (fn [[account networks] [_ query chain-ids]] | |||
| (let [tokens (map (fn [token] | |||
| (assoc token | |||
| :bridge-disabled? (not (send-utils/bridgeable? (:symbol token))) | |||
There was a problem hiding this comment.
I would suggest to use either of the namings bridge-disabled? or bridgeable? as they mean the same thing
| @@ -46,3 +50,4 @@ | |||
| :key-fn :symbol | |||
| :on-scroll-to-index-failed identity | |||
| :render-fn asset-component}])) | |||
|
|
|||
src/status_im/constants.cljs
Outdated
| @@ -558,6 +558,8 @@ | |||
| (def ^:const bridge-name-erc-1155-transfer "ERC1155Transfer") | |||
| (def ^:const bridge-name-hop "Hop") | |||
|
|
|||
| (def ^:const bridge-assets ["ETH" "USDT" "USDC" "DAI"]) | |||
There was a problem hiding this comment.
Is this the default list of bridge assets? (if so, could be renamed to default-bridge-assets)
J-Son89
force-pushed
the
jc/disable-bridges
branch
from
f966429
to
40c7908
Compare
| @@ -18,6 +18,7 @@ | |||
| :padding-vertical 8 | |||
| :border-radius 12 | |||
| :height 56 | |||
| :opacity (when (= state :disabled) 0.3) | |||
There was a problem hiding this comment.
this is in design system, just adding it now.
71% of end-end tests have passedFailed tests (1)Click to expandClass TestWalletMultipleDevice:
Expected to fail tests (1)Click to expandClass TestWalletOneDevice:
Passed tests (5)Click to expandClass TestWalletOneDevice:
Class TestCommunityOneDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestWalletMultipleDevice:
|
|
@J-Son89 did we leave the opportunity for the user to select an unbridgable token? IMG_7704.MP4I'd expect that we can't select these tokens, but it is already way better than not show anything at all |
sorry @churik, that is my basic mis step. I left that part out foolishly 😓 |
J-Son89
force-pushed
the
jc/disable-bridges
branch
2 times, most recently
from
e0416e9
to
8eea7e3
Compare
|
sorry @churik - my brain didn't initially comprehend that was an expectation 😅 disabled assets should not be clickable now. |
|
@J-Son89 :) assets are disabled in Send flow as well: FILE.2024-07-24.16.33.20.mp4Expected: that these assets are disable only in Brodge flow |
J-Son89
force-pushed
the
jc/disable-bridges
branch
5 times, most recently
from
e52b0b2
to
cf8032a
Compare
J-Son89
force-pushed
the
jc/disable-bridges
branch
from
e6515de
to
f04ed09
Compare
J-Son89
force-pushed
the
jc/disable-bridges
branch
from
f04ed09
to
2abdd4f
Compare
|
@churik this pr should be ready now :) |
|
Amazing work, thank you @J-Son89 !!! |
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: #20842
account page bottom sheet
home page bottom sheet
bridge page
To test:
check the bridge entry points,
wallet home long press on asset
account page long press on asset
bridge flow from account page cta
only a ETH and stable coins should be bridgeable.