Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: show fee rate when signing transaction #2249

Merged
merged 1 commit into from
May 25, 2022
Merged

Conversation

marnova
Copy link

@marnova marnova commented May 3, 2022

Resolves issue #1294

@marnova marnova self-assigned this May 3, 2022
@marnova marnova marked this pull request as ready for review May 17, 2022 07:52
@marnova marnova requested a review from prusnak as a code owner May 17, 2022 07:52
@prusnak
Copy link
Member

prusnak commented May 17, 2022

Can you please post before and after screenshots from emulators so we see how the new screens look like?

Also it would be helpful, if you set the fee to some custom value in the Suite (e.g. 3 sat/vbyte) and show the value is shown the same in the Suite and on the device?

@matejcik
Copy link
Contributor

note, CI will generate screenshot diff as soon as device tests pass

@marnova
Copy link
Author

marnova commented May 17, 2022

@prusnak @matejcik
Screenshots are generated even though core device test failed:

  1. summary for TT is e.g. here for last pipeline (diffs in screens are best visible in the failed tests),
  2. and for T1 summary could be found here and one example of recorded screen with fee rate here.

Will try it with Suite+emulator later today/tomorrow.

@marnova marnova linked an issue May 18, 2022 that may be closed by this pull request
@matejcik
Copy link
Contributor

@marnova please fix the tests that are still failing

@bosomt
Copy link

bosomt commented May 19, 2022

QA OK

a1d2289
trezor-fw-regular-2.5.1-a1d22890.bin
trezor-fw-regular-1.11.1-a1d22890.bin

@marnova
Copy link
Author

marnova commented May 19, 2022

@prusnak @matejcik

  • T1

fee_rate_T1_suite

fee_rate_T1_device


  • TT

fee_rate_TT_suite

fee_rate_TT_device

legacy/firmware/layout2.c Outdated Show resolved Hide resolved
@marnova marnova force-pushed the marnova/show_fee_rate branch from 828de33 to e3f46f4 Compare May 24, 2022 06:26
@mmilata
Copy link
Member

mmilata commented May 24, 2022

UI diff shows nothing due to #2285. For e3f46f4 you can see it here: https://satoshilabs.gitlab.io/-/trezor/trezor-firmware/-/jobs/2500213083/artifacts/master_diff/index.html

@matejcik
Copy link
Contributor

@marnova
Copy link
Author

marnova commented May 25, 2022

@matejcik
Not sure whether those differences are valid. The one you linked shows (for that particular test), that my branch has hash 1f46b6ec89be1742dc4649153e6d13b172c9e93be7c91063650f814e631b757c - but that's not true - see here

"T1_bitcoin-test_signtx_replacement.py::test_p2tr_invalid_signature": "00f4f745f88f1072f495e513d6a7747db494bb7c785c32d167652fe1d79509be",

Also the latest pipeline on Gitlab shows different screens - https://satoshilabs.gitlab.io/-/trezor/trezor-firmware/-/jobs/2497157113/artifacts/test_ui_report/passed/T1_bitcoin-test_signtx_replacement.py::test_p2tr_invalid_signature.html

@mmilata could be that the script has other bug(s)? Or it compared screens with different commit (even though I'm not aware that I would remove any screen completely)?

@mmilata
Copy link
Member

mmilata commented May 25, 2022

My bad, I forgot to git fetch and the report I posted is actually for 828de33. #2285 is merged, you can rebase on top of master now to get the up to date report.

@marnova marnova force-pushed the marnova/show_fee_rate branch from e3f46f4 to 7b7e06e Compare May 25, 2022 09:03
@matejcik
Copy link
Contributor

@bosomt can you please test this with DOGE where fees are very large (last time I checked)?

@bosomt
Copy link

bosomt commented May 25, 2022

@matejcik DOGE fees are already changed.
Suite and TT are reporting same fee rate Fee rate 1000 sat/B or 1000.0 sat/vB
When i try to set it manually to 6555555.99 sat/B they are displayed correctly
/except that device only shows6555555.00 sat/B but it's known issue /

@matejcik matejcik merged commit 85f0d3a into master May 25, 2022
@matejcik matejcik deleted the marnova/show_fee_rate branch May 25, 2022 12:43
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.

Show fee rate on Trezor
5 participants