newtonick
commented
5 months ago ACK and Tested
Closed 3rdIteration closed 5 months ago
newtonick
commented
5 months ago ACK and Tested
kdmukai
commented
5 months ago 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 self.run_screen() calls). Wherever possible we should maintain the strict discipline of Views being focused on just one task to keep the routing and history logic as clean and simple as possible.
see: PSBTUnsupportedScriptTypeWarningView, PSBTNoChangeWarningView, and others.
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: I'm not sure there'd be enough room for a clear enough explanation, but I think I'd be more in favor of a gentler info screen BEFORE any verification steps (before we review each output) that somehow quickly explains why you should load the multisig descriptor. Load or Skip on that screen.
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:
newtonick
commented
5 months ago 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.
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.
Implementation detail NACK: A View should not invoke additional Screen presentations to the user (i.e. should not have sequential self.run_screen() calls). Wherever possible we should maintain the strict discipline of Views being focused on just one task to keep the routing and history logic as clean and simple as possible.
see: PSBTUnsupportedScriptTypeWarningView, PSBTNoChangeWarningView, and others.
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.
I missed this, good catch.
newtonick
commented
5 months ago 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.
Description
Fix Always Wiping Multisig Descriptor at Main Menu
This pull request is categorized as a:
Checklist
pytestand 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.