[Refactor] Combine parameter checking of budget commands

[CaveSpectre11]

Release notes

• [Refactor] Move common budget parameter checking code into a function

Details

The code in both preparebudget and submitbudget used the same parameter checking. This pulls that parameter checking and loading into a common routine used by both; for ease of improvements and adjustments to the parameter checking... so the risk of not adjusting them consistently in both routines is removed.

Note

The early discussion below is based on a combined PR that addressed this, plus #950 and a PR to be named later. The decision was made to split and focus; so the discussions can be independent of each other and refactor as needed.

[random-zebra]

First of all, thanks for your continuing efforts to improve PIVX code.
The quality of your work is definitely appreciated.

I like the proposed changes (I have a question after code inspection, will test it asap).

One minor thing, I think it would be slightly better (from a styling and performance point of view) to include the following variables (either as references or pointers) directly as arguments in checkBudgetInputs and parse them there from UniValue& params:

std::string strProposalName,
std::string strURL,
int nPaymentCount,
int nBlockStart,
CBitcoinAddress address,
CAmount nAmount

[CaveSpectre11]

One minor thing, I think it would be slightly better (from a styling and performance point of view) to include the following variables (either as references or pointers) directly as arguments in checkBudgetInputs and parse them there from UniValue& params:

It was one thing I was considering; since I don't like pulling them out in the routine, and then pulling them out again. Now that I ponder it more; since the routine is pretty specific to exactly the parameters that are required; it probably does make sense to do that.

[Fuzzbawls]

Some thoughts on this particular PR:

• Its doing too much (optimizing existing code and adding new functionality at the same time)
• As convenient as Boost may be, we've set a precedent on trying to reduce it's usage, not add.
• Do we really want to try to account for all these kinds of letter casing situations, or do we just want to be strict about it and say that (for our purposes) only lowercase http:// and https:// is considered "valid"?

Personally, i'm ok with being uber strict on the URL scheme character casing. Saves from having to rely on boost or a locale dependent function, as well as many lines of code. I'm pretty sure that most browsers and URL shortening websites end up with the standard lowercase scheme in a users clipboard when copying, so someone would actually have to go out of their way to end up with all upper or mixed casing in she URL scheme.

[random-zebra]

I think it makes sense to have both refactoring and new functionality in the same place, here, given the narrow focus of the PR. Maybe just split the commits.

But, after pondering some more, I agree on the other points.
Accounting for anything else other than plain lowercase is probably overkill here.

[Fuzzbawls]

@random-zebra I brought up the refactor + new functionality issue because I think everyone can agree that refactoring out the duplicate code is a good thing, but the new added functionality here is clearly still controversial and no consensus has been reached on the best way to proceed with it.

For this particular situation, it shouldn't be too big of a deal as @CaveSpectre11 is very much active and open to the idea of multiple change requests, but it is good practice (and actually stated in our contributing guidelines) in general to not mix refactorings with functional changes.

[CaveSpectre11]

Some thoughts on this particular PR:

• Do we really want to try to account for all these kinds of letter casing situations, or do we just want to be strict about it and say that (for our purposes) only lowercase http:// and https:// is considered "valid"?

I'm thinking including Http and Https might be beneficial; since a lot of phones make it a hassle to get your first character to not be an uppercase.

e..g. boil down to:

std::string validateURL(std::string strURL) {

    // Check URL size
    if (strURL.size() > 64)
        return std::string("Invalid url, limit of 64 characters.");

    std::vector<std::string> reqPre;

    // Required initial strings; URL must contain one
    reqPre.push_back("http://");
    reqPre.push_back("https://");
    reqPre.push_back("Http://");
    reqPre.push_back("Https://");

    // check fronts
    bool found = false;
    for (int i=0; i < reqPre.size() && !found; i++) {
        if (strURL.find(reqPre[i]) == 0) found = true;
    }
    if ((!found) && (reqPre.size() > 0))
        return std::string("Invalid URL, check scheme (e.g. https://)");

    return std::string("");
}

And expand on it at a later date if more checking is required.

[Fuzzbawls]

I think that if someone is advanced enough to manage to interact with a core wallet's RPC interface from their phone, they can ensure the URL scheme is in standard lowercase themselves :P

Also, even phone browsers end up re-writing the URL scheme to standard lowercase when resolving/loading a URL, so copying to a phone's clipboard would still satisfy the all lowercase requirement.

[CaveSpectre11]

The code has been following the conversation; so it's pretty much good. What I'll do is push this one for the refactoring; and push the other one for the validateURL (after finding where best to move it), then just do the rebase when the time comes. The remaining question falls on the crash fixes... keep them in here, move them to the other one, or have them as a 3rd PR?

[Fuzzbawls]

Yeah I think that since the issue of how best to handle the URL scheme is still in debate, this PR can be changed to include just a code refactor of what currently exists in master (and renamed appropriately).

From there #950 can be retained for the purpose of enforcing whatever method of URL scheme restriction is eventually decided on.

Also, to keep things as atomic as possible, yes a new PR can be opened which is specifically to address the crash fix only.

[CaveSpectre11]

PR is refactored to be just the refactor ;)

[random-zebra]

ACK 7834c5e and tested

getbudgetinfo
[
...
   {
      "Name": "PR964_testing",
      ...
      "fValid": true
   }
]

[Fuzzbawls]

needs a quick rebase, looks like #951 may have caused a small conflict

[CaveSpectre11]

needs a quick rebase, looks like #951 may have caused a small conflict

done

[Fuzzbawls]

ACK 983d97a

[random-zebra]

ACK 983d97a