chore: add needed packages for WalletConnect implementation #19758
+1,648
−84
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
briansztamfater
requested review from
ilmotta,
shivekkhurana,
smohamedjavid,
J-Son89 and
siddarthkay
briansztamfater
force-pushed
the
chore/add-wallet-connect-package
branch
3 times, most recently
from
a0bdb3b
to
c3d6dfe
Compare
Jenkins BuildsClick to see older builds (32)
|
briansztamfater
force-pushed
the
chore/add-wallet-connect-package
branch
from
c3d6dfe
to
0c8ad7a
Compare
J-Son89
approved these changes
Apr 22, 2024
J-Son89
reviewed
Apr 22, 2024
| [] | ||
| (let [core (wallet-connect-core) | ||
| metadata (clj->js config/default-wallet-connect-metadata)] | ||
| (promesa/create |
There was a problem hiding this comment.
looks weird. If Web3Wallet.init already returns a promise, there's no need to create another promise with promesa/create. Just return the promise and it would be handled in the effect it's called from.
There was a problem hiding this comment.
@clauxx makes sense, I'll modify it 👍
There was a problem hiding this comment.
Metadata can be moved to a (defonce metadata #js {...}).
It never changes so clj->js is redundant.
briansztamfater
changed the title
clauxx
reviewed
Apr 23, 2024
src/status_im/config.cljs
Outdated
| "Status is a secure messaging app, crypto wallet, and Web3 browser built with state of the art technology." | ||
| :url "https://status.app" | ||
| :icons | ||
| ["https://res.cloudinary.com/dhgck7ebz/image/upload/f_auto,c_limit,w_1080,q_auto/Brand/Logo%20Section/Mark/Mark_01"]}) |
There was a problem hiding this comment.
I extracted it from our website just to provide some values, but we can update it later on
There was a problem hiding this comment.
Do we need to care about translations here ? Should we wrap description in (i18n) ?
There was a problem hiding this comment.
Not a change, but is it possible to pass icon from disk rather than a link?
What if the infra team stops using Cloudinary tomorrow ?
There was a problem hiding this comment.
Not a change, but is it possible to pass icon from disk rather than a link? What if the infra team stops using Cloudinary tomorrow ?
From all examples I've seen, only urls are passed for icons. I think we could do some research and update this part in a follow up in case it is needed. Also copies should be temporal, we should get better ones from the marketing or design team ultimately.
| @@ -0,0 +1,21 @@ | |||
| (ns status-im.contexts.wallet.common.utils.wallet-connect | |||
There was a problem hiding this comment.
What about having the wallet-connect stuff in its own namespace e.g. status-im.contexts.wallet.wallet-connect?
| [] | ||
| (let [core (wallet-connect-core) | ||
| metadata (clj->js config/default-wallet-connect-metadata)] | ||
| (promesa/create |
There was a problem hiding this comment.
looks weird. If Web3Wallet.init already returns a promise, there's no need to create another promise with promesa/create. Just return the promise and it would be handled in the effect it's called from.
|
|
||
| (defn- wallet-connect-core | ||
| [] | ||
| (Core. (clj->js {:project-id config/wallet-connect-project-id}))) |
There was a problem hiding this comment.
are we going to have a different project-id for testnet mode?
There was a problem hiding this comment.
Should be the same project id, we should just connect to different chains
There was a problem hiding this comment.
We can move the {:project-id ...} to a defonce with #js. clj->js is redundant because project id is static.
src/status_im/config.cljs
Outdated
Comment on lines
139
to
145
| (def default-wallet-connect-metadata | ||
| {:name "Status" | ||
| :description | ||
| "Status is a secure messaging app, crypto wallet, and Web3 browser built with state of the art technology." | ||
| :url "https://status.app" | ||
| :icons | ||
| ["https://res.cloudinary.com/dhgck7ebz/image/upload/f_auto,c_limit,w_1080,q_auto/Brand/Logo%20Section/Mark/Mark_01"]}) |
There was a problem hiding this comment.
not sure if makes sense to keep it in the config. Doesn't look like something that will be used outside the initialisation from status-im.contexts.wallet.common.utils.wallet-connect and might be easier to just keep them together.
siddarthkay
approved these changes
Apr 24, 2024
smohamedjavid
approved these changes
Apr 24, 2024
src/status_im/config.cljs
Outdated
| @@ -36,6 +37,7 @@ | |||
| (def goerli-chain-explorer-link "https://goerli.etherscan.io/address/") | |||
| (def optimism-goerli-chain-explorer-link "https://goerli-optimistic.etherscan.io/address/") | |||
| (def opensea-api-key OPENSEA_API_KEY) | |||
| (def wallet-connect-project-id WALLET_CONNECT_PROJECT_ID) | |||
There was a problem hiding this comment.
Not sure why we do this but we can use the WALLET_CONNECT_PROJECT_ID directly in other places.
| @@ -0,0 +1,21 @@ | |||
| (ns status-im.contexts.wallet.common.utils.wallet-connect | |||
ilmotta
reviewed
Apr 24, 2024
| (promesa/create | ||
| (fn [p-resolve p-reject] | ||
| (-> (Web3Wallet.init | ||
| (clj->js {:core core |
There was a problem hiding this comment.
@briansztamfater, a few suggestions for this namespace:
- The code
(clj->js {:project-id config/wallet-connect-project-id})can be changed to#js {:project-id config/WALLET_CONNECT_PROJECT_ID}. I tend to prefer the#jsliteral for small, non-nested maps because it's significantly faster thanclj->js. Theinitfunction will be called once, so it won't be a problem, but sharing as a reminder that we should be mindful ofclj->js. - I would make
default-wallet-connect-metadataa private var in this namespace and remove it fromstatus-im.configbecause the map doesn't depend on any value coming from an external env.
|
|
||
| (defn- wallet-connect-core | ||
| [] | ||
| (Core. (clj->js {:project-id config/wallet-connect-project-id}))) |
There was a problem hiding this comment.
Are we sure the option is in kebab-case? Here https://github.com/WalletConnect/walletconnect-monorepo/blob/v2.0/packages/core/src/core.ts#L67 I see there it should be projectId.
shivekkhurana
requested changes
Apr 25, 2024
src/status_im/config.cljs
Outdated
| "Status is a secure messaging app, crypto wallet, and Web3 browser built with state of the art technology." | ||
| :url "https://status.app" | ||
| :icons | ||
| ["https://res.cloudinary.com/dhgck7ebz/image/upload/f_auto,c_limit,w_1080,q_auto/Brand/Logo%20Section/Mark/Mark_01"]}) |
There was a problem hiding this comment.
Do we need to care about translations here ? Should we wrap description in (i18n) ?
src/status_im/config.cljs
Outdated
| "Status is a secure messaging app, crypto wallet, and Web3 browser built with state of the art technology." | ||
| :url "https://status.app" | ||
| :icons | ||
| ["https://res.cloudinary.com/dhgck7ebz/image/upload/f_auto,c_limit,w_1080,q_auto/Brand/Logo%20Section/Mark/Mark_01"]}) |
There was a problem hiding this comment.
Not a change, but is it possible to pass icon from disk rather than a link?
What if the infra team stops using Cloudinary tomorrow ?
|
|
||
| (defn- wallet-connect-core | ||
| [] | ||
| (Core. (clj->js {:project-id config/wallet-connect-project-id}))) |
There was a problem hiding this comment.
We can move the {:project-id ...} to a defonce with #js. clj->js is redundant because project id is static.
| [] | ||
| (let [core (wallet-connect-core) | ||
| metadata (clj->js config/default-wallet-connect-metadata)] | ||
| (promesa/create |
There was a problem hiding this comment.
Metadata can be moved to a (defonce metadata #js {...}).
It never changes so clj->js is redundant.
briansztamfater
force-pushed
the
chore/add-wallet-connect-package
branch
from
0c8ad7a
to
e223a4f
Compare
briansztamfater
force-pushed
the
chore/add-wallet-connect-package
branch
2 times, most recently
from
05f8ffd
to
2d95457
Compare
clauxx
approved these changes
Apr 30, 2024
There was a problem hiding this comment.
Thanks @briansztamfater! Looks good!
@siddarthkay is everything good dependency-wise here?
It was the last time I had looked at it. |
|
Hmm Indeed entries are missing from
@briansztamfater : please could you run |
|
@siddarthkay @jakubgs pushed changes from Also @shivekkhurana can you re-review? |
siddarthkay
reviewed
May 1, 2024
siddarthkay
force-pushed
the
chore/add-wallet-connect-package
branch
from
a9a21f3
to
0a2d615
Compare
|
I'm okay with this PR except for this comment: |
ilmotta
reviewed
May 3, 2024
| ["@walletconnect/web3wallet" :refer [Web3Wallet]] | ||
| [status-im.config :as config])) | ||
|
|
||
| (defonce ^:private wallet-connect-metadata |
There was a problem hiding this comment.
@briansztamfater @shivekkhurana defonce shouldn't be used in a case like this. As far as I've seen, defonce is basically a tool to help devs take control in the REPL of when side-effects should happen. A common example in the wild is to initialize global atoms, as in (defonce foo (atom :bar)).
There's no side-effect to evaluate wallet-connect-metadata (ignoring memory allocation could fail in a parallel universe), hence just def is sufficient. In fact, when I see defonce, I instantly search for the line of code with side-effects to understand why it's being used.
There was a problem hiding this comment.
Thanks for the feedback, I added translated strings, so probably defn is a better choice than def because language could change while the app is active 🤔 wdyt?
There was a problem hiding this comment.
Good question @briansztamfater. I have the impression our app doesn't react to system language changes and thus, requires a full restart. This means it's okay to use def and not worry about reactiveness. Not saying this is the best approach, but it's how I see our app behaving now.
Note: on emulated Android, when the whole system language changes, even after a full app restart it doesn't pick up the correct language and English is still seen everywhere. Maybe this is a problem deserving investigation in the near future.
clauxx
added
E:Mobile MVP Wallet Connect integration
briansztamfater
force-pushed
the
chore/add-wallet-connect-package
branch
from
8cb0e38
to
7f2a1c4
Compare
clauxx
approved these changes
May 6, 2024
There was a problem hiding this comment.
Looks good to me. Can be merged I guess as it doesn't affect any existing functionality 👍
@shivekkhurana can you give it a re-review when you have the chance?
jakubgs
approved these changes
May 6, 2024
ilmotta
approved these changes
May 6, 2024
|
@clauxx if Project ID was a key it would be called a Project Key. |
@jakubgs can't say about their naming consistency, but they advise hiding the "project keys" on the same page they introduce the Project ID, while "project keys" are not mentioned anywhere else in their documentation. Didn't look too much into it, but I'd rather be on the safer side and hide it. |
shivekkhurana
approved these changes
May 7, 2024
75% of end-end tests have passedFailed tests (11)Click to expandClass TestDeepLinksOneDevice:
Class TestWalletMultipleDevice:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestWalletOneDevice:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityMultipleDeviceMerged:
Class TestActivityMultipleDevicePR:
Expected to fail tests (2)Click to expandClass TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityOneDeviceMerged:
Passed tests (39)Click to expandClass TestCommunityMultipleDeviceMergedTwo:
Class TestActivityMultipleDevicePR:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityOneDeviceMerged:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityMultipleDeviceMerged:
Class TestActivityMultipleDevicePRTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
|
Signed-off-by: Brian Sztamfater <[email protected]>
briansztamfater
force-pushed
the
chore/add-wallet-connect-package
branch
from
c96818c
to
6d7e7f9
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.
This PR adds WalletConnect Web3Wallet library and required additional libraries as their documentation describes. Also defines
WALLET_CONNECT_PROJECT_IDkey for env var and a basic utils file to creating a Web3Wallet instance. Manual QA can be skipped, no new features nor bug fixes are included in this PR, we just need to make sure new packages don't make the app to crash.Also not sure if I am making correct use of promesa so feedback is appreciated.
status: ready