From b96a73f302aff50d6395e866ddbafc9d1d28ce12 Mon Sep 17 00:00:00 2001 From: NejcZdovc Date: Thu, 14 Dec 2017 15:47:24 +0100 Subject: [PATCH] Improves verified-caching Resolves #12272 Auditors: Test Plan: --- app/browser/api/ledger.js | 132 +++++++++++++++-------- app/browser/reducers/ledgerReducer.js | 4 + docs/state.md | 26 ++--- js/actions/appActions.js | 5 +- test/unit/app/browser/api/ledgerTest.js | 133 ++++++++++++++++++++++-- 5 files changed, 232 insertions(+), 68 deletions(-) diff --git a/app/browser/api/ledger.js b/app/browser/api/ledger.js index 1b92f48983d..684f2d4f252 100644 --- a/app/browser/api/ledger.js +++ b/app/browser/api/ledger.js @@ -73,6 +73,7 @@ let balanceTimeoutId = false let runTimeoutId let promotionTimeoutId let togglePromotionTimeoutId +let verifiedTimeoutId = false // Database let v2RulesetDB @@ -149,12 +150,24 @@ const paymentPresent = (state, tabId, present) => { if (!balanceTimeoutId) { module.exports.getBalance(state) } + + getPublisherTimestamp(true) } else if (balanceTimeoutId) { clearTimeout(balanceTimeoutId) balanceTimeoutId = false } } +const getPublisherTimestamp = (updateList) => { + client.publisherTimestamp((err, result) => { + if (err) { + console.error('Error while retrieving publisher timestamp', err.toString()) + return + } + appActions.onPublisherTimestamp(result.timestamp, updateList) + }) +} + const addFoundClosed = (state) => { if (balanceTimeoutId) { clearTimeout(balanceTimeoutId) @@ -185,13 +198,8 @@ const onBootStateFile = (state) => { try { clientprep() client = ledgerClient(null, underscore.extend({roundtrip: roundtrip}, clientOptions), null) - client.publisherTimestamp((err, result) => { - if (err) { - console.error('Error while retrieving publisher timestamp', err.toString()) - return - } - appActions.onPublisherTimestamp(result.timestamp) - }) + + getPublisherTimestamp() } catch (ex) { state = ledgerState.resetInfo(state) bootP = false @@ -540,21 +548,26 @@ const inspectP = (db, path, publisher, property, key, callback) => { // TODO rename function name const verifiedP = (state, publisherKey, callback) => { - client.publisherInfo(publisherKey, (err, result) => { + const clientCallback = (err, result) => { if (err) { console.error(`Error verifying publisher ${publisherKey}: `, err.toString()) return } if (callback) { - let data = false - if (result && result.properties && result.properties.verified) { - data = result.properties.verified + if (result) { + callback(null, result) + } else { + callback(err, {}) } - - callback(null, data) } - }) + } + + if (Array.isArray(publisherKey)) { + client.publishersInfo(publisherKey, clientCallback) + } else { + client.publisherInfo(publisherKey, clientCallback) + } if (process.env.NODE_ENV === 'test') { ['brianbondy.com', 'clifton.io'].forEach((key) => { @@ -689,30 +702,50 @@ const saveVisit = (state, publisherKey, options) => { }) state = ledgerState.setPublisher(state, publisherKey, synopsis.publishers[publisherKey]) state = updatePublisherInfo(state) - state = checkVerifiedStatus(state, publisherKey) + state = module.exports.checkVerifiedStatus(state, publisherKey) return state } -const checkVerifiedStatus = (state, publisherKey) => { - if (publisherKey == null) { +const checkVerifiedStatus = (state, publisherKeys, publisherTimestamp) => { + if (publisherKeys == null) { return state } - const lastUpdate = parseInt(ledgerState.getLedgerValue(state, 'publisherTimestamp')) - const lastPublisherUpdate = parseInt(ledgerState.getPublisherOption(state, publisherKey, 'verifiedTimestamp') || 0) + if (!Array.isArray(publisherKeys)) { + publisherKeys = [publisherKeys] + } + + const checkKeys = [] + const lastUpdate = parseInt(publisherTimestamp || ledgerState.getLedgerValue(state, 'publisherTimestamp')) - if (lastUpdate > lastPublisherUpdate) { - state = module.exports.verifiedP(state, publisherKey, (error, result) => { - if (!error) { - appActions.onPublisherOptionUpdate(publisherKey, 'verified', result) - appActions.onPublisherOptionUpdate(publisherKey, 'verifiedTimestamp', lastUpdate) - savePublisherOption(publisherKey, 'verified', result) - savePublisherOption(publisherKey, 'verifiedTimestamp', lastUpdate) - } - }) + publisherKeys.forEach(key => { + const lastPublisherUpdate = parseInt(ledgerState.getPublisherOption(state, key, 'verifiedTimestamp') || 0) + + if (lastUpdate > lastPublisherUpdate) { + checkKeys.push(key) + } + }) + + if (checkKeys.length === 0) { + return state } + state = module.exports.verifiedP(state, checkKeys, (error, result) => { + if (!error) { + const publisherKey = result.publisher + + if (result && result.properties && result.properties) { + const verified = !!result.properties.verified + appActions.onPublisherOptionUpdate(publisherKey, 'verified', verified) + savePublisherOption(publisherKey, 'verified', verified) + } + + appActions.onPublisherOptionUpdate(publisherKey, 'verifiedTimestamp', lastUpdate) + savePublisherOption(publisherKey, 'verifiedTimestamp', lastUpdate) + } + }) + return state } @@ -1748,12 +1781,18 @@ const initialize = (state, paymentsEnabled) => { ledgerNotifications.init() + if (verifiedTimeoutId) { + clearInterval(verifiedTimeoutId) + } + if (!paymentsEnabled) { client = null newClient = false return ledgerState.resetInfo(state, true) } + verifiedTimeoutId = setInterval(getPublisherTimestamp, 1 * ledgerUtil.milliseconds.hour) + if (client) { return state } @@ -1838,13 +1877,8 @@ const onInitRead = (state, parsedData) => { client = ledgerClient(parsedData.personaId, underscore.extend(parsedData.options, {roundtrip: roundtrip}, options), parsedData) - client.publisherTimestamp((err, result) => { - if (err) { - console.error('Error while retrieving publisher timestamp', err.toString()) - return - } - appActions.onPublisherTimestamp(result.timestamp) - }) + + getPublisherTimestamp(true) // Scenario: User enables Payments, disables it, waits 30+ days, then // enables it again -> reconcileStamp is in the past. @@ -2272,13 +2306,7 @@ const transitionWalletToBat = () => { })) appActions.onBitcoinToBatTransitioned() ledgerNotifications.showBraveWalletUpdated() - client.publisherTimestamp((err, result) => { - if (err) { - console.error('Error while retrieving publisher timestamp', err.toString()) - return - } - appActions.onPublisherTimestamp(result.timestamp) - }) + getPublisherTimestamp() } }) } catch (ex) { @@ -2508,6 +2536,21 @@ const onPromotionResponse = (state, status) => { return state } +const onPublisherTimestamp = (state, oldTimestamp, newTimestamp) => { + if (oldTimestamp === newTimestamp) { + return + } + + const publishers = ledgerState.getPublishers(state) + if (publishers.isEmpty()) { + return + } + + publishers.forEach((publisher, key) => { + module.exports.checkVerifiedStatus(state, key, newTimestamp) + }) +} + const getMethods = () => { const publicMethods = { backupKeys, @@ -2550,7 +2593,9 @@ const getMethods = () => { claimPromotion, onPromotionResponse, getBalance, - getPromotion + getPromotion, + onPublisherTimestamp, + checkVerifiedStatus } let privateMethods = {} @@ -2584,7 +2629,6 @@ const getMethods = () => { }, getCurrentMediaKey: (key) => currentMediaKey, synopsisNormalizer, - checkVerifiedStatus, roundtrip, observeTransactions, onWalletRecovery, diff --git a/app/browser/reducers/ledgerReducer.js b/app/browser/reducers/ledgerReducer.js index abd114e4f6e..f444e0eb6ba 100644 --- a/app/browser/reducers/ledgerReducer.js +++ b/app/browser/reducers/ledgerReducer.js @@ -402,7 +402,11 @@ const ledgerReducer = (state, action, immutableAction) => { } case appConstants.APP_ON_PUBLISHER_TIMESTAMP: { + const oldValue = ledgerState.getLedgerValue(state, 'publisherTimestamp') state = ledgerState.setLedgerValue(state, 'publisherTimestamp', action.get('timestamp')) + if (action.get('updateList')) { + ledgerApi.onPublisherTimestamp(state, oldValue, action.get('timestamp')) + } break } case appConstants.APP_SAVE_LEDGER_PROMOTION: diff --git a/docs/state.md b/docs/state.md index e48d6b4d24b..3ebaf51ada9 100644 --- a/docs/state.md +++ b/docs/state.md @@ -186,19 +186,19 @@ AppStore }, info: { addresses: { - BAT: string, - BTC: string, - CARD_ID: string, - ETH: string, + BAT: string, + BTC: string, + CARD_ID: string, + ETH: string, LTC: string }, balance: number, // confirmed balance in BAT.toFixed(2) bravery: { - days: number, + days: number, fee: { amount: number, currency: string - }, + }, setting: string }, converted: string, @@ -210,9 +210,9 @@ AppStore paymentId: string, probi: number, rates:{ - BTC: string, - ETH: number, - EUR: number, + BTC: string, + ETH: number, + EUR: number, USD: number }, reconcileFrequency: number // duration between each reconciliation in days @@ -265,7 +265,7 @@ AppStore options: { persist: boolean, style: string - } + } }, panel: { optInMarkup: { @@ -288,7 +288,7 @@ AppStore options: { persist: boolean, style: string - } + } }, panel: { disclaimer: string, @@ -313,7 +313,7 @@ AppStore options: { persist: boolean, style: string - } + } }, panel: { disclaimer: string, @@ -348,7 +348,7 @@ AppStore options: { exclude: boolean, verified: boolean, - verifiedTimestamp: number, // timestamp of the last change + verifiedTimestamp: number, // timestamp of the last change stickyP: boolean }, pinPercentage: number, diff --git a/js/actions/appActions.js b/js/actions/appActions.js index be8a770b205..70d34da97c2 100644 --- a/js/actions/appActions.js +++ b/js/actions/appActions.js @@ -1826,10 +1826,11 @@ const appActions = { }) }, - onPublisherTimestamp: function (timestamp) { + onPublisherTimestamp: function (timestamp, updateList) { dispatch({ actionType: appConstants.APP_ON_PUBLISHER_TIMESTAMP, - timestamp + timestamp, + updateList }) }, diff --git a/test/unit/app/browser/api/ledgerTest.js b/test/unit/app/browser/api/ledgerTest.js index 9ea9d477799..b6bb68de33d 100644 --- a/test/unit/app/browser/api/ledgerTest.js +++ b/test/unit/app/browser/api/ledgerTest.js @@ -47,6 +47,7 @@ describe('ledger api unit tests', function () { let ledgersetPromotionSpy let ledgergetPromotionSpy let ledgerSetTimeUntilReconcile + let onPublisherOptionUpdate const defaultAppState = Immutable.fromJS({ cache: { @@ -91,6 +92,7 @@ describe('ledger api unit tests', function () { onLedgerCallbackSpy = sinon.spy(appActions, 'onLedgerCallback') onBitcoinToBatBeginTransitionSpy = sinon.spy(appActions, 'onBitcoinToBatBeginTransition') onChangeSettingSpy = sinon.spy(appActions, 'changeSetting') + onPublisherOptionUpdate = sinon.spy(appActions, 'onPublisherOptionUpdate') // default to tab state which should be tracked tabState = tabState.setIn(['navigationState', 'activeEntry'], { @@ -489,18 +491,43 @@ describe('ledger api unit tests', function () { describe('checkVerifiedStatus', function () { let verifiedPSpy + let returnValue = false before(function () { + onPublisherOptionUpdate.reset() verifiedPSpy = sinon.spy(ledgerApi, 'verifiedP') + ledgerApi.setClient({ - publisherInfo: function () { - return false + publisherInfo: function (publisherKey, callback) { + callback(null, { + publisher: 'test.io', + properties: { + verified: returnValue + } + }) + }, + publishersInfo: function (publisherKey, callback) { + publisherKey.forEach(key => { + callback(null, { + publisher: key, + properties: { + verified: returnValue + } + }) + }) } }) }) + afterEach(function () { + returnValue = false + verifiedPSpy.reset() + onPublisherOptionUpdate.reset() + }) + after(function () { verifiedPSpy.restore() + onPublisherOptionUpdate.restore() ledgerApi.setClient(undefined) }) @@ -513,22 +540,54 @@ describe('ledger api unit tests', function () { it('only update if timestamp is older then current', function () { const newState = defaultAppState .setIn(['ledger', 'publisherTimestamp'], 20) - .setIn(['ledger', 'synopsis', 'publishers', 'clifton.io', 'options', 'verifiedTimestamp'], 20) - const result = ledgerApi.checkVerifiedStatus(newState, 'clifton.io') + .setIn(['ledger', 'synopsis', 'publishers', 'test.io', 'options', 'verifiedTimestamp'], 20) + const result = ledgerApi.checkVerifiedStatus(newState, 'test.io') assert.deepEqual(result.toJS(), newState.toJS()) assert(verifiedPSpy.notCalled) }) it('update when timestamp is older', function () { + returnValue = true const newState = defaultAppState .setIn(['ledger', 'publisherTimestamp'], 20) - .setIn(['ledger', 'synopsis', 'publishers', 'clifton.io', 'options', 'verifiedTimestamp'], 10) + .setIn(['ledger', 'synopsis', 'publishers', 'test.io', 'options', 'verifiedTimestamp'], 10) - const expectedState = newState - .setIn(['ledger', 'synopsis', 'publishers', 'clifton.io', 'options', 'verified'], true) - const result = ledgerApi.checkVerifiedStatus(newState, 'clifton.io') - assert.deepEqual(result.toJS(), expectedState.toJS()) + const result = ledgerApi.checkVerifiedStatus(newState, 'test.io') + assert.deepEqual(result.toJS(), newState.toJS()) assert(verifiedPSpy.calledOnce) + assert(onPublisherOptionUpdate.withArgs('test.io', 'verified', true).calledOnce) + }) + + it('change publisher verified status from true to false', function () { + const newState = defaultAppState + .setIn(['ledger', 'publisherTimestamp'], 20) + .setIn(['ledger', 'synopsis', 'publishers', 'test.io', 'options', 'verifiedTimestamp'], 10) + .setIn(['ledger', 'synopsis', 'publishers', 'test.io', 'options', 'verified'], true) + + const result = ledgerApi.checkVerifiedStatus(newState, 'test.io') + assert.deepEqual(result.toJS(), newState.toJS()) + assert(verifiedPSpy.calledOnce) + assert(onPublisherOptionUpdate.withArgs('test.io', 'verified', false).calledOnce) + }) + + it('handle multiple publishers', function () { + const newState = defaultAppState + .setIn(['ledger', 'publisherTimestamp'], 20) + .setIn(['ledger', 'synopsis', 'publishers', 'test.io', 'options', 'verifiedTimestamp'], 10) + .setIn(['ledger', 'synopsis', 'publishers', 'test1.io', 'options', 'verifiedTimestamp'], 15) + .setIn(['ledger', 'synopsis', 'publishers', 'test2.io', 'options', 'verifiedTimestamp'], 20) + .setIn(['ledger', 'synopsis', 'publishers', 'test3.io', 'options', 'verifiedTimestamp'], 30) + + const result = ledgerApi.checkVerifiedStatus(newState, [ + 'test1.io', + 'test2.io', + 'test3.io', + 'test.io' + ]) + assert.deepEqual(result.toJS(), newState.toJS()) + assert(verifiedPSpy.withArgs(sinon.match.any, ['test1.io', 'test.io'], sinon.match.any).calledOnce) + assert.deepEqual(onPublisherOptionUpdate.getCall(0).args, ['test1.io', 'verified', false]) + assert.deepEqual(onPublisherOptionUpdate.getCall(2).args, ['test.io', 'verified', false]) }) }) @@ -1705,4 +1764,60 @@ describe('ledger api unit tests', function () { }) }) }) + + describe('onPublisherTimestamp', function () { + let checkVerifiedStatusSpy + + const stateWithData = defaultAppState + .setIn(['ledger', 'synopsis', 'publishers', 'clifton.io'], Immutable.fromJS({ + visits: 1 + })) + + before(function () { + checkVerifiedStatusSpy = sinon.spy(ledgerApi, 'checkVerifiedStatus') + ledgerApi.setClient({ + publisherInfo: function () { + return false + }, + publishersInfo: function () { + return false + } + }) + }) + + afterEach(function () { + checkVerifiedStatusSpy.reset() + }) + + after(function () { + checkVerifiedStatusSpy.restore() + ledgerApi.setClient(undefined) + }) + + it('publisher timestamp is the same', function () { + ledgerApi.onPublisherTimestamp(defaultAppState, 10, 10) + assert(checkVerifiedStatusSpy.notCalled) + }) + + it('publisher list is empty', function () { + ledgerApi.onPublisherTimestamp(defaultAppState, 10, 20) + assert(checkVerifiedStatusSpy.notCalled) + }) + + it('check publishers', function () { + ledgerApi.onPublisherTimestamp(stateWithData, 10, 20) + assert(checkVerifiedStatusSpy.withArgs(sinon.match.any, 'clifton.io', 20).calledOnce) + }) + + it('check multiple publishers', function () { + const multiple = stateWithData + .setIn(['ledger', 'synopsis', 'publishers', 'brave.com'], Immutable.fromJS({ + visits: 1 + })) + ledgerApi.onPublisherTimestamp(multiple, 10, 20) + + assert.equal(checkVerifiedStatusSpy.getCall(0).args[1], 'clifton.io') + assert.equal(checkVerifiedStatusSpy.getCall(1).args[1], 'brave.com') + }) + }) })