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.

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

Add missing capabilities #3966

Closed
komret opened this issue Jun 20, 2024 · 15 comments
Closed

Add missing capabilities #3966

komret opened this issue Jun 20, 2024 · 15 comments
Labels
code Code improvements

Comments

@komret
Copy link

komret commented Jun 20, 2024

I miss some properties among the device capabilities when working on Suite, which leads to hard-coding device models accross the code base and worse maintainability. I would appreciate adding the following flags among device capabilities. @trezor/connect will handle backwards compatibility. The names are just my suggestions, the list might not be exhaustive.

Capability_SecureElement: device has a secure element and an ability to check device authenticity; hardcoded e.g. here
Capability_Tutorial: device has controls tutorials; hardcoded here
Capability_TouchScreen: device has a touchscreen as opposed to buttons, hardcoded e.g. here
Capability_DisplayRotation: device supports display rotation, hardcoded here

@komret komret added the code Code improvements label Jun 20, 2024
@matejcik
Copy link
Contributor

Capability_SecureElement: device has a secure element and an ability to check device authenticity; hardcoded e.g. here
Capability_Tutorial: device has controls tutorials; hardcoded here

presumably all future devices will support these two. we could even do a tutorial for TT, fwiw :)

Capability_TouchScreen: device has a touchscreen as opposed to buttons, hardcoded e.g. here

this is not a capability and in this particular place it just might happen that T3W1 will need different string.
(i'm afraid this needs a per-model entry, where some texts happen to be identical. or, you know, you can switch on internal_model[2] where T is "touch" and B is "button" :)) )

Capability_DisplayRotation: device supports display rotation, hardcoded here

this is plausibly a capability

@komret
Copy link
Author

komret commented Jun 20, 2024

Capability_SecureElement: device has a secure element and an ability to check device authenticity; hardcoded e.g. here
Capability_Tutorial: device has controls tutorials; hardcoded here

presumably all future devices will support these two. we could even do a tutorial for TT, fwiw :)

So in cases like this you're not adding a capability flag and we catch it in Suite? Acceptable.

Capability_TouchScreen: device has a touchscreen as opposed to buttons, hardcoded e.g. here

this is not a capability and in this particular place it just might happen that T3W1 will need different string. (i'm afraid this needs a per-model entry, where some texts happen to be identical. or, you know, you can switch on internal_model[2] where T is "touch" and B is "button" :)) )

Good idea.

@komret
Copy link
Author

komret commented Jun 20, 2024

Are you sure TS5 has a tutorial? If it does, Suite skips it.

@matejcik
Copy link
Contributor

Are you sure TS5 has a tutorial?

#3829
i assumed that it is already done, but it's not.
it is planned for the TS5 public release though

@komret
Copy link
Author

komret commented Jun 20, 2024

😬 This complicates the condition needed in Suite. A flag for Capability_Tutorial would be helpful if a situation like this arises in the future.

@Hannsek
Copy link
Contributor

Hannsek commented Jul 11, 2024

@komret can this be closed?

@komret
Copy link
Author

komret commented Jul 12, 2024

@Hannsek No, Capability_DisplayRotation should be added. I'd like to see Capability_Tutorial (to avoid code like this) and Capability_SecureElement as well.

@matejcik
Copy link
Contributor

let's not add Tutorial capability and instead assume that it's there. it seems wasteful to do conditional code for a single version of firmware that the vast majority of TS5 owners won't ever install

i was thinking about the Rotation capability and it's actually not good as a single-bit capability. All trezor-core models support display orientation, it's just that TS3 only has North and South while TT/TS5 has all four cardinal directions (because square display)
(a hypothetical circular Trezor Safe 11 could have 360 degrees of display rotation :) )

If we really want to report this from firmware, we could make:
(a) an enum: None, Vertical, Full
(b) bit field representing 0, 90, 180, 270
(c) list of allowable values

@komret
Copy link
Author

komret commented Jul 12, 2024

let's not add Tutorial capability and instead assume that it's there. it seems wasteful to do conditional code for a single version of firmware that the vast majority of TS5 owners won't ever install

It is still and official firmware users can download, and although it is an edge case, I want to avoid a possibility of a user ending up with a blank screen in Suite if they do onboarding on 2.7.2.

Another argument for having these capabilities reported by firmware is that we need to maintain inclusive lists in Suite/Connect for features like tutorial. If those lists were exclusive, (e.g. model not in [T1B1, T2T1]), any prototypes of new models would expect the tutorial feature, hence trezor/trezor-suite#9889.

I don't see the full picture, so If this doesn't justify adding the capability flags, let's keep status quo.

If we really want to report this from firmware, we could make: (a) an enum: None, Vertical, Full (b) bit field representing 0, 90, 180, 270 (c) list of allowable values

Either solution is fine, I prefer C.

@Hannsek
Copy link
Contributor

Hannsek commented Jul 12, 2024

I agree with matejcik. Let's assume that all TS5s have tutorial. Not many users will do the onboarding on 2.7.2.

@Hannsek
Copy link
Contributor

Hannsek commented Jul 12, 2024

Also, why do you want to have the capability of the screen rotation in features?🤔 This can be done based on the model, not the fw version.

@komret
Copy link
Author

komret commented Jul 12, 2024

Also, why do you want to have the capability of the screen rotation in features?🤔 This can be done based on the model, not the fw version.

I thought firmware would be the best place to set this Maybe Connect is better. Currently, this logic exists in Suite so it cannot be shared with other frontends.

@Hannsek
Copy link
Contributor

Hannsek commented Jul 12, 2024

Which other frontends? You mean different 3rd party apps?

@komret
Copy link
Author

komret commented Jul 15, 2024

Yes, or connect-popup.

@Hannsek
Copy link
Contributor

Hannsek commented Jul 15, 2024

Isn't then Connect a better place for this? cc @mroz22

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code Code improvements
Projects
Status: No status
Development

No branches or pull requests

3 participants