api/firmware/device: info use device status fields #98

NicolaLS · 2024-06-12T20:00:54Z

Return initialized status from info() and use unlocked and initialized to set the device status appropriately.

After firmware PR 1229

api/firmware/device.go

NicolaLS · 2024-06-24T16:26:04Z

(added version check and fixed some formatting)

benma · 2024-08-01T05:33:42Z

api/firmware/device.go

@@ -145,24 +145,24 @@ func NewDevice(

 // info uses the opInfo api endpoint to learn about the version, platform/edition, and unlock
 // status (true if unlocked).


update docstring please

benma · 2024-08-01T05:34:10Z

api/firmware/device.go

@@ -145,24 +145,24 @@ func NewDevice(

 // info uses the opInfo api endpoint to learn about the version, platform/edition, and unlock
 // status (true if unlocked).
-func (device *Device) info() (*semver.SemVer, common.Product, bool, error) {
+func (device *Device) info() (*semver.SemVer, common.Product, bool, *bool, error) {


at this point i think it makes sense to convert the result into a struct with named and documented fields

benma · 2024-08-01T05:41:54Z

api/firmware/device.go

@@ -261,7 +287,7 @@ func (device *Device) Init() error {

 	// Before 2.0.0, unlock was invoked automatically by the device before USB communication
 	// started.
-	if device.version.AtLeast(semver.NewSemVer(2, 0, 0)) {
+	if device.version.AtLeast(semver.NewSemVer(2, 0, 0)) && (device.status != StatusInitialized) {


This is wrong I think - Initialized means backup created, not unlocked. The docstring in status.go might be wrong. Regardless it's an unrelated change.

Yes the docstring on StatusInitialized is wrong, I got confused because I thought it actually meant something else in this repo (also because the animation stops playing when status changes from StatusConnected to StatusInitialized) but we actually need a new status StatusUnlocked and use that.

So this can then be changed to 'initialized' || 'seeded':
https://github.com/BitBoxSwiss/bitbox-wallet-app/blob/0fa7a7144984fa5a4f7731d222b69505902b220a/frontends/web/src/routes/device/bitbox02/wizard.tsx#L103

~~I don't think we need StatusUnlocked~~ 'seeded' is the step after setting password, before creating a backup, but upon re-plug the user starts from the beginning (by design). No need to change to 'seeded' in the above link.

Ok, I was not sure I thought there might be a situation where the user unplugs the device after setting a password but before finishing the backup which would cause the device to end up 'seeded' when reconnecting it.

I don't think we need StatusUnlocked.

makes sense, I'll remove StatusUnlocked and set the status to StatusInitialized if deviceInfo.unlocked || deviceInfo.initialized

benma · 2024-08-01T05:43:50Z

api/firmware/device.go

+	if err != nil {
+		return errp.New(
+			"OP_INFO unavailable; need to provide version and product via the USB HID descriptor")


This is incorrect - old firmwares don't support OP_INFO and we can't just error here. Before, the OP_INFO call to infer the version/product would be done only if it was not already provided.

What you now want to do is probably to call info() if supported by the firmware (need to look up fw version), and use the results of it when possible.

Ah yes. We do need to call info() if we don't know if its supported device.version == nil too tough, so I'll just put this into a version == nil || version atLeast(4.3.0)

As mentioned above here:
#98 (comment)

I think changing the if-clause around this to:
if device.version == nil || device.version.AtLeast(semver.NewSemVer(9, 2, 0)) { ...}
is the way to go.

side note: But I also think that it was not incorrect though, we basically do the same already:

bitbox02-api-go/api/firmware/device.go

Line 247 in a2115fe

return err

Just propagated from the inferVersionAndProduct function instead of directly.

api/firmware/device.go

NicolaLS · 2024-08-01T16:07:56Z

@benma thanks. addressed these, PTAL:

updated info docstring
return struct *DeviceInfoREQ_INFO from info now.
only call info if version == nil or version.atLeast(4,3,0)
add status StatusUnlocked and use it instead of StatusInitialized
fixed SatusInitialized docstring.

Now StatusConnected does not really do anything...but I think it is good to keep it instead of e.g. initializing device.status with nil or StatusUninitialized.

~~reminder to self: this needs to be changed after this PR is merged (breaking change).~~

NicolaLS · 2024-08-01T16:08:50Z

I also added one commit that renames OP_INFO to REQ_INFO in the comments/docstrings. but can drop that if not wanted.

benma

I need to think more about StatusUnlocked and how it relates to the problem in the bbapp. Could be good already, but I need to revisit it later.

benma · 2024-08-01T16:32:17Z

api/firmware/device.go

@@ -114,15 +114,28 @@ type DeviceInfo struct {
 	SecurechipModel string `json:"securechipModel"`
 }

+// DeviceInfoREQ_INFO is the data returned from the REQ_INFO api call.
+type DeviceInfoREQ_INFO struct {


can unexport the struct, and i'd simply name info struct or sth, REQ_INFO is not really following any naming pattern in Go :P

benma · 2024-08-01T16:37:57Z

api/firmware/device.go


-	if err := device.inferVersionAndProduct(); err != nil {
-		return err
+	if device.version == nil || device.version.AtLeast(semver.NewSemVer(4, 3, 0)) {


If the version is below 4.3, you can't call it either really as it does not exist, so the 2nd condition alone is enough.

If the fw is older and version is nil, you could return an error, as at least one of them has to be true.

I don't think you need to call inferVersionAndProduct at all really. It helped to avoid the OP_INFO call if the version was already known, but you call it now always, so its not really doing anything. Maybe you could compare the version/product to the one already set if available as a sanity check, but you can just also override it directly.

I don't understand.

we want to call info() always except if the version is known before 4.3. doesn't this mean we call it either if version is nil or if its not nil but higher than 4.3 ?

why is it an error if fw is older than 4.3 and nil ? device.version.AtLeast() will be false (older) if device.version is nil

I don't think you need to call inferVersionAndProduct at all really.

I agree, let me know what you think is best, sanity check, override or just removing the function.

Thinking about this again, I think removing the inferVersionAndProduct makes most sense.

Actually without the first condition there would be a nil pointer dereference error if the version is actually nil, this crashes:

- if device.version == nil || device.version.AtLeast(semver.NewSemVer(4, 3, 0)) { + device.version = nil + if device.version.AtLeast(semver.NewSemVer(4, 3, 0)) {

I think changing it to if (device.version == nil || device.version.AtLeast(semver.NewSemVer(9, 2, 0))) makes sense:
We always want to check info() because we're interested in the initialized field:

if we know the version and we know it supports the initialized field we want to call info()

if the version is nil we want to call info() like we did before in inferVersionAndProduct

if we know the version but we know we won't learn the initialized field from calling info() there is no need to call it anyways, just like inferVersionAndProduct avoided calling it if the version was already known.

(unimportant) I think I initially used AtLeast(4,3,0) because I thought it makes sense since info() will only work for at least this version, but I did not see that if we know the version and we don't want to call info() unless we know we can get the initialized status from it, even if calling it is supported.

benma · 2024-08-01T16:39:44Z

api/firmware/device.go

@@ -261,7 +293,7 @@ func (device *Device) Init() error {

 	// Before 2.0.0, unlock was invoked automatically by the device before USB communication
 	// started.
-	if device.version.AtLeast(semver.NewSemVer(2, 0, 0)) {
+	if device.version.AtLeast(semver.NewSemVer(2, 0, 0)) && (device.status != StatusUnlocked) {


again, unrelated change and not needed. why did you add it?

removed it now, mb I think I just thought since we can check for that now it makes sense to do so.

Return struct containing REQ_INFO response data from device.info(). Amongst the already existing fields that were previously returned by info() as a tuple, also add the initialized status to the struct that is returned after firmware version 9.20.0. In case the device does not respond with the initialized status byte yet it will be nil. Otherwise true/false depending on the initialized status (seeded and backup created) of the device. Also use the information from device.info to set the device.status earlier to avoid showing the password video if the device is not initialized. Also rename OP_INFO to REQ_INFO because the name changed after this commit: BitBoxSwiss/bitbox02-firmware@a785012 The name should be the same as in the firmware repositroy so that the code base is easier to understand. Also rename StatusInitialized to StatusUnlocked to avoid confusion.

NicolaLS · 2024-10-03T03:18:07Z

@benma I think this is ready now, I tested it thoroughly with BB02M FW 9.20.0 and BB02B FW 9.19.0.

App needs the following changes:
BitBoxSwiss/bitbox-wallet-app@eecba5d

To test I put replace github.com/BitBoxSwiss/bitbox02-api-go => github.com/NicolaLS/bitbox02-api-go f484ac834812ea3a433b5577bcc36711232c9cf4 in go.mod and then go mod tidy, go mod vendor.

I need to think more about StatusUnlocked and how it relates to the problem in the bbapp. Could be good already, but I need to revisit it later.

This was very confusing because Initialized means something different in the Firmware then it did in the app/go-api. In the app/go-api, we used StatusInitialized when the device was unlocked. But the Firmware regards any device that can be unlocked as initialized. I renamed this in the go-api now to avoid this confusion.

TLDR; Is that we want to set the status to StatusConnected if we can't learn the initialized (fw semantics) status because the fw is below 9.20.0 OR if the device is (fw semantics) is initialized (ie. wallet loaded).

If the device is unlocked we obviously set it to StatusUnlocked which was called StatusInitialized before this PR and caused the confusion.

side note: there is still a StatusInitialized in the backend now, one for the BB01 but this is not affected by go-api and another one here:
https://github.com/BitBoxSwiss/bitbox-wallet-app/blob/9ed78882c0dd6a9fc49a8d519d4888a171a8eb57/backend/devices/bitbox/status.go#L31

But this one is used for another call on the device and actually means initialized and not unlocked if I understand it correctly.

NicolaLS commented Jun 12, 2024

View reviewed changes

api/firmware/device.go Outdated Show resolved Hide resolved

NicolaLS force-pushed the device-info-initialized branch 2 times, most recently from 68e1951 to 0008efa Compare June 12, 2024 20:22

NicolaLS force-pushed the device-info-initialized branch from 0008efa to 51ae730 Compare June 24, 2024 16:25

NicolaLS force-pushed the device-info-initialized branch from 51ae730 to aca0124 Compare July 30, 2024 13:25

benma requested changes Aug 1, 2024

View reviewed changes

NicolaLS commented Aug 1, 2024

View reviewed changes

api/firmware/device.go Outdated Show resolved Hide resolved

NicolaLS commented Aug 1, 2024

View reviewed changes

api/firmware/device.go Outdated Show resolved Hide resolved

NicolaLS force-pushed the device-info-initialized branch from aca0124 to 15a4a16 Compare August 1, 2024 16:00

benma reviewed Aug 1, 2024

View reviewed changes

NicolaLS force-pushed the device-info-initialized branch from 15a4a16 to 02bafe6 Compare August 1, 2024 17:01

NicolaLS requested a review from benma August 1, 2024 17:01

NicolaLS force-pushed the device-info-initialized branch from 02bafe6 to ccf77e3 Compare August 1, 2024 18:08

NicolaLS force-pushed the device-info-initialized branch 3 times, most recently from 3c00608 to 77762fe Compare October 3, 2024 02:36

NicolaLS force-pushed the device-info-initialized branch from 77762fe to f484ac8 Compare October 3, 2024 03:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

api/firmware/device: info use device status fields #98

api/firmware/device: info use device status fields #98

NicolaLS commented Jun 12, 2024

NicolaLS commented Jun 24, 2024

benma Aug 1, 2024

benma Aug 1, 2024

benma Aug 1, 2024

NicolaLS Aug 1, 2024

NicolaLS Aug 1, 2024

benma Aug 1, 2024 •

edited

Loading

NicolaLS Aug 1, 2024

benma Aug 1, 2024

NicolaLS Aug 1, 2024

NicolaLS Oct 2, 2024

NicolaLS commented Aug 1, 2024 •

edited

Loading

NicolaLS commented Aug 1, 2024

benma left a comment

benma Aug 1, 2024

benma Aug 1, 2024 •

edited

Loading

NicolaLS Aug 1, 2024

NicolaLS Oct 2, 2024

benma Aug 1, 2024

NicolaLS Aug 1, 2024

NicolaLS commented Oct 3, 2024

		@@ -145,24 +145,24 @@ func NewDevice(

		// info uses the opInfo api endpoint to learn about the version, platform/edition, and unlock
		// status (true if unlocked).

api/firmware/device: info use device status fields #98

Are you sure you want to change the base?

api/firmware/device: info use device status fields #98

Conversation

NicolaLS commented Jun 12, 2024

NicolaLS commented Jun 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benma Aug 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NicolaLS commented Aug 1, 2024 • edited Loading

NicolaLS commented Aug 1, 2024

benma left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benma Aug 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NicolaLS commented Oct 3, 2024

benma Aug 1, 2024 •

edited

Loading

NicolaLS commented Aug 1, 2024 •

edited

Loading

benma Aug 1, 2024 •

edited

Loading