test(uniswap): run the liquidity tests on the emulator instead of skipping them - #32
Merged
Merged
Conversation
…pping them These three were skipped whenever firmware_variant starts with "Emulator", and the emulator is the only thing CI runs — so they have never executed in CI on any branch. The firmware test report reads "327/330 PASSED, 3 skipped (withheld)" with every EVM section green, and these are the 3. The skip said "Skip until emulator issue resolved" and the comment above it said "Pre-existing, unrelated to clear-signing" and "on-device this path is exercised by the app". Both were wrong. Lifting the skip found a firmware logic defect that fails on hardware too: confirmFromAccountMatch() in zxliquidtx.c ended in `return is_self`, refusing the transaction AFTER the user approved the recipient screen, so the device answered "Signing cancelled by user" for a transaction the user had just confirmed. The vectors show the split themselves — add_liquidity's recipient word is commented "# eth address (self)" and passes; remove_liquidity's is "# to address (not self)" and failed. Fixed in keepkey-firmware as "fix(evm): a Uniswap recipient screen the user approved is an approval". Against that firmware all three pass, asserting their exact sig_v/sig_r/sig_s, so the device is signing the right bytes. Keeping the skip would keep the report green by not looking.
BitHighlander
added a commit
to BitHighlander/keepkey-firmware
that referenced
this pull request
Aug 21, 2026
pyk master gains "test(uniswap): run the liquidity tests on the emulator instead of skipping them" (BitHighlander/python-keepkey#32). Those three were skipped whenever firmware_variant starts with "Emulator", and the emulator is the only thing CI runs, so they had never executed in CI on any branch. They are the "3 skipped (withheld)" behind the report's "327/330 PASSED" and its green EVM sections. With this pin they execute. Against this tree all three pass, asserting their exact sig_v/sig_r/sig_s, so the device signs the right bytes rather than merely not erroring. Verified locally against the emulator built from this commit: test_sign_uni_add_liquidity_ETH PASSED test_sign_uni_approve_liquidity_ETH PASSED test_sign_uni_remove_liquidity_ETH PASSED The report should now read 330/330 with nothing withheld. If a future run shows these skipped again, the pin moved.
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.
These three tests are skipped whenever
firmware_variantstarts with"Emulator"— and the emulator is the only thing CI runs, so they have never executed in CI on any branch.The firmware test report reads
327/330 PASSED, 3 skipped (withheld)with every EVM section green. These are the 3 withheld.What the skip was hiding
The guard said
"Skip until emulator issue resolved", and the comment above it said "Pre-existing, unrelated to clear-signing" and "on-device this path is exercised by the app". Both wrong — it is a firmware logic defect that fails on hardware too.confirmFromAccountMatch()inlib/firmware/ethereum_contracts/zxliquidtx.cended inreturn is_self, refusing the transaction after the user approved the recipient screen.ethereum.cturns that into ActionCancelled, so the device answeredSigning cancelled by userfor a transaction the user had just confirmed. EveryremoveLiquidityETHto a third party was unsignable.Lifting the skip, the vectors name the split themselves:
test_sign_uni_add_liquidity_ETH# eth address (self)test_sign_uni_approve_liquidity_ETHtest_sign_uni_remove_liquidity_ETH# to address (not self)Requires
keepkey-firmware
fix(evm): a Uniswap recipient screen the user approved is an approval, onalpha. Against that firmware all three pass — and they assert exactsig_v/sig_r/sig_s, so this is the device signing the right bytes, not merely not erroring.Keeping the skip would keep the report green by not looking.