feat(ui): Introduce experimental walletconnect #952
Conversation
There was a problem hiding this comment.
As discussed with @NunoAlexandre on IRC: I would prefer if we leave this in a feature branch until we have a clearer plan on how the functionality will look like and it is a bit more polished.
I fear that the added clutter of warnings might prevent us from detecting actual problems in future pull-requests if this gets merged.
(!) Circular dependencies
node_modules/hash-base/node_modules/readable-stream/lib/_stream_readable.js -> node_modules/hash-base/node_modules/readable-stream/lib/_stream_duplex.js -> node_modules/hash-base/node_modules/readable-stream/lib/_stream_readable.js
node_modules/hash-base/node_modules/readable-stream/lib/_stream_duplex.js -> node_modules/hash-base/node_modules/readable-stream/lib/_stream_writable.js -> node_modules/hash-base/node_modules/readable-stream/lib/_stream_duplex.js
node_modules/hash-base/node_modules/readable-stream/lib/_stream_duplex.js -> node_modules/hash-base/node_modules/readable-stream/lib/_stream_writable.js -> /Users/rudolfs/work/radicle-upstream-2/node_modules/hash-base/node_modules/readable-stream/lib/_stream_duplex.js?commonjs-proxy -> node_modules/hash-base/node_modules/readable-stream/lib/_stream_duplex.js
...and 13 more
(!) `this` has been rewritten to `undefined`
https://rollupjs.org/guide/en/#error-this-is-undefined
node_modules/@ethersproject/properties/lib.esm/index.js
1: "use strict";
2: var __awaiter = (this && this.__awaiter) || function (thisArg, _arguments, P, generator) {
^
3: function adopt(value) { return value instanceof P ? value : new P(function (resolve) { resolve(value); }); }
4: return new (P || (P = Promise))(function (resolve, reject) {
...and 1 other occurrence
(!) Plugin inject: rollup-plugin-inject: failed to parse /Users/rudolfs/work/radicle-upstream-2/node_modules/eccrypto-js/node_modules/secp256k1/lib/messages.json. Consider restricting the plugin to particular files via options.include
node_modules/eccrypto-js/node_modules/secp256k1/lib/messages.json
| "marked": "^1.1.1", | ||
| "pure-svg-code": "^1.0.6", | ||
| "radicle-contracts": "https://buildkite.com/organizations/monadic/pipelines/radicle-contracts/builds/72/jobs/e036d8ff-a6e5-4bf4-9d63-fd02bd297b2a/artifacts/b2ac2a23-6183-4219-ac35-1e36c444f7b2", |
There was a problem hiding this comment.
Is there a plan to publish the contracts somewhere in a stable location? I'm not sure how long these build artefacts are kept around...
There was a problem hiding this comment.
Not for now, but this is a stable location for the period we are concerned with. We can’t point to the GitHub project directly (i.e. yarn add https://github.com/radicle-dev/radicle-contracts.git) because we need to create build artefacts (compile TS and the contracts) and yarn doesn't do this for git dependencies.
There was a problem hiding this comment.
We should definitely publish an npm package though soon.
There was a problem hiding this comment.
Installing from the git dependency should be possible with this yarnpkg/yarn#5235 (comment) in mind.
There was a problem hiding this comment.
Ah! Does that mean that if we had a .npmignore file to the Radicle Contracts that pointing to the git project should work?
There was a problem hiding this comment.
AFAIU yes as the issue seems to be that yarn cleans up the build artefacts.
There was a problem hiding this comment.
will give it a shot
There was a problem hiding this comment.
gave it a try but it doesn't work. the issue being that Yarn does not run prepack when installing Git dependencies
|
|
||
| async function main() { | ||
| const provider = new ethers.providers.JsonRpcProvider( | ||
| "http://localhost:8545" |
There was a problem hiding this comment.
Will this be user configurable at some point? Or will it always be hardcoded?
For hardcoded configuration variables we have src/config.ts, for everything user configurable we write the initial value to a kv store on disk and allow users to modify it later in the Settings screen.
There was a problem hiding this comment.
I think that in the future this should point to the mainnet instance.
There was a problem hiding this comment.
Isn't the script installing dev contracsts? Maybe some documentation would help explaining the utility of this script and what the expected workflow should look like. A lot of maintainers won't have much context to these changes otherwise.
There was a problem hiding this comment.
Added a yarn command as suggested by @rudolfs and a corresponding (minimal) documentation on DEVELOPMENT.md. I'd prefer to not be more extensive now or else we might be documenting stuff that will change soon enough.
NunoAlexandre
force-pushed
the
walletconnect
branch
from
9981a24
to
74442ec
Compare
Yes let's do that. Created |
|
Looks awesome so far 💯 ! In the interest of keeping things moving and not letting this feature branch diverge from master for too long, maybe we can aim to merge this in once the console error concerns are figured out. |
That would be fine by me. |
| "marked": "^1.1.1", | ||
| "pure-svg-code": "^1.0.6", | ||
| "radicle-contracts": "https://buildkite.com/organizations/monadic/pipelines/radicle-contracts/builds/72/jobs/e036d8ff-a6e5-4bf4-9d63-fd02bd297b2a/artifacts/b2ac2a23-6183-4219-ac35-1e36c444f7b2", |
There was a problem hiding this comment.
Installing from the git dependency should be possible with this yarnpkg/yarn#5235 (comment) in mind.
| inject({ | ||
| modules: { | ||
| process: "_process", | ||
| Buffer: ["buffer", "Buffer"], |
There was a problem hiding this comment.
Is this something one of the new dependencies needs, or is used by changes introduced in this PR?
There was a problem hiding this comment.
This was added by Thomas fyi, and I believe that yes, it's necessary by some of the new dependencies. Will double-check.
|
|
||
| async function main() { | ||
| const provider = new ethers.providers.JsonRpcProvider( | ||
| "http://localhost:8545" |
There was a problem hiding this comment.
Isn't the script installing dev contracsts? Maybe some documentation would help explaining the utility of this script and what the expected workflow should look like. A lot of maintainers won't have much context to these changes otherwise.
| "exclude": ["native/*", "node_modules/*", "public/*"], | ||
| "compilerOptions": { | ||
| "skipLibCheck": true, |
There was a problem hiding this comment.
Could you explain why this is a good config change for upstream?
There was a problem hiding this comment.
Same as #952 (comment)
|
|
||
| type State = | ||
| | { status: Status.NotConnected } | ||
| | { status: Status.Connected; connected: Connected }; |
There was a problem hiding this comment.
There is a lot of connecting in this pr. 😆 It would be helpful to give at leas this field and type a different name.
There was a problem hiding this comment.
do you have a suggestion? thought the same but name it Wallet doesn't help and could come up with a better name.
|
I tried to connect my mobile Metamask wallet. The connection seemed to work, but I don't see my balance, do I need to run some daemon locally (if so, how do I set that up)?
|
ah, that is right, sorry! This should answer all your getting started needs: https://www.notion.so/monadic/Spike-Walletconnect-on-Upstream-18a27328098845b091f25bf0b429a375 |
Co-Authored-By: geigerzaehler
Co-authored-by: Rūdolfs Ošiņš <rudolfs@osins.org>
Co-authored-by: Rūdolfs Ošiņš <rudolfs@osins.org>
Co-authored-by: Alexander Simmerl <a.simmerl@gmail.com>
This reverts commit f937c17. We actually need ethers in ./scripts/deploy-dev-contracts.js
NunoAlexandre
force-pushed
the
walletconnect
branch
from
4106aa6
to
c1116d1
Compare
|
Superseded by #974 @geigerzaehler leave it up to you to close it since there are open discussions that'd be better addressed by you 👍 |
Closes #940
In this branch, we introduce an experimental feature where we use
walletconnectto connect an Eth wallet to the Upstream. This experiment lives under theWalletsection for now and allows us toconnectby scanning a QR Code using a compatible wallet. We also introduce a dependency on the Radicle Contracts and a script to deploy them to a local blockchain instance.When building/running, we see js warnings which I don't yet know how to solve which does not get in the way.
I am also seeing an issue with disconnecting from a connected wallet, where it seems that the underlying components don't quite expect to disconnect and a consecutive
connectiattempt does nothing. This is ok for now and can be walked around by switching tabs, which creates a fresh wallet.To give it a try, go to
Wallet>Connect, use your preferred wallet and scan the QR Code the app shows. Once you approve the connection on your wallet, you should see your balance and address in the app. Try switching accounts from your wallet app to see how it reflects in the app.