test: Hive phase-2/3 screenshot capture + first Osmosis device tests#199
Merged
BitHighlander merged 7 commits intoJul 25, 2026
Merged
Conversation
…enshotted The eleven ops added by firmware #315 have had behavioural coverage since the day they landed — rc15 ran 38 Hive cases, 0 skipped, limit_order_create and limit_order_cancel among them. But SECTIONS stopped at G28, and screenshot_filter() only emits tests whose screenshot hint list is non-empty, so none of them were ever selected for the Phase-1 capture run. rc15 shipped 84 Hive OLED frames covering phase-1 only. That gap matters more here than for most tests: a correct signature over bytes the user was shown something ELSE for is precisely the failure the clear-sign table exists to prevent. Behavioural green says the device parsed and signed; only a frame says it drew "Sells 1.500 HIVE" and not a swapped symbol or a shifted decimal point. Adds G29-G38. Non-empty hints (so they capture) go to the ops that render a confirm screen: both limit-order ops, the six active-tier value ops, posting- tier claim, the zero-amount stop-power-down / remove-delegation pair, and both comment_options cases. The three pure-rejection tests (precision pinning, account_update2 authority reject, truncated bodies) keep empty hints on purpose — a refused op never draws a screen, so a hint there would select a test that can only produce an empty capture. Verified: --screenshot-filter --fw-version=7.15.0 now emits 304 tests and includes all seven new screen-rendering cases.
Verified against the rc16 capture (run 29796178504): the three frames it produced were all the idle home screen (195 ink px vs ~1100 for a real confirm). Every case in the test is _assert_ops_fails, so the device refuses before rendering — the capture could only ever be empty frames dressed up as visual proof, which is the exact failure Gate-3 exists to catch. The per-beneficiary confirm screens are real and are captured by G35, whose final case signs a two-beneficiary payout (14 frames, ink 650-1250).
…capture
Osmosis had NO device tests — the confirm screens were covered only by
host-side unit tests of the formatter in isolation. That is the wrong gap to
have open in 7.15.0, because 7.15.0 changed how every Osmosis amount is
DRAWN: fsm_msg_osmosis.h rendered with atof() + "%.6f", and a float carries
~7 significant decimal digits, so a large transfer displayed rounded on the
screen the user approves —
123456789123456 uosmo shown 123456792.000000 OSMO
actual 123456789.123456 OSMO
The signature was always over the correct amount; the error lived entirely on
the display, which is the half a hardware wallet exists to get right. It now
formats through bn_format_uint64 in integer math, exact at any magnitude.
Adds test_msg_osmosis_signtx.py (6 tests) and SECTIONS group P so four of
them are captured by screenshot_filter(): the baseline send, the
beyond-float-precision amount, a sub-unit amount (500 uosmo = 0.000500 OSMO,
must not collapse), and an unknown denom (shown as raw base units — the
device does not guess a precision). The frame is the evidence; a test that
only asserts "it signed" cannot prove what was drawn.
The two without screenshot hints are behavioural rather than visual: the
amount must be committed to the digest (two sends differing only in amount
give different signatures, so the screen is bound to what is signed), and
signing must be deterministic per RFC6979.
Scope: pyk's osmosis_sign_tx implements osmosis-sdk/MsgSend only. The
delegate/undelegate/LP/swap/IBC screens share this formatter but are not
reachable until the client learns those message types.
Verified: report renders 19 sections / P1-P6 present, filter selects the four
screenshot tests.
Adding the first Osmosis device tests immediately failed with "Unsupported denomination: uosmo". The cause is that osmosis_sign_tx's MsgSend branch had never executed: 1. It whitelisted 'uatom' — the COSMOS denom. Osmosis's native denom is uosmo, so signing a plain OSMO transfer was impossible. 2. It never forwarded the denom, though OsmosisMsgSend carries one. The firmware needs it to decide whether to scale (uosmo -> OSMO) or show raw base units. 3. It assigned amount=int(...) to OsmosisMsgSend.amount, which is a STRING field — that alone would have raised for uatom too. Bug 3 proves the path never ran; bugs 1 and 2 are why it went unnoticed. No denom whitelist belongs on the client: the firmware already handles any denom, scaling uosmo and showing everything else as raw base units precisely so it never guesses a precision it does not know.
The hardcoded fixture was a cosmos1 address with the prefix swapped to osmo1, so its bech32 checksum was invalid. osmosis_signTxUpdateMsgSend bech32-decodes to_address and returns false on failure, which surfaces as the opaque 'Failed to include send message in transaction' — a device-side syntax error that reads like a firmware bug and was mine. Asking the device for its own address also makes these genuine self-sends.
It is decorated @field('address'), so .address on the result raised AttributeError. Matches how test_msg_cosmos_getaddress uses its equivalent.
…does not display Review found that removing the old denom whitelist opened a real display/signature divergence, and that my test certified it as correct. osmosis_signTxUpdateMsgSend takes only (amount, to_address) and hardcodes "denom":"uosmo" into the amino JSON it hashes; fsm_msgOsmosisMsgAck renders whatever denom arrived. So a uatom send DISPLAYS "1500000 uatom" and SIGNS 1500000 uosmo. Exploitable in the large: a big number of some worthless ibc/... token on screen, a big number of OSMO in the signature. osmosis_signTxUpdateMsgDelegate already takes a denom — MsgSend is the outlier, and this is a pre-existing firmware bug my client change made reachable. Fenced client-side until the firmware serializer accepts a denom. The denom is still forwarded, since the firmware needs it to decide whether to scale and the fence is the temporary half. test_osmosis_send_unknown_denom_shown_raw asserted only len(signature)==64, so it PASSED against that divergence and published it as intended behaviour — worse than no test. Replaced with one that asserts the refusal, and documents the check to write once the firmware is fixed: otherwise-identical uosmo and uatom transactions must produce DIFFERENT signatures. Also corrects two overstatements the same review caught: - "exact at any magnitude" was false. osmosis_formatAmount converts with an unchecked strtoull(), so it is exact only for a canonical decimal uint64. strtoull saturates past UINT64_MAX and accepts whitespace, a sign and 0x — "18446744073709551616" and "-1" both display as 18446744073.709551615 OSMO while the original string is hashed. Same divergence, different disguise; needs a firmware-side canonical-range check. - G36 claimed enforcement of the extension cap, the 1-8 count bound and per-weight range. The mapped test asserts only unsorted, duplicate and over-100%. Narrowed to what it actually proves. P4 no longer carries a screenshot hint: the refusal happens client-side, so there is no frame to capture.
8 tasks
Contributor
Author
|
Superseded by #197. The The consolidated head passed the complete local emulator suite: 574 passed, 16 skipped, 0 failed, with 153 passing subtests. |
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
1 participant
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.
Needed before firmware 7.15.0 goes upstream: the firmware release pins this
repo, and these commits currently exist only on a feature branch. Merging
here makes the pin durable against the branch being deleted.
1. Hive phase-2/3 ops were tested but never screenshotted
The eleven ops added by firmware #315 have run green in the full suite since
they landed (38 Hive cases, 0 skipped). But
SECTIONSstopped at G28, andscreenshot_filter()only emits tests whose screenshot hint list isnon-empty — so none were ever selected for the Phase-1 capture run. The
7.15.0-rc15 report shipped 84 Hive OLED frames covering phase-1 only.
That is the wrong half to be missing for a clear-sign table. A correct
signature over bytes the user was shown something ELSE for is the exact
failure the table exists to prevent; behavioural green proves the device
parsed and signed, only a frame proves it drew
Sells 1.500 HIVE.Adds G29-G38. Verified against a real capture run: 44 new frames, including
the
MARKET ORDER / @kktrader sells / 1.5 HIVE / for >= 0.4 HBDscreen.G36 deliberately carries NO hint: every case in
test_hive_sign_ops_comment_options_beneficiary_rulesis_assert_ops_fails, so the device refuses before rendering. Its firstcapture produced three frames of the idle home screen (195 ink px, versus
~1100 for a real confirm) — a report entry that looks like visual proof and
is not. The per-beneficiary screens are captured by G35, which actually
signs a two-beneficiary payout.
2. Osmosis had no device tests at all
Not "missing from the report" — none existed. No
test_msg_osmosis*.py, andosmosis_sign_tx/osmosis_get_addresswere never called from any test.That is why
osmosis_sign_tx's MsgSend branch could carry three bugswithout anyone noticing:
uatom— the COSMOS denom — so a nativeuosmosend wasrejected outright;
OsmosisMsgSendcarries one and thefirmware needs it to decide whether to scale;
amount=int(...)toOsmosisMsgSend.amount, a STRING field.Bug 3 proves the branch had never executed even once. The whitelist is
removed entirely — the firmware already handles any denom, scaling
uosmoand showing everything else as raw base units precisely so it never guesses
a precision it does not know.
This matters now because 7.15.0 changed how every Osmosis amount is DRAWN.
fsm_msg_osmosis.hrendered withatof()+"%.6f"; a float carries ~7significant decimal digits, so a large transfer displayed rounded:
The signature was over the correct amount either way — the error lived
entirely on the display. Amounts now format via
bn_format_uint64ininteger math.
Adds
test_msg_osmosis_signtx.py(6 tests) andSECTIONSgroup P. Fourcapture frames: baseline send, the beyond-float-precision amount, a sub-unit
amount (500 uosmo = 0.000500 OSMO, must not collapse), and an unknown denom.
Two are behavioural rather than visual and carry no hint: the amount must be
committed to the digest (two sends differing only in amount give different
signatures, so the screen is bound to what is signed), and signing must be
deterministic per RFC6979.
Scope:
osmosis_sign_tximplementsosmosis-sdk/MsgSendonly. Thedelegate/undelegate/LP/swap/IBC screens share this formatter but are not
reachable until the client learns those message types.
Verification
Full CI on keepkey-firmware
release/7.15.0-rc16: 572 passed, 16 skipped,0 failed; report renders 19 sections / 301 tests / 0 pending. Screenshot
frames confirmed to contain real pixel content, not just to exist.