Tenv e2e #17101
Tenv e2e #17101
Conversation
mroz22
requested review from
karliatto,
szymonlesisz,
martykan,
marekrjpolak and
vdovhanych
as code owners
|
Warning Rate limit exceeded@mroz22 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 2 minutes and 48 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (8)
WalkthroughThe changes introduce a new GitHub Actions workflow file named Additionally, an asynchronous self-invoking function is added to the e2e test suite, which connects to the Trezor environment, checks firmware configurations, and manages emulators for various firmware versions, ensuring proper teardown after testing. The Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
.github/workflows/test-trezor-user-env-link-e2e.yml (1)
1-39:⚠️ Potential issueFix GitHub Actions workflow syntax.
The workflow file has several syntax issues that need to be addressed:
- Missing "jobs" section.
- Missing group name in "concurrency" section.
- Incorrect placement of job definition.
Apply this diff to fix the workflow syntax:
name: "[Test] trezor-user-env-link e2e" on: pull_request: branches: - develop paths: - ".github/workflows/test-trezor-user-env-link-e2e.yml" - "packages/trezor-user-env/**" workflow_dispatch: concurrency: + group: ${{ github.workflow }}-${{ github.ref }} cancel-in-progress: true -e2e-test-trezor-user-env-link: - if: github.repository == 'trezor/trezor-suite' - runs-on: ubuntu-latest - steps: - - name: Checkout - uses: actions/checkout@v4 - with: - ref: ${{github.event.after}} - fetch-depth: 2 - - name: Setup node - uses: actions/setup-node@v4 - with: - node-version-file: ".nvmrc" - cache: yarn - - name: Install dependencies - run: | - echo -e "\nenableScripts: false" >> .yarnrc.yml - echo -e "\nenableHardenedMode: false" >> .yarnrc.yml - yarn workspaces focus @trezor/trezor-user-env-link - - - name: Run e2e tests - run: | - docker run ghcr.io/trezor/trezor-user-env:b23cde1860e03c8bdaebbbc8fcbcd2cc9d6375d3 -d - yarn workspace @trezor/trezor-user-env-link test:e2e +jobs: + e2e-test-trezor-user-env-link: + if: github.repository == 'trezor/trezor-suite' + runs-on: ubuntu-latest + steps: + - name: Checkout + uses: actions/checkout@v4 + with: + ref: ${{github.event.after}} + fetch-depth: 2 + - name: Setup node + uses: actions/setup-node@v4 + with: + node-version-file: ".nvmrc" + cache: yarn + - name: Install dependencies + run: | + echo -e "\nenableScripts: false" >> .yarnrc.yml + echo -e "\nenableHardenedMode: false" >> .yarnrc.yml + yarn workspaces focus @trezor/trezor-user-env-link + + - name: Run e2e tests + run: | + docker run ghcr.io/trezor/trezor-user-env:b23cde1860e03c8bdaebbbc8fcbcd2cc9d6375d3 -d + yarn workspace @trezor/trezor-user-env-link test:e2eAdditionally, consider these improvements:
- Add error handling for the Docker container startup.
- Add a step to wait for the container to be ready.
- Add a cleanup step to stop the container after tests.
Would you like me to provide an improved version of the workflow with these additional features?
🧰 Tools
🪛 actionlint (1.7.4)
1-1: "jobs" section is missing in workflow
(syntax-check)
12-12: group name is missing in "concurrency" section
(syntax-check)
15-15: unexpected key "e2e-test-trezor-user-env-link" for "workflow" section. expected one of "concurrency", "defaults", "env", "jobs", "name", "on", "permissions", "run-name"
(syntax-check)
🧹 Nitpick comments (1)
packages/utils/src/typedObjectKeys.ts (1)
1-2: LGTM! Consider adding documentation.The implementation is correct and provides good type safety. Consider adding JSDoc documentation with examples to help users understand the utility's purpose and usage.
+/** + * Type-safe wrapper around Object.keys() that preserves the key types of the input object. + * @example + * const obj = { foo: 1, bar: 'hello' }; + * const keys = typedObjectKeys(obj); // Array<'foo' | 'bar'> + */ export const typedObjectKeys = <T extends Record<any, any>>(obj: T): Array<keyof T> => Object.keys(obj) as Array<keyof T>;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (8)
.github/workflows/test-trezor-user-env-link-e2e.yml(1 hunks)packages/connect/src/device/DeviceList.ts(1 hunks)packages/connect/src/types/utils.ts(0 hunks)packages/transport/src/sessions/background.ts(1 hunks)packages/trezor-user-env-link/e2e/api.test.ts(1 hunks)packages/trezor-user-env-link/package.json(2 hunks)packages/utils/src/index.ts(1 hunks)packages/utils/src/typedObjectKeys.ts(1 hunks)
💤 Files with no reviewable changes (1)
- packages/connect/src/types/utils.ts
✅ Files skipped from review due to trivial changes (2)
- packages/transport/src/sessions/background.ts
- packages/connect/src/device/DeviceList.ts
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/test-trezor-user-env-link-e2e.yml
1-1: "jobs" section is missing in workflow
(syntax-check)
12-12: group name is missing in "concurrency" section
(syntax-check)
15-15: unexpected key "e2e-test-trezor-user-env-link" for "workflow" section. expected one of "concurrency", "defaults", "env", "jobs", "name", "on", "permissions", "run-name"
(syntax-check)
🔇 Additional comments (2)
packages/utils/src/index.ts (1)
54-54: LGTM!The export is correctly placed in alphabetical order and follows the consistent export pattern used throughout the file.
packages/trezor-user-env-link/package.json (1)
9-10:✅ Verification successful
Verify ts-node version compatibility.
The ts-node version and configuration look good. However, let's verify it's the latest compatible version.
Also applies to: 19-20
🌐 Web query:
What is the latest version of ts-node that is compatible with TypeScript 5.x?💡 Result:
The latest version of ts-node compatible with TypeScript 5.x depends on your project configuration:
General Compatibility:
- ts-node v10+ works with TypeScript ≥4.x, including TypeScript ≥5.x, as long as your project installs both
ts-nodeandtypescript@^5locally[2][9].- Example setup:
npm install --save-dev typescript@^5 npm install --save-dev ts-node@^10ESM Support:
For projects using native ECMAScript modules ("type": "module"), ensure:
- Use
tsx,node --loader=tsx/esm, orts-node-esm[3][9].- Enable
verbatimModuleSyntaxintsconfig.jsonif importing.tsfiles directly[3].Node.js Version:
- TypeScript ≥5 requires Node.js ≥14 due to ES2020 runtime dependencies[1][6].
For optimal performance and feature support with TypeScript ≥5, update both
typescriptandts-nodeto their latest stable releases[8][9].Citations:
- 1: https://www.typescriptlang.org/docs/handbook/release-notes/typescript-5-1.html
- 2: https://github.com/TypeStrong/ts-node
- 3: TS 5.x and ESM does not work with
verbatimModuleSyntaxTypeStrong/ts-node#1987- 4: https://angular.dev/reference/versions
- 5: https://devblogs.microsoft.com/typescript/announcing-typescript-5-0/
- 6: https://javascript-conference.com/blog/typescript-5/
- 7: https://www.bennadel.com/blog/3268-experimenting-with-ts-node-and-using-typescript-in-node-js-on-the-server.htm
- 8: https://www.npmjs.com/package/typescript
- 9: https://www.npmjs.com/ts-node
- 10: https://www.typescriptlang.org/docs/handbook/modules/guides/choosing-compiler-options.html
Ensure ts-node is up-to-date for TypeScript 5.x compatibility
The current configuration—with the
ts-nodecommand using the-Tflag—appears correct as long as your project is using ts-node version 10 or later. As the web research indicates, ts-node v10+ is compatible with TypeScript 5.x when both are installed locally. Please double-check that your dev dependency is updated accordingly (for example, vianpm install --save-dev ts-node@^10and ensuring TypeScript is at^5.x). This verification also applies to lines 19-20.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
.github/workflows/test-trezor-user-env-link-e2e.yml (1)
36-38: Parameterize Docker Image Version
Hardcoding the Docker image hash may reduce flexibility for future updates. Consider parameterizing or using a variable for the Docker image version to simplify maintenance.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (8)
.github/workflows/test-trezor-user-env-link-e2e.yml(1 hunks)packages/connect/src/device/DeviceList.ts(1 hunks)packages/connect/src/types/utils.ts(0 hunks)packages/transport/src/sessions/background.ts(1 hunks)packages/trezor-user-env-link/e2e/api.test.ts(1 hunks)packages/trezor-user-env-link/package.json(1 hunks)packages/utils/src/index.ts(1 hunks)packages/utils/src/typedObjectKeys.ts(1 hunks)
💤 Files with no reviewable changes (1)
- packages/connect/src/types/utils.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- packages/utils/src/index.ts
- packages/utils/src/typedObjectKeys.ts
- packages/trezor-user-env-link/package.json
- packages/transport/src/sessions/background.ts
- packages/connect/src/device/DeviceList.ts
- packages/trezor-user-env-link/e2e/api.test.ts
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/test-trezor-user-env-link-e2e.yml
1-1: "jobs" section is missing in workflow
(syntax-check)
12-12: group name is missing in "concurrency" section
(syntax-check)
15-15: unexpected key "e2e-test-trezor-user-env-link" for "workflow" section. expected one of "concurrency", "defaults", "env", "jobs", "name", "on", "permissions", "run-name"
(syntax-check)
⏰ Context from checks skipped due to timeout of 90000ms (21)
- GitHub Check: PR-check / node-authorizeCoinjoin cancelCoinjoinAuthorization passphrase unlockPath setBusy checkFirmwareAuthenticity keepSession cancel.test info.test resetDevice-api
- GitHub Check: PR-check / web-authorizeCoinjoin cancelCoinjoinAuthorization passphrase unlockPath setBusy checkFirmwareAuthenticity keepSession cancel.test info.test resetDevice-api
- GitHub Check: PR-check / web-override init-api-flaky
- GitHub Check: PR-check / node-override init-api-flaky
- GitHub Check: build-deploy
- GitHub Check: build-deploy
- GitHub Check: run-desktop-tests (@group=wallet, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: run-desktop-tests (@group=other, trezor-user-env-unix)
- GitHub Check: EAS Update
- GitHub Check: run-desktop-tests (@group=passphrase, trezor-user-env-unix)
- GitHub Check: transport-e2e-test
- GitHub Check: run-desktop-tests (@group=settings, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: Analyze with CodeQL (javascript)
- GitHub Check: build-web
- GitHub Check: run-desktop-tests (@group=device-management, trezor-user-env-unix)
- GitHub Check: test
- GitHub Check: Setup and Cache Dependencies
- GitHub Check: run-desktop-tests (@group=suite, trezor-user-env-unix)
- GitHub Check: prepare_android_test_app
- GitHub Check: build-web
- GitHub Check: Socket Security: Pull Request Alerts
|
🚀 Expo preview is ready!
|
mroz22
added
the
no-project
mroz22
force-pushed
the
tenv-e2e
branch
4 times, most recently
from
ab602ff to
8fdd2ac
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (4)
.github/workflows/test-trezor-user-env-link-e2e.yml(1 hunks)packages/trezor-user-env-link/e2e/api.test.ts(1 hunks)packages/trezor-user-env-link/package.json(1 hunks)packages/trezor-user-env-link/src/api.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/trezor-user-env-link/e2e/api.test.ts
- packages/trezor-user-env-link/package.json
- .github/workflows/test-trezor-user-env-link-e2e.yml
⏰ Context from checks skipped due to timeout of 90000ms (20)
- GitHub Check: Type Checking
- GitHub Check: Releases revision Checks
- GitHub Check: Other Checks
- GitHub Check: Linting and formatting
- GitHub Check: Build libs for publishing
- GitHub Check: Unit Tests
- GitHub Check: PR-check / node-authorizeCoinjoin cancelCoinjoinAuthorization passphrase unlockPath setBusy checkFirmwareAuthenticity keepSession cancel.test info.test resetDevice-api
- GitHub Check: PR-check / node-override init-api-flaky
- GitHub Check: PR-check / web-override init-api-flaky
- GitHub Check: PR-check / web-authorizeCoinjoin cancelCoinjoinAuthorization passphrase unlockPath setBusy checkFirmwareAuthenticity keepSession cancel.test info.test resetDevice-api
- GitHub Check: build-deploy
- GitHub Check: build-deploy
- GitHub Check: EAS Update
- GitHub Check: build-web
- GitHub Check: Analyze with CodeQL (javascript)
- GitHub Check: build-web
- GitHub Check: transport-e2e-test
- GitHub Check: e2e-test-trezor-user-env-link
- GitHub Check: test
- GitHub Check: Socket Security: Pull Request Alerts
🔇 Additional comments (1)
packages/trezor-user-env-link/src/api.ts (1)
302-306:✅ Verification successful
Verify usage of allowUnsafePaths in tests.
Since this method has security implications, let's verify its usage is limited to test environments.
🏁 Script executed:
#!/bin/bash # Description: Check if allowUnsafePaths is only used in test files # Search for allowUnsafePaths usage echo "Searching for allowUnsafePaths usage..." rg "allowUnsafePaths" --type tsLength of output: 294
Security method usage verified: The verification confirms that
allowUnsafePathsis only called from test files (specifically inpackages/trezor-user-env-link/e2e/api.test.ts). No production usage was detected, so the current implementation aligns with the security guidelines outlined in the review comment.
| await TrezorUserEnvLink.setupEmu(); | ||
|
|
||
| await TrezorUserEnvLink.allowUnsafePaths(); | ||
| // todo: run more complex commands, typically those that require interaction with multiple screens (dry run..) |
There was a problem hiding this comment.
I need to get device somehow into recovery/backup state. Can we do it with current tenv API? I can't see anything. If no, could you please either expose some trezorlib method which allows this, or maybe if no such method exists I would be ok with passing raw bytes. cc @grdddj (no hurry here)
There was a problem hiding this comment.
I have added emulator-set-for-backup method into the websocket API. It requires the bridge to be running - otherwise it cannot work properly
mroz22
force-pushed
the
tenv-e2e
branch
3 times, most recently
from
8815efd to
fd763d8
Compare
would be nice to have something to prove that trezor-user-env-link to trezor-user-env server integration works as expected. this is mostly intended to run on tenv image updates so that we can tell where potential failures come from.