Skip to content

feat(trezor): add FirmwareError and DeviceLocked TrezorException variants (#124) - #143

Open
coreyphillips wants to merge 3 commits into
masterfrom
issue-124
Open

feat(trezor): add FirmwareError and DeviceLocked TrezorException variants (#124)#143
coreyphillips wants to merge 3 commits into
masterfrom
issue-124

Conversation

@coreyphillips

Copy link
Copy Markdown
Collaborator

Closes #124

Add TrezorError::FirmwareError (protocol failure code 99) and TrezorError::DeviceLocked, plus trezor_ensure_unlocked() so apps stop sniffing error strings and re-deriving the locked rule.

Two Trezor failure states had no typed representation at the FFI boundary. Protocol failure code 99 (Failure_FirmwareError) landed in the catch-all TrezorError::DeviceError arm in src/modules/trezor/errors.rs, so Android told firmware faults apart by matching the strings "Device error (code 99)" and "Firmware error". A locked device was worse: ThpError::DeviceLocked was mapped to DeviceBusy, and the apps separately re-derived pin_protection && !unlocked from TrezorFeatures and synthesized a DeviceBusy of their own. That collapses two states with opposite UX ("unlock your device" vs "wait and retry") onto one variant, and both classifications break the moment core's error copy changes.

What changed

  • TrezorError gains FirmwareError { error_details } and DeviceLocked. Both are appended at the end of the enum so the existing variants keep their FFI discriminants.
  • In errors.rs, code 99 now maps to FirmwareError from both upstream shapes that can carry it (DeviceError::Failure { code: Some(99), .. } and DeviceError::DeviceError { code: 99, .. }). The error_details text is byte-for-byte what it was, so apps still running the message heuristics keep working until they switch over.
  • ThpError::DeviceLocked maps to TrezorError::DeviceLocked instead of DeviceBusy, leaving DeviceBusy to mean transport/session contention only.
  • TrezorFeatures::is_locked() in types.rs owns the rule: pin_protection == Some(true) && unlocked == Some(false). unlocked == None (firmware too old to report it) counts as not locked.
  • TrezorManager::ensure_unlocked() and the exported trezor_ensure_unlocked() apply that rule to the cached features and return DeviceLocked, or NotConnected when nothing is connected. No device interaction, so callers wanting a fresh answer call trezor_refresh_features() first.

How to test

  • cargo test --lib modules::trezor, 65 pass. New/changed: test_failure_code_firmware_error_surfaces_as_firmware_error, test_direct_device_error_code_99_surfaces_as_firmware_error, test_failure_code_unknown_stays_generic_device_error (now uses code 2 and still asserts generic DeviceError), test_thp_device_locked_surfaces_as_device_locked_not_busy, errors::tests::thp_device_locked_maps_to_device_locked, test_features_is_locked, test_ensure_unlocked_without_device_is_not_connected, and two added assertions in test_error_display_messages. The existing test_error_conversion_device_busy and the two callback busy-mapping tests are untouched and still pass.
  • cargo test --lib, 498 pass, 11 fail. All 11 failures are Blocktank tests that need api.stag.blocktank.to, which this sandbox blocks; they fail the same way on an unmodified tree.
  • cargo fmt --check clean, cargo clippy --lib reports only the warnings already present on master (nothing in the new code).

Notes

Two calls worth pushing back on. I did not regenerate the UniFFI bindings. I started ./build.sh -r --patch all, and after ~90 minutes it was still on the first of seven release builds, so six cross-target builds were hours away. I killed it and restored the tree (the version is still 0.5.9 and bindings/ is untouched). This matches how the repo already works: the last two releases landed the source change and then a separate chore: bump to X and regenerate bindings commit. That commit still needs to happen before the apps can consume this. trezor_ensure_unlocked() is a pre-flight check, not a gate inside get_address() / sign_tx(). The issue offered gating operations as an option and I deliberately did not take it: a locked device can still be unlocked mid-operation, either through the PIN callback (Trezor One over USB, which UiCallbackAdapter::on_pin_request exists to serve) or on the device's own screen. Rejecting those calls up front would break a flow that works today. The app calls this where requireUnlocked() used to sit and gets the same behaviour, with the rule living in core. The first commit (e3b503d) is just cargo fmt output for src/modules/activity/backup_migration.rs, which was unformatted on master. Split out so it does not muddy the feature diff.

Trezor protocol failure code 99 (Failure_FirmwareError) was mapped to the
generic TrezorError::DeviceError, and a device that reported itself locked
during the THP handshake was mapped to DeviceBusy. Native apps therefore
classified both by sniffing error message text ("Device error (code 99)",
"Firmware error") or by re-deriving the locked rule from TrezorFeatures and
synthesizing DeviceBusy, which conflates "unlock your device" with "back off
and retry".

Add FirmwareError and DeviceLocked to the exported error enum, map code 99 to
FirmwareError from both DeviceError shapes, and map ThpError::DeviceLocked to
DeviceLocked so DeviceBusy means transport/session contention only. The
error_details strings are unchanged, so existing app heuristics keep working
until they switch to the typed variants.

Also add trezor_ensure_unlocked(), which applies the
pin_protection && !unlocked rule to the cached features and returns
DeviceLocked. It is a pre-flight check rather than a gate inside get_address /
sign_tx, because a locked device can still be unlocked mid-operation via the
PIN callback (Trezor One) or on its own screen.
Swift, Kotlin and Python bindings plus native artifacts were still built
from the 21-case TrezorError enum, so clients could not reach FirmwareError
(tag 22), DeviceLocked (tag 23) or trezor_ensure_unlocked.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

feat(trezor): add FirmwareError and DeviceLocked TrezorException variants

1 participant