Skip to content
This repository has been archived by the owner on Dec 11, 2019. It is now read-only.

Reconcile is no longer triggered if there are no publishers in the ledger table. #13385

Merged
merged 1 commit into from
Apr 11, 2018

Conversation

ryanml
Copy link
Contributor

@ryanml ryanml commented Mar 5, 2018

Fixes #13325

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).
  • 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. (Ask a Brave employee to help if you cannot access this document.)

Test Plan:

  1. Clean install 0.21.16, do not visit any site
  2. Enable payment and add funds to wallet
  3. Change reconcile stamp to older date.
  4. Confirm that the reconciliation does not process and that date is pushed back

Reviewer Checklist:

  • Request a security/privacy review as needed if one was not already requested.

Tests

  • Adequate test coverage exists to prevent regressions
  • Tests should be independent and work correctly when run individually or as a suite ref
  • New files have MPL2 license header

@ryanml
Copy link
Contributor Author

ryanml commented Mar 5, 2018

@srirambv @NejcZdovc

@ryanml ryanml changed the title Fixes Issue #13325, a reconcile is no longer triggered during run if there are no publishers in the ledger table. Reconcile is no longer triggered if there are no publishers in the ledger table. Mar 5, 2018
@bsclifton bsclifton requested a review from NejcZdovc March 5, 2018 05:22
@bsclifton bsclifton requested a review from LaurenWags March 5, 2018 05:22
@ryanml
Copy link
Contributor Author

ryanml commented Mar 5, 2018

@NejcZdovc let me know if this is a change that makes more sense to implement in to the bat-client itself, and not browser-side. Thanks!

@NejcZdovc
Copy link
Contributor

@ryanml well I think that solution is sadly not that simple. We need to define what actually happens in this case with contribution time should we push it back for 30 days or just fuzz it for couple of days. I think that if your table is empty I would push it back for 30 days. So we need to update reconcile timestamp.

@ryanml
Copy link
Contributor Author

ryanml commented Mar 15, 2018

@NejcZdovc makes complete sense.

Would we just be updating delayTime in this case or setting setTimeUntilReconcile via the client?

Either way, I'll work something out and we can review from there :)

@NejcZdovc
Copy link
Contributor

@ryanml we should set setTimeUntilReconcile

@ryanml ryanml force-pushed the no-reconcile-on-empty-pubs branch from 459b913 to 06f21a3 Compare March 16, 2018 22:52
@codecov-io
Copy link

codecov-io commented Mar 16, 2018

Codecov Report

Merging #13385 into master will decrease coverage by 0.14%.
The diff coverage is 93.75%.

@@            Coverage Diff             @@
##           master   #13385      +/-   ##
==========================================
- Coverage   56.78%   56.64%   -0.15%     
==========================================
  Files         285      285              
  Lines       29038    29034       -4     
  Branches     4803     4803              
==========================================
- Hits        16488    16445      -43     
- Misses      12550    12589      +39
Flag Coverage Δ
#unittest 56.64% <93.75%> (-0.15%) ⬇️
Impacted Files Coverage Δ
app/browser/api/ledger.js 60.46% <93.75%> (-1.46%) ⬇️
js/stores/appStoreRenderer.js 91.66% <0%> (-8.34%) ⬇️
app/renderer/components/reduxComponent.js 57.75% <0%> (-3.45%) ⬇️
js/stores/windowStore.js 28.46% <0%> (-0.3%) ⬇️
app/common/state/ledgerState.js 85.19% <0%> (+0.24%) ⬆️

@ryanml
Copy link
Contributor Author

ryanml commented Mar 19, 2018

@NejcZdovc I think we're a little closer to the desired result now

const publishers = ledgerState.getPublisher(state)
if (publishers.isEmpty()) {
const newReconcileTime = (new Date().getTime() + (ledgerUtil.milliseconds.day * 30))
client.setTimeUntilReconcile(newReconcileTime, (err, stateResult) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's extract this block into separate function and just call this function here. Also please replace this block in other places that you can find it with this new function. You don't need to pass newReconcileTime, you can just pass null in, which will default to 30 days.

@ryanml
Copy link
Contributor Author

ryanml commented Apr 5, 2018

@NejcZdovc changes pushed

@NejcZdovc NejcZdovc added this to the Completed work milestone Apr 6, 2018
@@ -2243,6 +2247,11 @@ const run = (state, delayTime) => {
return
}

const publishers = ledgerState.getPublisher(state)
if (publishers.isEmpty()) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's extend this check to if (publishers.isEmpty() && client.isReadyToReconcile(synopsis)) {. This way we are not pushing back time every time run is called, but we push it back only once every month.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@NejcZdovc good call, changes pushed

@NejcZdovc NejcZdovc self-requested a review April 6, 2018 05:16
@NejcZdovc
Copy link
Contributor

@ryanml let's just squash this and we are good to go

…n if there are no publishers in the ledger table.
@ryanml ryanml force-pushed the no-reconcile-on-empty-pubs branch from a67cba1 to 6d3f795 Compare April 6, 2018 05:46
@ryanml
Copy link
Contributor Author

ryanml commented Apr 6, 2018

@NejcZdovc squashed 👍 thanks for your guidance on this one!

Copy link
Contributor

@NejcZdovc NejcZdovc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me ++

@NejcZdovc
Copy link
Contributor

@ryanml sure, not a problem

@NejcZdovc NejcZdovc modified the milestones: Completed work, 0.22.x Release 2, 0.22.x Release 3 (Beta channel) Apr 10, 2018
@NejcZdovc NejcZdovc merged commit bc3262a into brave:master Apr 11, 2018
NejcZdovc added a commit that referenced this pull request Apr 11, 2018
Reconcile is no longer triggered if there are no publishers in the ledger table.
NejcZdovc added a commit that referenced this pull request Apr 11, 2018
Reconcile is no longer triggered if there are no publishers in the ledger table.
@NejcZdovc
Copy link
Contributor

master bc3262a
0.23 372c933
0.22 331000e

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants