Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Payment "receipts" cashed in between the 1st and 5th of the month do not appear in ad totals #4330

Closed
jsecretan opened this issue May 7, 2019 · 8 comments · Fixed by brave/brave-core#2609

Comments

@jsecretan
Copy link

jsecretan commented May 7, 2019

Description

Take the following case:

  1. A user signs up for Brave Ads on 4/25.
  2. Every day until 4/30 (6 days), they see 4 ads, at 0.25BAT/each
  3. Payment tokens/receipts are cashed in 5/2.
  4. Every day from 5/1 until EOD 5/4 (4 days), he sees 4 ads, at 0.25BAT/each
  5. The user wakes up and logs in on 5/5.

Because the user is supposed to get paid for payment tokens cashed in from 4/1 to EOD 4/30, he will not have anything coming to him for the payout on 5/5. Because of this, his settings view should have 4 * 6 + 4 * 4 = 40 views, 40 * 0.25 = 10 BAT on the morning of 5/5. The user is not seeing the BAT from 4/25 to 4/30 in the totals because those were “cashed in” before 5/5.

Steps to Reproduce

  1. Roll clock to 4/25
  2. View ads
  3. Roll forward a week to cash in
  4. Roll clock to 5th

Actual result:

The user is not seeing total views and BAT accumulated from ads seen 4/25 to 4/30.

Expected result:

The user is not seeing total views and BAT accumulated from ads seen 4/25 to 4/30 if those are not going to be included in his next payout on 5/5.

Reproduces how often:

