Fix PaymentServer::handleURIOrFile warning message #225

1. 
   hebasto commented at 12:58 pm on February 24, 2021: member

   GUIUtil::parseBitcoinURI does not check address validity. Therefore, an invalid address cannot cause parsing failure.

   An invalid address case is covered by its own message.

   This PR fixes the warning messages.

2. hebasto renamed this:
   qt: Fix PaymentServer::handleURIOrFile warning message
   Fix PaymentServer::handleURIOrFile warning message
   on Feb 24, 2021
3. Talkless approved
4. 
   Talkless commented at 2:50 pm on February 28, 2021: none

   utACK 2434d004219f90f8d5408d22f222cfb7060565ac, no any testing, just built on Debian Sid with Qt 5.15.2
5. hebasto force-pushed on May 10, 2021
6. 
   hebasto commented at 10:26 pm on May 10, 2021: member

   Updated 2434d004219f90f8d5408d22f222cfb7060565ac -> 45ebd9e1825886767efc38ddc3e300c638fcae6c (pr225.01 -> pr225.02):

   • rebased on top of the #280
   • dropped all refactoring changes
7. 
   hebasto commented at 10:27 pm on May 10, 2021: member

   @prayank23

   Do you mind reviewing this PR?

8. hebasto added the label UX on May 10, 2021
9. 
   ghost commented at 0:47 am on May 11, 2021: none

   GUIUtil::parseBitcoinURI does not check address validity. Therefore, an invalid address cannot cause parsing failure.

   If address validity is not checked, can it be the cause for parsing failure?

   An invalid address case is covered by its own message “Invalid payment address %1”.

   Invalid address case is covered by IsValidDestination() and updated in https://github.com/bitcoin-core/gui/pull/280/ however, if code for else statement is executed can we assume address was never checked for validity?

   https://github.com/bitcoin-core/gui/blob/4a267057617a8aa6dc9793c4d711725df5338025/src/qt/paymentserver.cpp#L252

10. 
    hebasto commented at 4:36 pm on May 11, 2021: member

    GUIUtil::parseBitcoinURI does not check address validity. Therefore, an invalid address cannot cause parsing failure.

    If address validity is not checked, can it be the cause for parsing failure?

    Internally the GUIUtil::parseBitcoinURI uses the QUrlQuery class, so if an invalid address does not contain delimiters, it shouldn’t cause parsing failure.

    An invalid address case is covered by its own message “Invalid payment address %1”.

    Invalid address case is covered by IsValidDestination() and updated in #280 however, if code for else statement is executed can we assume address was never checked for validity?

    I think we can.

11. qt: Fix PaymentServer::handleURIOrFile warning message

    GUIUtil::parseBitcoinURI does not check address validity. Therefore, an
    invalid address cannot cause parsing failure.
    An invalid address case is covered by its own message "Invalid payment
    address %1".

    5b20d46f8b
12. hebasto force-pushed on May 29, 2021
13. 
    hebasto commented at 1:53 pm on May 29, 2021: member

    Rebased 45ebd9e1825886767efc38ddc3e300c638fcae6c -> 5b20d46f8be6b7fc08012d80a9fc42e9dbc812ff (pr225.02 -> pr225.03) on top of the recent CI changes.
14. Talkless approved
15. 
    Talkless commented at 6:49 pm on June 10, 2021: none

    re-utACK 5b20d46f8be6b7fc08012d80a9fc42e9dbc812ff after rebase.
16. 
    jarolrod commented at 4:26 am on July 6, 2021: member

    Code Review ACK 5b20d46f8be6b7fc08012d80a9fc42e9dbc812ff 🥃
17. 
    ghost commented at 11:47 am on July 6, 2021: none

    @hebasto Can you share any examples that I can try for which GUIUtil::parseBitcoinURI will return false and https://github.com/hebasto/gui/blob/5b20d46f8be6b7fc08012d80a9fc42e9dbc812ff/src/qt/paymentserver.cpp#L254 will be executed ?
18. 
    hebasto commented at 3:44 pm on July 8, 2021: member

    @hebasto Can you share any examples that I can try for which GUIUtil::parseBitcoinURI will return false and https://github.com/hebasto/gui/blob/5b20d46f8be6b7fc08012d80a9fc42e9dbc812ff/src/qt/paymentserver.cpp#L254 will be executed ?

    I can’t. It looks like this code is never executed due to checks in earlier stages, e.g. in PaymentServer::ipcParseCommandLine.

    Closing for now.

19. hebasto closed this on Jul 8, 2021

    ---

20. bitcoin-core locked this on Aug 16, 2022

Contributors
hebasto Talkless ghost jarolrod

Labels
UX