[Qt] payment request / server work - part 2 #5467
Conversation
|
Seems I used some Qt5 stuff, need to check ;). |
|
I'd love to see that reviewed or ACKd @laanwj @gavinandresen, mind taking a look? |
There was a problem hiding this comment.
This method is unused. Is there a plan to make use of it soon? IMO we should keep unused implementation in master at minimum. Better keep this in a separate branch and add it if we use it.
There was a problem hiding this comment.
Nevermind.
You removed it. Sorry.
|
Needs rebase. |
|
It's not related to this pull, but better here than in a new issue: |
|
@jonasschnelli Rebased As for the cert problems with gavins php tool, IMHO you won't get any authenticated payment request from it and consensus then is to silently treat it as valid unauthenticated request. I had that discussion with @laanwj @gavinandresen and @mikehearn ;). |
|
Tested, reviewed, stepped. ACK |
|
I did each change in a seperate commit to make it easy to review :). |
|
Does anyone have a test plan for this? |
|
@luke-jr Are you serious?? |
src/qt/paymentrequestplus.cpp
Outdated
There was a problem hiding this comment.
NACK on changing this. There is check for certs.empty() that prevents this from becoming a bug, but for loops that count down, I generally prefer to use signed types. In this case, i will never visit 0, but normally a loop like this would look like:
for (int i = certs.size() - 1; i >= 0; i--) {
If you then change the int to e.g. size_t, it will loop forever.
|
@jonasschnelli If you've got an idea how to group the commits so they can get merged I'll gladly do this. Guess this needs an ACK from @laanwj still :). |
|
@laanwj @jonasschnelli Is there some consensus possible to at least pick most of these commits or some, so we can get on here!? |
|
I'd like to improve payment request stuff, but not getting on with this is blocking, you know ;). |
|
ACK apart from the char* change. |
- also add a few more comments in PaymentServer::LoadRootCAs
Before and after was tested in Windows:
before:
GUI: ReportInvalidCertificate : Payment server found
an invalid certificate: ("Microsoft Authenticode(tm) Root Authority")
GUI: ReportInvalidCertificate : Payment server found
an invalid certificate: ()
GUI: ReportInvalidCertificate : Payment server found
an invalid certificate: ()
GUI: ReportInvalidCertificate : Payment server found
an invalid certificate: ()
after:
GUI: ReportInvalidCertificate: Payment server found an
invalid certificate: "01" ("Microsoft Authenticode(tm) Root Authority")
() ()
GUI: ReportInvalidCertificate: Payment server found an
invalid certificate: "01" () () ("Copyright (c) 1997 Microsoft Corp.",
"Microsoft Time Stamping Service Root", "Microsoft Corporation")
GUI: ReportInvalidCertificate: Payment server found an
invalid certificate: "4a:19:d2:38:8c:82:59:1c:a5:5d:73:5f:15:5d:dc:a3" ()
() ("NO LIABILITY ACCEPTED, (c)97 VeriSign, Inc.", "VeriSign Time Stamping
Service Root", "VeriSign, Inc.")
GUI: ReportInvalidCertificate: Payment server found an
invalid certificate: "e4:9e:fd:f3:3a:e8:0e:cf:a5:11:3e:19:a4:24:02:32" ()
() ("Class 3 Public Primary Certification Authority")
- used in sendcoinsdialog.cpp and paymentserver.cpp - removes an unneded translation string
|
@laanwj Thanks for review, I fixed the spelling error and remove the QVariant commit, should be ready now :). |
6171e49 [Qt] Use identical strings for expired payment request message (Philip Kaufmann) 06087bd [Qt] minor comment updates in PaymentServer (Philip Kaufmann) 35d1595 [Qt] constify first parameter of processPaymentRequest() (Philip Kaufmann) 9b14aef [Qt] take care of a missing typecast in PaymentRequestPlus::getMerchant() (Philip Kaufmann) d19ae3c [Qt] remove unused PaymentRequestPlus::getPKIType function (Philip Kaufmann) 6e17a74 [Qt] paymentserver: better logging of invalid certs (Philip Kaufmann) 5a53d7c [Qt] paymentserver: do not log NULL certificates (Philip Kaufmann)
5 participants
This continues my work on the payment request / server part of our BIP70 implementation.
Please see individual commits for detailed descriptions.