-
Notifications
You must be signed in to change notification settings - Fork 79
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
feature(connector)_: Add model join to handle connected dApps #15954
Conversation
Jenkins BuildsClick to see older builds (32)
|
4ee6005
to
af8a916
Compare
✔️ status-desktop/prs/linux/x86_64/tests-nim/PR-15954#3 🔹 ~7 min 39 sec 🔹 af8a916 🔹 📦 tests/nim package |
af8a916
to
ad5242e
Compare
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.
Looks good
✔️ status-desktop/prs/linux/x86_64/tests-nim/PR-15954#6 🔹 ~6 min 59 sec 🔹 94e44d1 🔹 📦 tests/nim package |
94e44d1
to
58a90b9
Compare
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.
Looks fine!
58a90b9
to
5a51c2a
Compare
Note for testers, and QA : The current code is under a feature flag waiting for 1/ Status-go merge, 2/ QA testing. For those interested into testing the applicatio :
export FLAG_DAPPS_ENABLED=1
|
Performed testing again after merging status-go changes to make sure everything works as expected. Bumping status-go now. |
Hi @anastasiyaig could you be able to perform testing on this feature, instructions and information here : #15954 (comment) please 🙏🏾 |
* feature(connector)_: Add model join to handle connected dApps * Fix review comments * chore: bump status-go
* feature(connector)_: Add model join to handle connected dApps * Fix review comments * chore: bump status-go
My quick notes:
|
Could you post a screenshot for this point, I am actually confused what you mean by
Do you have some more information about the crash? any logs to share please? Is it reproducible or it's just a one off? Also any estimation about the amount of time when saying |
Thanks for bringing this is interesting perspective... 🙏🏾 If I understand what you meant here, the connections should obviously be filtered by account. I believe this should work unless there is a small tweak to make in the WC model fetching. @stefandunca do you confirm that this should work on WC or is there anything in progress? |
I am opening a new ticket, this PR is merged. let's move these on the other ticket |
On macOS, not logs to share atm. Try running it for more than ~1h, happens regularly. |
Indeed, it was implemented by @Seitseman here #15877 |
fixes #15885
Waits for status-im/status-go#5646
What does the PR do
This PR contains the changes to be able to see the list of connected DApps on the Status desktop for Connector service. It also contains changes to managed
DAppPermissionGranted
andDAppPermissionRevoked
Affected areas
Screenshot of functionality (including design for comparison)
Screencast.from.2024-08-02.01-51-36.webm
Impact on end user
Similar to other PR's this PR reuses some Wallet connect functionalities. I performed test to make sure Wallet Connect still works as expected
How to test
Please refer to the video to test. Please be aware that this PR requires setting the
FLAG_DAPPS_ENABLED
flag toenabled
since it enables seeing the list of DApps.Or use : https://app.uniswap.org/swap
Risk
Very low risk since Connector service duplicated some WC facilities which live isolated and would not create regression by definition.
Tick one: