Skip to content

Feat/thp transport prerequisite #19122

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

Merged
merged 4 commits into from
Jun 4, 2025
Merged

Conversation

szymonlesisz
Copy link
Contributor

@szymonlesisz szymonlesisz commented May 23, 2025

Description

based on #19073, ignore first two commits

  1. feat(protocol): add THP protobuf definitions and types
    protobuf messages are not merged in the firmware repo yet but i think we can have them here, im sure they will not change significantly.
    The comment claims it was build by protobuf script which is missing in here, i will add it later once protobuf is merged into main to avoid protobuf build problems

  2. add ThpState and ThpMessageResponse to transport api

  • ThpState as params of call/send/receive functions
  • expose union of protobuf Messages.MessageResponse + ThpMessageResponse (used by @trezor/connect typed call)
  1. feat(transport-bridge) accept protocol-v2 (THP) messages
    Just a preparation for the bridge to accept additional params (protocol)

  2. feat(connect): add ThpState and ThpMessageResponse to DeviceCurrentSession

  • use protocol from this.device, the value is dynamic and could be changed after DeviceCurrentSession is created
  • add thpState to transport call/send/receive
  • use MessageResponse (union of protobuf and thp) instead of Messages.MessageResponse (protobuf only)

🔍🖥️ Suite web test results: View in Currents

🔍🖥️ Suite desktop test results: View in Currents

🔍🖥️ Suite native android test results: View in Currents

@szymonlesisz szymonlesisz added connect Connect API related (ie. fee calculation) no-project This label is used to specify that PR doesn't need to be added to a project cherry-pick🍒 labels May 23, 2025
@szymonlesisz szymonlesisz force-pushed the feat/thp-transport-prerequisite branch from 312effa to 3ae03c6 Compare May 23, 2025 12:54
@trezor-bot
Copy link

trezor-bot bot commented May 23, 2025

✅ Previously successful run of [Test] PR Suite Desktop e2e tests workflow has been found.
⏭️ Skipping tests for this run.
💡 If you are unsure about your latest changes, please rerun the workflow manually. (Use the Re-run all jobs option)

@szymonlesisz szymonlesisz force-pushed the feat/thp-transport-prerequisite branch from 3ae03c6 to 830a081 Compare May 23, 2025 12:58
@trezor-bot
Copy link

trezor-bot bot commented May 23, 2025

✅ Previously successful run of [Test] PR Suite Desktop e2e tests workflow has been found.
⏭️ Skipping tests for this run.
💡 If you are unsure about your latest changes, please rerun the workflow manually. (Use the Re-run all jobs option)

@szymonlesisz szymonlesisz force-pushed the feat/thp-transport-prerequisite branch from 830a081 to 68888c9 Compare May 27, 2025 12:05
@trezor-bot
Copy link

trezor-bot bot commented May 27, 2025

✅ Previously successful run of [Test] PR Suite Desktop e2e tests workflow has been found.
⏭️ Skipping tests for this run.
💡 If you are unsure about your latest changes, please rerun the workflow manually. (Use the Re-run all jobs option)

Comment on lines +125 to +136
private _protocol: TransportProtocol;
public get protocol() {
return this._protocol;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

btw is this better then simply accessing public readonly protocol?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

idk. i had it as public then i decided to adjust to the rest of the code

Comment on lines 433 to 434
} else if (wasUnacquired && this.isUnacquired() && this.thp?.properties) {
this.emitLifecycle(DEVICE.CONNECT_UNACQUIRED);
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be nice if @marekrjpolak could check this line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

actually thanks for noticing, it does not belong to this PR i will add it later with some more context to it

- ThpState as params of send/receive/call functions
- union of protobuf Messages.MessageResponse + ThpMessageResponse;
…ntSession

- use `protocol` from this.device, the value is dynamic and could be changed after DeviceCurrentSession is created
- add `thpState` to transport call/send/receive
@szymonlesisz szymonlesisz force-pushed the feat/thp-transport-prerequisite branch from 68888c9 to 0c80213 Compare June 4, 2025 11:39
@trezor-bot
Copy link

trezor-bot bot commented Jun 4, 2025

✅ Previously successful run of [Test] PR Suite Desktop e2e tests workflow has been found.
⏭️ Skipping tests for this run.
💡 If you are unsure about your latest changes, please rerun the workflow manually. (Use the Re-run all jobs option)

@trezor-bot
Copy link

trezor-bot bot commented Jun 4, 2025

✅ Previously successful run of [Test] PR Suite Web e2e tests workflow has been found.
⏭️ Skipping tests for this run.
💡 If you are unsure about your latest changes, please rerun the workflow manually. (Use the Re-run all jobs option)

@szymonlesisz szymonlesisz merged commit 5919fe9 into develop Jun 4, 2025
67 checks passed
@szymonlesisz szymonlesisz deleted the feat/thp-transport-prerequisite branch June 4, 2025 18:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cherry-pick🍒 connect Connect API related (ie. fee calculation) no-project This label is used to specify that PR doesn't need to be added to a project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants