-
-
Notifications
You must be signed in to change notification settings - Fork 670
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
core: add hard limit for transaction fees #1155
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Few comments.
request_output(1), | ||
request_finished(), | ||
] | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@matejcik is there any added value in testing all the expected responses in this test case? It makes sense mostly because of "B.FeeOverThreshold" but other than that it is almost annoying because we need to change a lot of tests when we change something. Maybe it would be interesting to introduce client.assertItemInExpectedResponses(proto.ButtonRequest(code=B.FeeOverThreshold))
?
Also @mmilata please replace the "proto" with "messages", we are trying to get rid of that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that's easy enough to implement -- just record responses and then assert something in client.responses
maybe a better choice is to use the layout functionality though. Unfortunately that only works on TT, but this buttonrequest doesn't happen on T1 anyway.
Speaking of, I'm wondering what to do about the layouts thing when the port is done. Presumably some of the text will be wildly different. But maybe the extended ButtonRequest functionality will make up for it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As we have set_input_flow
and set_expected_responses
, I didn't want to add another mechanism for monitoring messages. Instead I tried to use the former in f45099d. Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's interesting, but not worth it IMHO. there are three button requests total; better write out all three layout assertions explicitly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed to set_input_flow
with explicit ButtonRequest assertions in 52b4502.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK from me but I agree with @tsusanka's comments
There is a conflict due to #1127 getting merged. OK to rebase? |
Yes, please autosquash, rebase and force push. We will do one last check afterwards. |
ff1d65a
to
315acbf
Compare
The hard limit is set to 10*fee_warning_threshold. The limit is not enforced when `safety_checks` is set to "Prompt".
315acbf
to
306558a
Compare
Rebased & CI passes. Please take a look. |
The hard limit is set to
10*fee_warning_threshold
. The limit is not enforced whensafety_checks
is set to "Prompt".TT only because T1 doesn't support
safety_checks
. Do we want to add it?Fixes (part of) #1087.