-
Notifications
You must be signed in to change notification settings - Fork 988
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement Wallet Connect 1.0 #13032
Implement Wallet Connect 1.0 #13032
Conversation
Jenkins BuildsClick to see older builds (54)
|
94a8bf7
to
38b8891
Compare
e38e15f
to
9495755
Compare
amazing! is there a testflight link? |
@briansztamfater it is still WIP and not ready for review? |
da5f397
to
24f0ab3
Compare
3b9dc86
to
d56b35a
Compare
It is ready for review, just updated the PR description and status. However, it points to feat/wallet-connect-2.0 branch at the moment because it deeply depends on it, so I'd wait until 2.0 is merged before starting testing this one. Code review can be done though. Thanks!! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks good to me. Herculean work since it's a delicate topic. Awesome job!
96% of end-end tests have passed
Failed tests (3)Click to expand
Passed tests (80)Click to expand |
src/status_im/ui/screens/wallet_connect/session_proposal/views.cljs
Outdated
Show resolved
Hide resolved
src/status_im/ui/screens/wallet_connect/session_proposal/views.cljs
Outdated
Show resolved
Hide resolved
79% of end-end tests have passed
Failed tests (15)Click to expand
Passed tests (58)Click to expand
|
@briansztamfater @qoqobolo I'm fine with merging it if we are in a hurry, but there are still a few issues in the code which should be fixed. It might happen separately from PR if necessary. |
c0f5c57
to
1046cae
Compare
2871e48
to
d2b7665
Compare
@rasom pending comments now fixed |
@briansztamfater thank you |
@qoqobolo can you please make a final testing round to recheck I didn't break anything while fixing comments? :D |
81% of end-end tests have passed
Failed tests (30)Click to expand
Passed tests (125)Click to expand
|
60% of end-end tests have passed
Failed tests (2)Click to expand
Passed tests (3)Click to expand
|
Thanks @briansztamfater! Still looks good! :) |
7fb2a31
to
afb4f58
Compare
36d0fa9
to
9915d07
Compare
a20605e
to
bd7da02
Compare
afb4f58
to
466d441
Compare
Signed-off-by: Brian Sztamfater <brian@status.im>
466d441
to
a7b0764
Compare
yay! merged! wen AppStore? |
@caffeinum out already, you will have to enable it in advanced settings, since currently we haven't implemented revoking permissions on mobile (you'd have to do that from desktop), which is coming in the next release. |
fixes #12870
Summary
This PR integrates Wallet Connect 1.0 in addition to 2.0
Testing notes
Platforms
Functional
Steps to test
Steps should be the same as #12701 but with dapps that uses Wallet Connect 1.0
status: ready