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

Feat/websocket-client package #15797

Open
wants to merge 5 commits into
base: develop
Choose a base branch
from
Open

Feat/websocket-client package #15797

wants to merge 5 commits into from
+422 −437

Conversation

szymonlesisz
Copy link
Contributor

@szymonlesisz szymonlesisz commented Dec 5, 2024 •
edited
Loading

Description

Side quest from bluetooth

Create and use shared websocket package.
This is basically code moved from blockchain-link/baseWebsocket file to standalone package, so the logic behind connection/ping/timeouts and sending/receiving messages can be used elsewhere

This should be used in:

  • blockchain-link (in this PR)
  • trezor-user-env-link (in this PR)
  • transport-bluetooth (in bluetooth branch )
  • anywhere else?

Additional benefit:
webpack configs doesnt need to use webpack.NormalModuleReplacementPlugin on ws module

Additional fixes:

  • ignore eslinting .cache directory (if you run playwright locally there are some .js builds in there)
  • fix CustomError in blockchain-link, there are few places where the error message is passed as code param like: 
    if (!hex) throw new CustomError(`Missing hex of ${request.payload}`);

// -> the result: Error({ code: 'blockchain_link/', message: '' })
// -> after the fix: Error({ code: undefined, message: 'Missing hex...' })

@github-actions GitHub Actions
Copy link

github-actions bot commented Dec 5, 2024 •
edited
Loading

🚀 Expo preview is ready!

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

Learn more about 𝝠 Expo Github Action

@szymonlesisz szymonlesisz force-pushed the feat/websocket-package branch 2 times, most recently from a0fc770 to 221cfac Compare December 9, 2024 10:37
@szymonlesisz szymonlesisz marked this pull request as ready for review December 9, 2024 10:59
@szymonlesisz szymonlesisz force-pushed the feat/websocket-package branch 2 times, most recently from 15ffec4 to 42604de Compare December 9, 2024 16:33
@szymonlesisz
Copy link
Contributor Author

last minute changes.

new package renamed to @trezor/websocket-client

@szymonlesisz szymonlesisz changed the title Feat/websocket package Feat/websocket-client package Dec 9, 2024
@szymonlesisz szymonlesisz force-pushed the feat/websocket-package branch from 42604de to 6412ddb Compare December 11, 2024 15:32
@szymonlesisz
Copy link
Contributor Author

/rebase

@github-actions GitHub Actions
Copy link

Start rebasing: https://github.com/trezor/trezor-suite/actions/runs/12297127542

@trezor-ci trezor-ci force-pushed the feat/websocket-package branch from d56c090 to 1824969 Compare December 12, 2024 13:12
marekrjpolak
marekrjpolak approved these changes Dec 13, 2024
View reviewed changes
Copy link
Contributor

@marekrjpolak marekrjpolak left a comment

Choose a reason for hiding this comment

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

I like it.

packages/websocket-client/src/client.ts
Comment on lines +57 to +62
if (url.startsWith('https')) {
url = url.replace('https', 'wss');
}
if (url.startsWith('http')) {
url = url.replace('http', 'ws');
}
Copy link
Contributor

Choose a reason for hiding this comment

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

total nitpick: the second one covers the first one as well

packages/blockchain-link/src/workers/baseWebsocket.ts
}

protected onMessage(message: string) {
protected onMessage(message: WebsocketResponse) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably not very important, but these overriden onMessage don't call setPingTimeout as the base one does, so the pings will be timed differently.

@szymonlesisz
Copy link
Contributor Author

im not sure if failing android e2e are somehow related to this change or not?

@mroz22
Copy link
Contributor

mroz22 commented Dec 17, 2024

/rebase

@github-actions GitHub Actions
Copy link

Start rebasing: https://github.com/trezor/trezor-suite/actions/runs/12368290749

@trezor-ci trezor-ci force-pushed the feat/websocket-package branch from 1824969 to 1d3f760 Compare December 17, 2024 07:26
@mroz22
Copy link
Contributor

mroz22 commented Dec 17, 2024 •
edited
Loading

android tests look like they have hanged for 40 minutes albeit finishing correctly
image

and it is probably somehow related. nightly scheduled test finished successfully just 8 hours ago. Here is the list of recently finished android tests - https://github.com/trezor/trezor-suite/actions/workflows/test-suite-native-e2e-android.yml

@mroz22
Copy link
Contributor

mroz22 commented Jan 20, 2025

lets try rebasing it and see whether android tests pass or not

@mroz22
Copy link
Contributor

mroz22 commented Jan 20, 2025

/rebase

@github-actions GitHub Actions
Copy link

Start rebasing: https://github.com/trezor/trezor-suite/actions/runs/12862656946

@trezor-ci trezor-ci force-pushed the feat/websocket-package branch from 1d3f760 to f693b8c Compare January 20, 2025 07:00
@mroz22 mroz22 added the no-project This label is used to specify that PR doesn't need to be added to a project label Jan 20, 2025
@mroz22
Copy link
Contributor

mroz22 commented Jan 20, 2025

android tests still timing out - https://github.com/trezor/trezor-suite/actions/runs/12862670578/job/35858213762?pr=15797#step:11:403

something has changed in the unified websocket implementation for suite-native apparently

@matejkriz matejkriz requested a review from a team as a code owner January 21, 2025 15:29
@PeKne
Copy link
Contributor

PeKne commented Jan 21, 2025 •
edited
Loading

@mroz22, there is probably hanging synchronization with the new WebSocket implementation. Detox E2E tests synchronize with the network events and pause the test until the communication is closed (if you do not set it differently on purpose). More here. All the tests pass, but the connection stays open so the test never ends and it times out.

I can look at the tests tomorrow if you need help. We have encountered this issue multiple times on mobile.

@matejkriz matejkriz force-pushed the feat/websocket-package branch from 69c5d49 to fbd5a15 Compare January 22, 2025 16:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@marekrjpolak marekrjpolak marekrjpolak approved these changes

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

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

Assignees
No one assigned
Labels
no-project This label is used to specify that PR doesn't need to be added to a project
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@szymonlesisz @mroz22 @PeKne @marekrjpolak