Wallet Connect 2.0 integration #12701
Conversation
Jenkins BuildsClick to see older builds (290)
|
briansztamfater
force-pushed
the
feat/wallet-connect-2.0
branch
from
a00eaa6 to
dcc8417
Compare
|
just wondering, if WC2.0 uses waku why don't we have our own implementation? |
|
i mean we should use our waku implementation in status-go, no sense to have an implementation in js with all dependencies |
briansztamfater
force-pushed
the
feat/wallet-connect-2.0
branch
from
f29b231 to
04884ba
Compare
Yes, it make sense, I'm using the client implementation by now to implement the flows and have a basic implementation and will switch to pure wakuv2 implementation on a later iteration. |
briansztamfater
force-pushed
the
feat/wallet-connect-2.0
branch
from
cb96d1b to
fa63133
Compare
briansztamfater
force-pushed
the
feat/wallet-connect-2.0
branch
2 times, most recently
from
c233ac7 to
d62bec4
Compare
briansztamfater
changed the title
briansztamfater
force-pushed
the
feat/wallet-connect-2.0
branch
2 times, most recently
from
4d7423a to
cd5b5ac
Compare
briansztamfater
changed the title
briansztamfater
force-pushed
the
feat/wallet-connect-2.0
branch
13 times, most recently
from
dd539a5 to
5a7659d
Compare
|
@churik Made several fixes, and tested them with a Xiaomi MI 10 T, and also iOS Simulator, now it seems to be working fine. Can you try again? Let me know any issues :) |
|
@briansztamfater FILE.2022-02-10.13.21.02.mp4Logs: logs (3).zip |
|
Link on the PR description was outdated, https://react-app.walletconnect.com/ seems to work. I updated the link with this one. |
|
New dapp url: https://react-app-git-extend-expiry-method-walletconnect1.vercel.app/ ISSUE 1: can't sign typed message (Mainnet)Steps:
Expected result: Actual result: OS: IOS, Android ISSUE 2: can't use eth_sendTransaction (Mainnet)Steps:
Expected result: network fee is set, can sign tx Question 3: should tap on "Cancel" in app discard Pending JSON-RPC Request?Sometimes tapping on some button in dapp (i.e. personal sign) may lead to several bottom sheet in Status app. Not sure we are considering that as an issue on that stage. ISSUE 4: if changing wallet after connecting to wallet-connect dapp, the old account will be shownSteps:
|
|
@churik Thanks for the feedback! Issue 1: This issue seems like a bug on the dapp side, because it always sends chainId: 42 in the message payload. I think chain ID 42 is Kovan network, should work on that network. I can see if I can fork the dapp repo to send the current chain ID, but at the meantime you could try to configure Kovan network and see if it works (I tested before and worked with Kovan). Issue 2: I need to check this. I didn't test it with value transactions because I didn't have funds, but I'll see how can I reproduce it. Question 3: I would say that would not be a real life scenario so I'd dismiss it by now and we can improve later if it makes sense, but I don't have a strong opinion about it. Issue 4: What happens if you refresh the browser website after changing the wallet account? If it changes, I assume it is a bug on the dapp side that is not handling session updates in real time. |
Yes, you're right that's dapp behavior - after 10 sec or so after refreshing right wallet is shown, so can skip issue 4 |
|
I've managed to get some test ETH on Kovan network. |
|
Ok, tried around 20 apps from https://walletconnect.com/registry/apps and yeah, I suppose most of them are using wallet-connect 1.0 (which is understandable) |
| @@ -39,6 +40,8 @@ | |||
|
|
|||
| (def linear-gradient (reagent/adapt-react-class LinearGradient)) | |||
|
|
|||
| (def blur-view (reagent/adapt-react-class (.-BlurView blur))) | |||
There was a problem hiding this comment.
just wondering, why do we want to use it? i mean we already have bottom sheets and popups, but why for wallet connect we want to use blur-view?
There was a problem hiding this comment.
We use it for a blur effect here 
There was a problem hiding this comment.
mhm thats interesting, thanks, i guess we have other places in the app where we could use it
There was a problem hiding this comment.
btw, why do we need select account sheet? why don't select it right on the connection sheet ?
There was a problem hiding this comment.
i mean its a cool idea to select account with horizontal scroll so you don't need additional sheet. but for some reason we do it in additional sheet
There was a problem hiding this comment.
It's more of a session management sheet, at that moment the session is connected but you can update the session account or disconnect. We'll also re-use it to manage the list of active sessions.
src/status_im/ui/screens/wallet_connect/session_proposal/views.cljs
Outdated
Show resolved
Hide resolved
| :on-select on-select} | ||
| :horizontal true | ||
| :shows-horizontal-scroll-indicator false | ||
| :extraData @selected-account |
There was a problem hiding this comment.
We tell FlatList to re-render when the extraData changes
| :on-press on-press}]])) | ||
|
|
||
| (defview success-sheet-view [{:keys [topic]}] | ||
| (letsubs [visible-accounts @(re-frame/subscribe [:visible-accounts-without-watch-only]) |
There was a problem hiding this comment.
if you use letsubs you don't need @(re-frame/subscribe
There was a problem hiding this comment.
Yes, absolutely, my bad. Fixed
| :color :secondary | ||
| :style styles/message-title} | ||
| (i18n/label :t/manage-connections)] | ||
| [react/view (styles/footer) |
There was a problem hiding this comment.
Not specific reason in this case, just refactored it
| dapps-account @(re-frame/subscribe [:dapps-account]) | ||
| icon-uri (when (and icons (> (count icons) 0)) (first icons)) | ||
| selected-account (reagent/atom dapps-account)] | ||
| [react/view (styles/acc-sheet) |
| (fx/defn wallet-connect-client-initate | ||
| {:events [:wallet-connect/client-init]} | ||
| [{:keys [db] :as cofx} client] | ||
| (subscribe-to-events client) |
| (let [client (get db :wallet-connect/client) | ||
| wallet-connect-enabled? (get db :wallet-connect/enabled?)] | ||
| (when wallet-connect-enabled? | ||
| (.pair client (clj->js {:uri data}))) |
| [{:keys [db]} account] | ||
| (let [client (get db :wallet-connect/client) | ||
| proposal (get db :wallet-connect/proposal)] | ||
| (-> ^js client |
There was a problem hiding this comment.
Moved to effect (and so on... :P)
| metadata (get db :wallet-connect/proposal-metadata) | ||
| response {:state {:accounts accounts} | ||
| :metadata config/default-wallet-connect-metadata}] | ||
| (-> ^js client |
briansztamfater
force-pushed
the
feat/wallet-connect-2.0
branch
from
7d27b92 to
135e762
Compare
briansztamfater
force-pushed
the
feat/wallet-connect-2.0
branch
2 times, most recently
from
c0f5c57 to
1046cae
Compare
briansztamfater
requested review from
flexsurfer
and removed request for
shivekkhurana
| icon-uri (when (and icons (> (count icons) 0)) (first icons)) | ||
| address (last (string/split (first accounts) #":")) | ||
| account (first (filter #(= (:address %) address) visible-accounts)) | ||
| selected-account (reagent/atom account)] |
There was a problem hiding this comment.
i think we still don't have any agreement on how to name atoms, but it's better to somehow show that its an atom, selected-account-atom would work for me
There was a problem hiding this comment.
why do you need to wrap it with the atom btw ?
There was a problem hiding this comment.
We need to get the session account and wrap it with the atom so user can select another one and pass it to extraData props of flat list so Flat List re-renders when value of the atom changes.
| {:keys [name icons url]} metadata | ||
| icon-uri (when (and icons (> (count icons) 0)) (first icons)) | ||
| account-address (last (string/split (first accounts) #":")) | ||
| selected-account (reagent/atom (first (filter #(= (:address %) account-address) visible-accounts)))] |
There was a problem hiding this comment.
atom must be defined outside the render function, don't remember if its play well with letsubs , should work I guess
There was a problem hiding this comment.
I would leave this as is if possible, we need to initialize the atom with the session address value and pass it to the flat list to work properly. Any suggestions on how to do this without defining in let?
There was a problem hiding this comment.
we can leave it because its not critical here, but in proper way you have to create extra function pass params in it and in that function define atom outside render function
| (letsubs [visible-accounts [:visible-accounts-without-watch-only] | ||
| dapps-account [:dapps-account]] | ||
| (let [icon-uri (when (and icons (> (count icons) 0)) (first icons)) | ||
| selected-account (reagent/atom dapps-account)] |
| (fx/defn switch-wallet-connect-enabled | ||
| {:events [:multiaccounts.ui/switch-wallet-connect-enabled]} | ||
| [{:keys [db]} enabled?] | ||
| (when enabled? |
| (fx/defn proposal-handler | ||
| {:events [:wallet-connect/proposal]} | ||
| [{:keys [db] :as cofx} request-event] | ||
| (let [proposal (js->clj request-event :keywordize-keys true) |
| (when (#{"eth_accounts" "eth_coinbase"} method) | ||
| (wallet-connect-complete-transaction cofx message-id topic (if (= method "eth_coinbase") dapps-address [dapps-address])))))) | ||
|
|
||
| (def permissioned-method |
There was a problem hiding this comment.
same methods as for dapps ? why copy them ?
There was a problem hiding this comment.
I think is not used, will delete it
briansztamfater
force-pushed
the
feat/wallet-connect-2.0
branch
2 times, most recently
from
36d0fa9 to
9915d07
Compare
|
Hey @briansztamfater! IMG_1081.MP4 |
May be related to wallet registry not published yet on wallet connect. Will check with @jakubgs and come back to you |
|
Still pending review:
|
|
Thanks @briansztamfater, works great now. |
Signed-off-by: Brian Sztamfater <brian@status.im>
briansztamfater
force-pushed
the
feat/wallet-connect-2.0
branch
from
a20605e to
bd7da02
Compare
fixes #12546
Summary
Wallet Connect 2.0 integration. This first approach, as discussed with the team, includes the client library, which is a centralized and experimental approach because it depends on a relay proxy server and the wallet-connect/client package. The reason behind this is that go-waku does not support the waku2 methods that are needed to use to implement a fully decentralized approach, and also status-go does not support having a waku and waku2 connection in parallel. The plan is to remove the
@wallet-connect/clientdependency in the future and replace it with a pure waku2 implementation when feasible.Platforms
Areas that maybe impacted
Functional
Steps to test
eth_signTypedDataincludes a chainId parameter, so wallet should be configured on that same network in order to sign that messagestatus: ready