feat: allow Qt app to talk to the simulator. #3621
Conversation
| // DeviceInfos returns the list of connected USB devices. If the simulator is running, it returns only | ||
| // the simulator device info. | ||
| // Otherwise, it returns the actual connected USB devices. | ||
| func DeviceInfos() []usb.DeviceInfo { |
There was a problem hiding this comment.
I would undo this and put it back into servewallet/main.go. It's only used there, and to me it makes more sense that the caller makes the distinction, and not the bitbox02/simulator package, similar like you do it in qt's server.go
There was a problem hiding this comment.
Done. The "ugly" part of this, is that there is duplication of code, as now servewallet/main's DeviceInfos is basically duplicated in server.go but I think we can live with it.
frontends/qt/server/server.go
Outdated
| func deviceInfos() []usb.DeviceInfo { | ||
| testDeviceInfo := simulator.TestDeviceInfo() | ||
| if testDeviceInfo != nil { | ||
| return []usb.DeviceInfo{*testDeviceInfo} |
There was a problem hiding this comment.
why dereference? can just keep testDeviceInfo by ref right? Same in servewallet/main.go
backend/bridgecommon/bridgecommon.go
Outdated
| } | ||
|
|
||
| if simulator && !globalBackend.Testing() { | ||
| log.Panic("Simulator can only be used in testnet mode") |
There was a problem hiding this comment.
This is not visible on the command line. Maybe also write to stderr so users can see what went wrong.
Thinking some more about this, maybe it's most ergonomical if -simulator just implied -testnet? Then you could remove the simulator flag here and this check here and just pass testnet=true if simulator is set.
Wdyt?
frontends/qt/server/server.go
Outdated
| log.WithField("args", os.Args).Info("Started Qt application") | ||
| testnet := flag.Bool("testnet", false, "activate testnets") | ||
| simulatorPort := flag.Int("simulatorPort", 15423, "port for the BitBox02 simulator") | ||
| useSimulator := flag.Bool("simulator", false, "use the BitBox02 simulator") |
There was a problem hiding this comment.
mention that it implies testnet (if you make this change), or mention that testnet is required if you prefer to keep the testnet check
2 participants
Before asking for reviews, here is a check list of the most common things you might need to consider: