From 601f744824235db6de105aee1f9fa25ba8e0144f Mon Sep 17 00:00:00 2001 From: Vit Stanislav Date: Tue, 12 Sep 2017 10:33:47 +0200 Subject: [PATCH 01/10] Update transactions reducer to prefer newly fetched txs because they have higher "confirmations" field --- src/store/reducers/transactions.js | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/src/store/reducers/transactions.js b/src/store/reducers/transactions.js index 3d0a4f189..01d3ba7a1 100644 --- a/src/store/reducers/transactions.js +++ b/src/store/reducers/transactions.js @@ -6,8 +6,6 @@ import actionTypes from '../../constants/actions'; * @param {Object} action */ const transactions = (state = { pending: [], confirmed: [], count: 0 }, action) => { - let startTimestamp; - switch (action.type) { case actionTypes.transactionAdded: return Object.assign({}, state, { @@ -22,9 +20,6 @@ const transactions = (state = { pending: [], confirmed: [], count: 0 }, action) count: action.data.count, }); case actionTypes.transactionsUpdated: - startTimestamp = state.confirmed.length ? - state.confirmed[0].timestamp : - 0; return Object.assign({}, state, { // Filter any newly confirmed transaction from pending pending: state.pending.filter( @@ -32,8 +27,10 @@ const transactions = (state = { pending: [], confirmed: [], count: 0 }, action) transaction => transaction.id === pendingTransaction.id).length === 0), // Add any newly confirmed transaction to confirmed confirmed: [ - ...action.data.confirmed.filter(transaction => transaction.timestamp > startTimestamp), - ...state.confirmed, + ...action.data.confirmed, + ...state.confirmed.filter( + confirmedTransaction => action.data.confirmed.filter( + transaction => transaction.id === confirmedTransaction.id).length === 0), ], count: action.data.count, }); From 5013e0742fb6bb0abae50b4f165b4d1df2135aa5 Mon Sep 17 00:00:00 2001 From: Vit Stanislav Date: Tue, 12 Sep 2017 10:36:01 +0200 Subject: [PATCH 02/10] Fetch new transactions on each metronome beat if the app is not in background, because we want to show updated number of "confirmations" --- src/store/middlewares/account.js | 26 ++++++++++++++++++-------- src/utils/metronome.js | 9 +++++---- 2 files changed, 23 insertions(+), 12 deletions(-) diff --git a/src/store/middlewares/account.js b/src/store/middlewares/account.js index 33628e478..d106a2cc7 100644 --- a/src/store/middlewares/account.js +++ b/src/store/middlewares/account.js @@ -6,18 +6,28 @@ import actionTypes from '../../constants/actions'; import { fetchAndUpdateForgedBlocks } from '../../actions/forging'; import { getDelegate } from '../../utils/api/delegate'; import transactionTypes from '../../constants/transactionTypes'; +import { SYNC_ACTIVE_INTERVAL, SYNC_INACTIVE_INTERVAL } from '../../constants/api'; -const updateAccountData = (store) => { // eslint-disable-line +const updateTransactions = (store, peers, account) => { + const maxBlockSize = 25; + transactions(peers.data, account.address, maxBlockSize) + .then(response => store.dispatch(transactionsUpdated({ + confirmed: response.transactions, + count: parseInt(response.count, 10), + }))); +}; + +const updateAccountData = (store, action) => { // eslint-disable-line const { peers, account } = store.getState(); getAccount(peers.data, account.address).then((result) => { + if (action.data.interval === SYNC_ACTIVE_INTERVAL) { + updateTransactions(store, peers, account); + } if (result.balance !== account.balance) { - const maxBlockSize = 25; - transactions(peers.data, account.address, maxBlockSize) - .then(response => store.dispatch(transactionsUpdated({ - confirmed: response.transactions, - count: parseInt(response.count, 10), - }))); + if (action.data.interval === SYNC_INACTIVE_INTERVAL) { + updateTransactions(store, peers, account); + } if (account.isDelegate) { store.dispatch(fetchAndUpdateForgedBlocks({ activePeer: peers.data, @@ -55,7 +65,7 @@ const accountMiddleware = store => next => (action) => { next(action); switch (action.type) { case actionTypes.metronomeBeat: - updateAccountData(store); + updateAccountData(store, action); break; case actionTypes.transactionsUpdated: delegateRegistration(store, action); diff --git a/src/utils/metronome.js b/src/utils/metronome.js index 23e8b5b88..83ff4199b 100644 --- a/src/utils/metronome.js +++ b/src/utils/metronome.js @@ -17,17 +17,18 @@ class Metronome { * @param {Date} lastBeat * @param {Date} now * @param {Number} factor + * @param {Number} interval * @memberOf Metronome * @private */ - _dispatch(lastBeat, now, factor) { + _dispatch(lastBeat, now, factor, interval) { this.dispatchFn({ type: actionsType.metronomeBeat, - data: { lastBeat, now, factor }, + data: { lastBeat, now, factor, interval }, }); } - /** + /** * We're calling this in framerate. * calls broadcast method every SYNC_(IN)ACTIVE_INTERVAL and * sends a numeric factor for ease of use as multiples of updateInterval. @@ -38,7 +39,7 @@ class Metronome { _step() { const now = new Date(); if (!this.lastBeat || (now - this.lastBeat >= this.interval)) { - this._dispatch(this.lastBeat, now, this.factor); + this._dispatch(this.lastBeat, now, this.factor, this.interval); this.lastBeat = now; this.factor += this.factor < 9 ? 1 : -9; } From 4101d5b02e28b3224986e012171437f9a8927f16 Mon Sep 17 00:00:00 2001 From: Vit Stanislav Date: Tue, 12 Sep 2017 10:37:54 +0200 Subject: [PATCH 03/10] Always rerender transactions component because we need to update "confirmations" number --- src/components/transactions/transactions.js | 6 ------ 1 file changed, 6 deletions(-) diff --git a/src/components/transactions/transactions.js b/src/components/transactions/transactions.js index 092fc256b..ece120073 100644 --- a/src/components/transactions/transactions.js +++ b/src/components/transactions/transactions.js @@ -23,12 +23,6 @@ class Transactions extends React.Component { } } - shouldComponentUpdate(nextProps) { - const shouldUpdate = ((nextProps.confirmedCount !== this.props.confirmedCount) || - (nextProps.pendingCount !== this.props.pendingCount)); - return shouldUpdate; - } - componentDidUpdate() { this.canLoadMore = this.props.count > this.props.transactions.length; } From 8f72edf37ba5887e741e66af54d670b153d14cc1 Mon Sep 17 00:00:00 2001 From: Vit Stanislav Date: Tue, 12 Sep 2017 10:43:29 +0200 Subject: [PATCH 04/10] Re-render transactionRow only if confirmations < 1000 ... because transactions table was slow when it was re-rendering all rows. --- src/components/transactions/transactionRow.js | 52 +++++++++++-------- 1 file changed, 30 insertions(+), 22 deletions(-) diff --git a/src/components/transactions/transactionRow.js b/src/components/transactions/transactionRow.js index 874912793..6dabe4ba5 100644 --- a/src/components/transactions/transactionRow.js +++ b/src/components/transactions/transactionRow.js @@ -7,31 +7,39 @@ import Status from './status'; import Amount from './amount'; import Spinner from '../spinner'; -const TransactionRow = props => ( - - +class TransactionRow extends React.Component { + // eslint-disable-next-line class-methods-use-this + shouldComponentUpdate(nextProps) { + return nextProps.value.confirmations <= 1000; + } + + render() { + const props = this.props; + return ( + {props.value.confirmations ? : } - - + + {props.value.id} - - - - - - - - - - - - - - - - -); + + + + + + + + + + + + + + + + ); + } +} export default TransactionRow; From f531dfc18a9158fbed3dc8399c3a05dde7a52bb3 Mon Sep 17 00:00:00 2001 From: Vit Stanislav Date: Tue, 12 Sep 2017 10:44:01 +0200 Subject: [PATCH 05/10] Don't display exact confirmations above 1000 Format it with 'k', e.g. 1k instead of 1324, 23k instead of 23456 so that we don't have to re-render too often. --- package.json | 1 + src/components/transactions/transactionRow.js | 3 ++- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/package.json b/package.json index 55f78b485..9c92a402a 100644 --- a/package.json +++ b/package.json @@ -34,6 +34,7 @@ "flexboxgrid": "=6.3.1", "lisk-js": "=0.4.5", "moment": "=2.15.1", + "numeral": "=2.0.6", "postcss": "=6.0.2", "postcss-cssnext": "=2.11.0", "prop-types": "=15.5.10", diff --git a/src/components/transactions/transactionRow.js b/src/components/transactions/transactionRow.js index 6dabe4ba5..69c7422fb 100644 --- a/src/components/transactions/transactionRow.js +++ b/src/components/transactions/transactionRow.js @@ -1,4 +1,5 @@ import React from 'react'; +import numeral from 'numeral'; import LiskAmount from '../liskAmount'; import { TooltipTime, TooltipWrapper } from '../timestamp'; import TransactionType from './transactionType'; @@ -22,7 +23,7 @@ class TransactionRow extends React.Component { } - {props.value.id} + {props.value.id} From d2a8462a6a91a030e09c0a982a9efe677ee68108 Mon Sep 17 00:00:00 2001 From: Vit Stanislav Date: Tue, 12 Sep 2017 11:31:54 +0200 Subject: [PATCH 06/10] Fix a typo --- src/store/middlewares/account.test.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/store/middlewares/account.test.js b/src/store/middlewares/account.test.js index 894f7a701..fe8d7351f 100644 --- a/src/store/middlewares/account.test.js +++ b/src/store/middlewares/account.test.js @@ -34,7 +34,7 @@ describe('Account middleware', () => { next = spy(); }); - it('should passes the action to next middleware', () => { + it('should pass the action to next middleware', () => { const expectedAction = { type: 'TEST_ACTION', }; From e06150a6865debc775f338fb76a2341dc3814b54 Mon Sep 17 00:00:00 2001 From: Vit Stanislav Date: Tue, 12 Sep 2017 12:31:23 +0200 Subject: [PATCH 07/10] Add accountMiddleware unit tests --- src/store/middlewares/account.test.js | 83 +++++++++++++++++++++------ 1 file changed, 67 insertions(+), 16 deletions(-) diff --git a/src/store/middlewares/account.test.js b/src/store/middlewares/account.test.js index fe8d7351f..703adb4bb 100644 --- a/src/store/middlewares/account.test.js +++ b/src/store/middlewares/account.test.js @@ -5,11 +5,16 @@ import * as accountApi from '../../utils/api/account'; import * as delegateApi from '../../utils/api/delegate'; import actionTypes from '../../constants/actions'; import transactionTypes from '../../constants/transactionTypes'; +import { SYNC_ACTIVE_INTERVAL, SYNC_INACTIVE_INTERVAL } from '../../constants/api'; describe('Account middleware', () => { let store; let next; let state; + let stubGetAccount; + let stubGetAccountStatus; + let stubTransactions; + const transactionsUpdatedAction = { type: actionTypes.transactionsUpdated, data: { @@ -19,6 +24,20 @@ describe('Account middleware', () => { }, }; + const activeBeatAction = { + type: actionTypes.metronomeBeat, + data: { + interval: SYNC_ACTIVE_INTERVAL, + }, + }; + + const inactiveBeatAction = { + type: actionTypes.metronomeBeat, + data: { + interval: SYNC_INACTIVE_INTERVAL, + }, + }; + beforeEach(() => { store = stub(); store.dispatch = spy(); @@ -31,7 +50,17 @@ describe('Account middleware', () => { }, }; store.getState = () => (state); + next = spy(); + stubGetAccount = stub(accountApi, 'getAccount').returnsPromise(); + stubGetAccountStatus = stub(accountApi, 'getAccountStatus').returnsPromise(); + stubTransactions = stub(accountApi, 'transactions').returnsPromise().resolves(true); + }); + + afterEach(() => { + stubGetAccount.restore(); + stubGetAccountStatus.restore(); + stubTransactions.restore(); }); it('should pass the action to next middleware', () => { @@ -44,46 +73,68 @@ describe('Account middleware', () => { }); it(`should call account API methods on ${actionTypes.metronomeBeat} action`, () => { - const stubGetAccount = stub(accountApi, 'getAccount').resolves({ balance: 0 }); - const stubGetAccountStatus = stub(accountApi, 'getAccountStatus').resolves(true); + stubGetAccount.resolves({ balance: 0 }); - middleware(store)(next)({ type: actionTypes.metronomeBeat }); + middleware(store)(next)(activeBeatAction); expect(stubGetAccount).to.have.been.calledWith(); expect(stubGetAccountStatus).to.have.been.calledWith(); - - stubGetAccount.restore(); - stubGetAccountStatus.restore(); }); it(`should call transactions API methods on ${actionTypes.metronomeBeat} action if account.balance changes`, () => { - const stubGetAccount = stub(accountApi, 'getAccount').resolves({ balance: 10e8 }); - const stubTransactions = stub(accountApi, 'transactions').resolves(true); + stubGetAccount.resolves({ balance: 10e8 }); + stubGetAccountStatus.resolves(true); - middleware(store)(next)({ type: actionTypes.metronomeBeat }); + middleware(store)(next)(activeBeatAction); expect(stubGetAccount).to.have.been.calledWith(); // TODO why next expect doesn't work despite it being called according to test coverage? // expect(stubTransactions).to.have.been.calledWith(); + }); - stubGetAccount.restore(); - stubTransactions.restore(); + it(`should call transactions API methods on ${actionTypes.metronomeBeat} action if account.balance changes and action.data.interval is SYNC_INACTIVE_INTERVAL`, () => { + stubGetAccount.resolves({ balance: 10e8 }); + stubGetAccountStatus.rejects(false); + + middleware(store)(next)(inactiveBeatAction); + + expect(stubGetAccount).to.have.been.calledWith(); + // TODO why next expect doesn't work despite it being called according to test coverage? + // expect(stubTransactions).to.have.been.calledWith(); + }); + + it(`should call transactions API methods on ${actionTypes.metronomeBeat} action if action.data.interval is SYNC_ACTIVE_INTERVAL`, () => { + stubGetAccount.resolves({ balance: 0 }); + + middleware(store)(next)(activeBeatAction); + + expect(stubGetAccount).to.have.been.calledWith(); + // TODO why next expect doesn't work despite it being called according to test coverage? + // expect(stubTransactions).to.have.been.calledWith(); + }); + + it(`should fetch delegate info on ${actionTypes.metronomeBeat} action if account.balance changes and account.isDelegate`, () => { + const delegateApiMock = stub(delegateApi, 'getDelegate').returnsPromise().resolves({ success: true, delegate: {} }); + stubGetAccount.resolves({ balance: 10e8 }); + state.account.isDelegate = true; + store.getState = () => (state); + + middleware(store)(next)(activeBeatAction); + expect(store.dispatch).to.have.been.calledWith(); + + delegateApiMock.restore(); }); it(`should call fetchAndUpdateForgedBlocks(...) on ${actionTypes.metronomeBeat} action if account.balance changes and account.isDelegate`, () => { state.account.isDelegate = true; store.getState = () => (state); - const stubGetAccount = stub(accountApi, 'getAccount').resolves({ balance: 10e8 }); - const stubGetAccountStatus = stub(accountApi, 'getAccountStatus').resolves(true); + stubGetAccount.resolves({ balance: 10e8 }); // const fetchAndUpdateForgedBlocksSpy = spy(forgingActions, 'fetchAndUpdateForgedBlocks'); middleware(store)(next)({ type: actionTypes.metronomeBeat }); // TODO why next expect doesn't work despite it being called according to test coverage? // expect(fetchAndUpdateForgedBlocksSpy).to.have.been.calledWith(); - - stubGetAccount.restore(); - stubGetAccountStatus.restore(); }); it(`should fetch delegate info on ${actionTypes.transactionsUpdated} action if action.data.confirmed contains delegateRegistration transactions`, () => { From 2da347ee6f3895648ebbc3cf5fdd14d4b4679312 Mon Sep 17 00:00:00 2001 From: Vit Stanislav Date: Tue, 12 Sep 2017 12:32:17 +0200 Subject: [PATCH 08/10] Update "should show transactions" e2e test --- features/transactions.feature | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/features/transactions.feature b/features/transactions.feature index 65c4a2032..8b52f50a9 100644 --- a/features/transactions.feature +++ b/features/transactions.feature @@ -2,7 +2,7 @@ Feature: Transactions tab Scenario: should show transactions Given I'm logged in as "genesis" When I click tab number 1 - Then I should see table with 40 lines + Then I should see table with 25 lines Scenario: should allow send to address Given I'm logged in as "genesis" From 6d5e7a83a99763b0ea5ab0858b38668fdcc752c5 Mon Sep 17 00:00:00 2001 From: Vit Stanislav Date: Thu, 14 Sep 2017 10:01:38 +0200 Subject: [PATCH 09/10] Refetch transactions only if there are some recent ones --- src/store/middlewares/account.js | 10 ++++++++-- src/store/middlewares/account.test.js | 8 +++++++- 2 files changed, 15 insertions(+), 3 deletions(-) diff --git a/src/store/middlewares/account.js b/src/store/middlewares/account.js index d106a2cc7..0ac142224 100644 --- a/src/store/middlewares/account.js +++ b/src/store/middlewares/account.js @@ -17,11 +17,17 @@ const updateTransactions = (store, peers, account) => { }))); }; +const hasRecentTransactions = state => ( + state.transactions.confirmed.filter(tx => tx.confirmations < 1000).length !== 0 || + state.transactions.pending.length !== 0 +); + const updateAccountData = (store, action) => { // eslint-disable-line - const { peers, account } = store.getState(); + const state = store.getState(); + const { peers, account } = state; getAccount(peers.data, account.address).then((result) => { - if (action.data.interval === SYNC_ACTIVE_INTERVAL) { + if (action.data.interval === SYNC_ACTIVE_INTERVAL && hasRecentTransactions(state)) { updateTransactions(store, peers, account); } if (result.balance !== account.balance) { diff --git a/src/store/middlewares/account.test.js b/src/store/middlewares/account.test.js index 703adb4bb..b0399f2af 100644 --- a/src/store/middlewares/account.test.js +++ b/src/store/middlewares/account.test.js @@ -48,6 +48,12 @@ describe('Account middleware', () => { account: { balance: 0, }, + transactions: { + pending: [{ + id: 12498250891724098, + }], + confirmed: [], + }, }; store.getState = () => (state); @@ -103,7 +109,7 @@ describe('Account middleware', () => { // expect(stubTransactions).to.have.been.calledWith(); }); - it(`should call transactions API methods on ${actionTypes.metronomeBeat} action if action.data.interval is SYNC_ACTIVE_INTERVAL`, () => { + it(`should call transactions API methods on ${actionTypes.metronomeBeat} action if action.data.interval is SYNC_ACTIVE_INTERVAL and there are recent transactions`, () => { stubGetAccount.resolves({ balance: 0 }); middleware(store)(next)(activeBeatAction); From bb76c512df2fe08c0c9e42e3b725ad264a1506f6 Mon Sep 17 00:00:00 2001 From: Vit Stanislav Date: Thu, 14 Sep 2017 11:21:21 +0200 Subject: [PATCH 10/10] Revert "Update "should show transactions" e2e test" This reverts commit 2da347ee6f3895648ebbc3cf5fdd14d4b4679312. --- features/transactions.feature | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/features/transactions.feature b/features/transactions.feature index 8b52f50a9..65c4a2032 100644 --- a/features/transactions.feature +++ b/features/transactions.feature @@ -2,7 +2,7 @@ Feature: Transactions tab Scenario: should show transactions Given I'm logged in as "genesis" When I click tab number 1 - Then I should see table with 25 lines + Then I should see table with 40 lines Scenario: should allow send to address Given I'm logged in as "genesis"