feat(trezor): add FirmwareError and DeviceLocked TrezorException variants (#124) - #143
Open
coreyphillips wants to merge 3 commits into
Open
feat(trezor): add FirmwareError and DeviceLocked TrezorException variants (#124)#143coreyphillips wants to merge 3 commits into
coreyphillips wants to merge 3 commits into
Conversation
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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::DeviceErrorarm insrc/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::DeviceLockedwas mapped toDeviceBusy, and the apps separately re-derivedpin_protection && !unlockedfromTrezorFeaturesand synthesized aDeviceBusyof 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
TrezorErrorgainsFirmwareError { error_details }andDeviceLocked. Both are appended at the end of the enum so the existing variants keep their FFI discriminants.errors.rs, code 99 now maps toFirmwareErrorfrom both upstream shapes that can carry it (DeviceError::Failure { code: Some(99), .. }andDeviceError::DeviceError { code: 99, .. }). Theerror_detailstext is byte-for-byte what it was, so apps still running the message heuristics keep working until they switch over.ThpError::DeviceLockedmaps toTrezorError::DeviceLockedinstead ofDeviceBusy, leavingDeviceBusyto mean transport/session contention only.TrezorFeatures::is_locked()intypes.rsowns 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 exportedtrezor_ensure_unlocked()apply that rule to the cached features and returnDeviceLocked, orNotConnectedwhen nothing is connected. No device interaction, so callers wanting a fresh answer calltrezor_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 genericDeviceError),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 intest_error_display_messages. The existingtest_error_conversion_device_busyand 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 needapi.stag.blocktank.to, which this sandbox blocks; they fail the same way on an unmodified tree.cargo fmt --checkclean,cargo clippy --libreports 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 andbindings/is untouched). This matches how the repo already works: the last two releases landed the source change and then a separatechore: bump to X and regenerate bindingscommit. That commit still needs to happen before the apps can consume this.trezor_ensure_unlocked()is a pre-flight check, not a gate insideget_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, whichUiCallbackAdapter::on_pin_requestexists 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 whererequireUnlocked()used to sit and gets the same behaviour, with the rule living in core. The first commit (e3b503d) is justcargo fmtoutput forsrc/modules/activity/backup_migration.rs, which was unformatted on master. Split out so it does not muddy the feature diff.