Skip to content
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.

Sign up for GitHub

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

Jump to bottom

connect - core in suite-desktop #14690

Merged
merged 11 commits into from
Nov 25, 2024
Merged

connect - core in suite-desktop #14690

merged 11 commits into from
Nov 25, 2024

Conversation

mroz22
Copy link
Contributor

@mroz22 mroz22 commented Oct 2, 2024 •
edited
Loading

So far only some preliminary research and exploration of replacing connect-popup with suite-desktop. My general idea is to merge this feature behind debug. The motivation is that there are similar UX problems to be solved for both core-in-suite-native and core-in-suite-desktop so it makes sense to me to have some playground available for both.

suite-desktop:

  • in dev or when --expose-connect-ws flag is on, suite-desktop opens websocket on 8090 and accepts connect calls here.
  • the 1st commit has support for inter-module inducted window-focus and window-blur. it probably needs some more polishing.

connect-web:

  • added new imp core-in-suite-desktop. what is a better name? core-in-ws-server?
  • this impl should later be available also from base connect package. And also from connect-webextension. I need to discuss this with @karliatto
image

@mroz22 mroz22 force-pushed the connect-desktop branch 2 times, most recently from 10a19b9 to 813585f Compare October 3, 2024 06:00
@mroz22 mroz22 mentioned this pull request Oct 3, 2024
Closed
@MiroslavProchazka MiroslavProchazka added the build-desktop This will trigger the build of desktop apps for your PR label Oct 3, 2024
@mroz22 mroz22 mentioned this pull request Oct 3, 2024
Draft
@Hannsek Hannsek added build-desktop This will trigger the build of desktop apps for your PR and removed build-desktop This will trigger the build of desktop apps for your PR labels Oct 7, 2024
@mroz22 mroz22 mentioned this pull request Oct 7, 2024
Closed
@mroz22 mroz22 force-pushed the connect-desktop branch 2 times, most recently from 6aa7d61 to 8e58f34 Compare October 29, 2024 07:58
@mroz22 mroz22 changed the title [wip] connect - core in suite-desktop [lets talk] connect - core in suite-desktop Oct 29, 2024
mroz22
mroz22 commented Oct 29, 2024
View reviewed changes
packages/suite-desktop-core/src/app.ts Outdated

mainThreadEmitter.on('blur-window', () => {
app.hide();
});
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

one preliminary question - is it the correct way to achieve inter-module focus-window and blur-window? if yes, then of course code duplication needs to be solved.

cc @marekrjpolak, @martykan

Copy link
Member

@martykan martykan Oct 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We already have handlers in window-controls that respond to ipcMain app/hide and app/focus

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

However it can only focus an existing window, not recreate it

@Hannsek Hannsek added build-desktop This will trigger the build of desktop apps for your PR and removed build-desktop This will trigger the build of desktop apps for your PR labels Nov 10, 2024
@github-actions GitHub Actions
Copy link

github-actions bot commented Nov 11, 2024 •
edited
Loading

🚀 Expo preview is ready!

  • Project → trezor-suite-preview
  • Platforms → android, ios
  • Scheme → trezorsuitelite
  • Runtime Version → 11
  • More info

Learn more about 𝝠 Expo Github Action

@martykan
Copy link
Member

Current state:

  • Suite runs Websocket server at http://localhost:2335/connect-ws
  • New Core mode in Connect (suite-desktop) that communicates with said WS
  • Modal that shows info about the method being called, needs to be confirmed every time
  • Security info about the process that created the connection + web origin header
Screenshot 2024-11-18 at 14 10 38

Missing features:

  • Support better device modals (refactor modal state)
    • ConfirmAddressModal
    • ConfirmXpubModal
    • TransactionReviewModal
  • Special methods with UI requests
    • getAccountInfo
      • SELECT_ACCOUNT
    • composeTransaction
      • SELECT_FEE, UPDATE_CUSTOM_FEE, INSUFFICIENT_FUNDS
  • Permissions handling
    • Keep using internal Connect logic - but need different settings → Second instance of connect? Different API
    • Or skip internal Connect logic and use wrapper?

@mroz22 mroz22 marked this pull request as ready for review November 21, 2024 14:42
@mroz22 mroz22 changed the title [lets talk] connect - core in suite-desktop connect - core in suite-desktop Nov 21, 2024
@martykan martykan force-pushed the connect-desktop branch 2 times, most recently from 2b772b3 to d0ee74b Compare November 22, 2024 09:25
mroz22
mroz22 commented Nov 25, 2024
View reviewed changes
Copy link
Contributor Author

@mroz22 mroz22 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦭 ✅

@martykan martykan merged commit cba164c into develop Nov 25, 2024
68 checks passed
@martykan martykan deleted the connect-desktop branch November 25, 2024 14:30
matejkriz added a commit that referenced this pull request Dec 16, 2024
@matejkriz
chore(ci): temporary workaround to fix EAS builds
6c358c1 
- #14690 introduced dependency on @trezor/suite-desktop-api in @suite-common/connect-init
- The ultimate solution would be to split that logic to a new package and remove the dependency on desktop-api from connect-init
@matejkriz matejkriz mentioned this pull request Dec 16, 2024
Merged
matejkriz added a commit that referenced this pull request Dec 17, 2024
@matejkriz
chore(ci): temporary workaround to fix EAS builds
a2d3765 
- #14690 introduced dependency on @trezor/suite-desktop-api in @suite-common/connect-init
- The ultimate solution would be to split that logic to a new package and remove the dependency on desktop-api from connect-init
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@karliatto karliatto karliatto left review comments

@martykan martykan martykan approved these changes

@szymonlesisz szymonlesisz Awaiting requested review from szymonlesisz szymonlesisz is a code owner

@marekrjpolak marekrjpolak Awaiting requested review from marekrjpolak marekrjpolak is a code owner

Assignees
No one assigned
Labels
build-desktop This will trigger the build of desktop apps for your PR
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@mroz22 @martykan @karliatto @Hannsek @MiroslavProchazka