-
Notifications
You must be signed in to change notification settings - Fork 174
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
Clarify Language & Add Warning for Signing Without Multisig Descriptor Loaded #533
Clarify Language & Add Warning for Signing Without Multisig Descriptor Loaded #533
Conversation
… a descriptor loaded.
This reverts commit df7e1cb.
DireWarningScreen, | ||
title="Security Warning", | ||
status_icon_name=SeedSignerIconConstants.WARNING, | ||
status_headline="Potential Loss of Funds", |
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.
I don't think this needs to be a DireWarningScreen (but a yellow standard WarningScreen
I'd be good with). This said, I think this PR overall is a good improvement and will merge as is. Would be trivial to change to WarningScreen
prior to another release if needed.
ACK and Tested |
Sorry for being late to the party but I'm definitely a NACK on the DireWarningScreen usage here. Also, the message doesn't convey that this is a direct result of the user choosing to not verify the output. For the less well-informed user this message is intended for, this warning could make them think there's something bad about the PSBT itself. Or the opposite(!): "I ran into this Verify Multisig Change error, so I rescanned the PSBT and made sure I told it to SKIP verification, but it must still be trying to verify because that error is still showing up." Implementation detail NACK: A View should not invoke additional Screen presentations to the user (i.e. should not have sequential see: Another consequence of the current implementation is that we can't generate a screenshot for this warning screen due to the way it's implemented. Misc: The warning text itself should not be using such inconsistent mixed capitalization: "Can't Verify that Change Outputs Belong to your Wallet". Our standardized approach everywhere else is to use normal sentence capitalization. I'm not sure "Security Warning" is quite the right phrase, but can't really articulate why. Alternate suggestion to consider: Then when we review each output, we wouldn't have to offer the "Verify" button nor this "Security Warning". We could simply display the state:
|
I agree with this feedback but I also think the 0.7.0 verbiage of "Next" in the Multisig change verification view does not correctly inform the user either.
I missed this, good catch. |
I regret merging this PR as is. I've created this follow up PR #549 to remove the parts merged in this PR I now think should have not been included. |
Remove DireWarning just added in PR #533
Description
Fix Always Wiping Multisig Descriptor at Main Menu
This pull request is categorized as a:
Checklist
pytest
and made sure all unit tests pass before sumbitting the PRIf you modified or added functionality/workflow, did you add new unit tests?
I have tested this PR on the following platforms/os:
Note: Keep your changes limited in scope; if you uncover other issues or improvements along the way, ideally submit those as a separate PR. The more complicated the PR the harder to review, test, and merge.