Brave version (brave://version info)

0.63.55.

Version/Channel Information:

All channels

@tmancey
Copy link
Contributor

tmancey commented May 7, 2019

Proposed Solution 1

Ads Serve Changes:

Update POST /confirmation/payment/{paymentId} Ads Serve call which is responsible for token redemption to return earnings in the JSON response if the returned status is ok, i.e:

{
  "status": {
    "status": "ok"
  },
  "adsEarnings": {
    "currentCycle": {
      "balance": "0.55",
      "transactionCount": "11"
    },
    "due": {
      "balance": "7.25",
      "transactionCount": "145"
    }
  }
}

"currentCycle" are tokens redeemed between the 1st of the current month and present day.

"due" are tokens redeemed on or before the last day of the previous month which will be paid out in an Ads grant on the 5th of the month.

These values will be cached locally in Defaults/rewards_service/confirmations.json.

Client-Side Changes:

Estimated earnings this cycle which can be calculated with "adsEarnings/currentCycle/balance" + Transaction history since last successful redemption.

Payment date can be contextual in that between the 1st and 5th of the month show Ads grant of 1.25 BAT is due for payment on the 5th which can be retrieved from "adsEarnings/due/balance", then between the 6th and the last day of the month show Payment date Monthly, 5th.

Ad notifications received which can be calculated with "adsEarnings/currentCycle/transactionCount" + Count of transaction history since last successful redemption (if estimated redemption value is greater than 0).

Pros:

  • Server already has all information to provided the necessary data
  • Users will see the Ads grant total due for payment on the 5th (highly requested by the community)
  • Users will see accurate Estimated earnings for this cycle and Ad notifications received even if tokens were redeemed after the end of month cut off date
  • If a user deletes Defaults/rewards_service/confirmations.json (or their user profile) only unredeemed tokens will be lost (which may be planned to be resolved with Sync)
  • After upgrading, the Ads panel UI would reflect the correct values (after the next token redemption date which is made every ~7 days (however if it is agreed we could force a one off redemption within ~1 day or less)
  • If the server failed to deliver an Ads grant the Ads panel UI would reflect the correct values
  • We would only need to sync unredeemed tokens

Cons:

  • Potential privacy concerns. @NejcZdovc has confirmed that Ledger retrieves balances from the server (which are not encrypted)
  • If a user deletes Defaults/rewards_service/confirmations.json (or their user profile) all unredeemed tokens will be lost

Questions:

@jsecretan jsecretan added feature/ads priority/P1 A very extremely bad problem. We might push a hotfix for it. labels May 7, 2019
@jsecretan
Copy link
Author

The alternative here is to cache the the timestamp of cashing in payment receipts within the confirmations object and using that to determine the starting timestamp. The downside is that for anybody already affected, nobody's balance will pop back into view.

@tmancey
Copy link
Contributor

tmancey commented May 7, 2019

Proposed Solution 2

Client-Side Only Changes:

Add redemption_timestamp_in_seconds optional key-value pair to transactions key to persist the current time in seconds when tokens are successfully redeemed with the Ads Serve.

{
  "transaction_history": {
    "transactions": [
      {
        "confirmation_type": "view",
        "estimated_redemption_value": 0.05,
        "timestamp_in_seconds": "13200508360",
        "redemption_timestamp_in_seconds": "13200512000"
      },
      {
        "confirmation_type": "click",
        "estimated_redemption_value": 0,
        "timestamp_in_seconds": "13200508380",
        "redemption_timestamp_in_seconds": "13200512000"
      }
    ]
  }
}

Estimated earnings this cycle which can be calculated with Estimated redemption value of all unredeemed tokens + Transaction history for tokens redeemed between the 1st of the current month and present day.

Payment date can be contextual in that between the 1st and 5th of the month show Ads grant of 1.25 BAT is due for payment on the 5th which can be calculated with Transaction history for tokens redeemed for the previous month, then between the 6th and the last day of the month show Payment date Monthly, 5th.

Ad notifications received which can be calculated with Count of all unredeemed tokens (if estimated redemption value is greater than 0) + Count of transaction history for tokens redeemed between the 1st of the current month and present day (if estimated redemption value is greater than 0).

Pros:

  • Users will see the Ads grant total due for payment on the 5th (highly requested by the community)
  • Users will see Estimated earnings for this cycle and Ad notifications received even if tokens were redeemed after the end of month cut off date

Cons:

  • If the server fails to cash-out redeemed tokens the Ads grant total would be out of sync
  • After upgrading, the Ads panel UI will not reflect the correct values for legacy transactions
  • If a user deletes Defaults/rewards_service/confirmations.json (or their user profile) all transaction history will be lost and the Ads panel UI will be reset to 0

@jsecretan
Copy link
Author

jsecretan commented May 20, 2019

So, per @davidtemkin, the "fix" here will also involve modifying the way the display conveys what has been paid so far and what will be paid in the future:

  1. "Current earnings this month" becomes "Estimated pending earnings".
  2. "Payment date" becomes "Next payment date" and shows an actual date instead of a day of the month. It actively calculates the next payment date by being aware of what is cashed in so far.
  3. "Ad notifications received" becomes "Ad notifications received this month", locally counts and resets monthly

@jsecretan
Copy link
Author

Calculating the next payment date should look as follows. When the browser calls to get total redeemed payment tokens, it will have that broken out by month. So, depending on the time of the month:

1st-5th (23:59): If last month had cashed in tokens > 0, then show next payment date as this month.
6th-end of month: If this month has any cashed in payment tokens, then show next payout date as next month.
Else if this month has no cashed in payment tokens, but calculated next cash in date is still this month, then show next payout date as next month.
Else if this month has no cashed in payment tokens, and calculated next cash in date is not in this month, then next payment date is month after next.

So example if it is the 15th of May, and no cashed in tokens, and next cash in date is May 22nd, next payment date is 5th of June.
But if it is the 28th of May, no cashed in tokens and next cash in date is 5th of June, next payment date would say 5th of July.

@jenn-rhim
Copy link

jenn-rhim commented May 29, 2019

Here is the UI reflecting the changes. Please note that the 'ads earnings' are to be 'ads rewards'. @mandar-brave @jsecretan @rebron

Screen Shot 2019-05-29 at 11 54 22 AM

@jsecretan
Copy link
Author

So @jenn-rhim just as a note, we are going to leave off the "Ad pages viewed this month" for another story. We had left that off originally to save time and complexity, and want to make sure this story is as focused as possible.

@srirambv
Copy link
Contributor

srirambv commented Jun 27, 2019

  • Verification passed on
Brave 0.66.99 Chromium: 75.0.3770.100 (Official Build) (64-bit)
Revision cd0b15c8b6a4e70c44e27f35c37a4029bad3e3b0-refs/branch-heads/3770@{#1033}
OS Linux

Verified passed with

Brave 0.66.99 Chromium: 75.0.3770.100 (Official Build) (64-bit)
Revision cd0b15c8b6a4e70c44e27f35c37a4029bad3e3b0-refs/branch-heads/3770@{#1033}
OS Mac OS X

Verification passed on

Brave 0.66.99 Chromium: 75.0.3770.100 (Official Build) (64-bit)
Revision cd0b15c8b6a4e70c44e27f35c37a4029bad3e3b0-refs/branch-heads/3770@{#1033}
OS Windows 10 OS Version 1803 (Build 17134.523)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment