Audit Rewards server responses

[emerick]

Resolves brave/brave-browser#9666

Submitter Checklist:

• Submitted a ticket for my issue if one did not already exist.
• Used Github auto-closing keywords in the commit message.
• Added/updated tests for this change (for new code or code which already has tests).
• Verified that these changes build without errors on
  • Android
  • iOS
  • Linux
  • macOS
  • Windows
• Verified that these changes pass automated tests (unit, browser, security tests) on
  • iOS
  • Linux
  • macOS
  • Windows
• Verified that all lint errors/warnings are resolved (npm run lint)
• Ran git rebase master (if needed).
• Ran git rebase -i to squash commits (if needed).
• Tagged reviewers and labelled the pull request as needed.
• Request a security/privacy review as needed.
• Add appropriate QA labels (QA/Yes or QA/No) to include the closed issue in milestone
• Public documentation has been updated as necessary. For instance:
  • https://github.com/brave/brave-browser/wiki/Deviations-from-Chromium-(features-we-disable-or-remove)
  • https://github.com/brave/brave-browser/wiki/Proxy-redirected-URLs
  • https://github.com/brave/brave-browser/wiki/Fingerprinting-Protections
  • https://github.com/brave/brave-browser/wiki/Brave%E2%80%99s-Use-of-Referral-Codes
  • https://github.com/brave/brave-browser/wiki/Custom-Headers
  • https://github.com/brave/brave-browser/wiki/Web-compatibility-issues-with-tracking-protection
  • https://github.com/brave/brave-browser/wiki/QA-Guide

Test Plan:

No specific plan here, we just need to run through a general test of Rewards functionality with a focus on server communications.

Reviewer Checklist:

• New files have MPL-2.0 license header.
• Request a security/privacy review as needed.
• Adequate test coverage exists to prevent regressions
• Verify test plan is specified in PR before merging to source

After-merge Checklist:

• The associated issue milestone is set to the smallest version that the
  changes has landed on.
• All relevant documentation has been updated.

[emerick]

@husobee If you take a look at the response/response_*.cc files, you'll see the HTTP response codes that we currently handle on the client and the JSON that we expect back from the server. Figured this might help you visualize the current client support as you work on doc'ing the ledger response codes for us.

[NejcZdovc]

1. All header guards need to be changed in response folder
   BRAVELEDGER_COMMON_RESPONSE_API_H_ -> BRAVELEDGER_RESPONSE_RESPONSE_API_H_`

2. Functions in response folder should not have Response at the end of the functions as it's already in braveledger_response_util namespace

3. Some functions in response folder have Parse prefix, but they are not parsing anything, just reading HTTP_CODE. I would suggest that we rename them to something like Check prefix.

[zenparsing]

The response format documentation and parsing consolidations in this PR are great, and make it much easier to see our API client assumptions. Obviously keeping code comments in sync will be a challenge over time, but I think it's probably worth it in this case.

I'm less confident that moving all of this code out of its feature/responsibility area into a "response" folder is helpful, though. As it is, the code for accessing any particular HTTP service is already split out into a few different places:

• static_values.h
• The request folder
• The general "subject" area folder that uses it

And having a "response" folder just means one more place to split things out into. Having a structure like this makes it harder to see the whole picture, harder to learn, and makes it more difficult to do gradual refactors. One option might be to create a new "endpoints" folder (or similar) that groups together each endpoint as an entity and provides a fetch function for each one.

In my opinion, for now I think we should keep these response files together in the folder where they are used. What do you think?

[zenparsing]

I would prefer not merge any refactors of the publist code at this time (as there might be uplift requirements).

[zenparsing]

The troubles that I've had this week have convinced me that we should not be refactoring publisher list v4 files until the feature has stabilized in 1.12.

[NejcZdovc]

we receive RETRY_SHORT, but it's ignored

[emerick]

This code receives RETRY_SHORT as a suggestion from a previous code review, but I guess I'm not sure why we wanted to make that logic change in the first place. None of this particular code has ever handled RETRY_SHORT before, from what I can see. It seems like ParseSignedCreds should return RETRY to remain consistent with how things were before.

[emerick]

Spoke offline, I'll add a RETRY_SHORT handler to GetSignedCredsFromResponse.