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

Prevent race conditions when loading account #868

Merged
merged 3 commits into from
Jul 1, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions src/app/state/account/saga.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
import { testSaga } from 'redux-saga-test-plan'
import { accountActions } from '.'
import { accountSaga, loadAccount, refreshAccountOnTransaction } from './saga'

describe('Account Sagas', () => {
test('accountSaga', () => {
testSaga(accountSaga)
.next()
.fork(refreshAccountOnTransaction)
.next()
.takeLatest(accountActions.fetchAccount, loadAccount)
.next()
.isDone()
Comment on lines +7 to +13
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this is a useful test.

I'd rather see approximately

expectSaga(stakingSaga)
  .provide(getAccount, () => delay(10*Math.random()))
  .put(fetchAccount('a'))
  .put(fetchAccount('b'))
  .hasFinalState({ address: 'b' })
  .run()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this unit test if valid unless I am missing something, but I am following one of redux saga test plan pattern. I am adding a unit test not integration test. Unit test is testing a watcher saga only and it doesn't care about integration with other parts of code. The bug exists because of using a wrong effect creator. Unit test is validating if a correct effect creator is used so it covers current bug/potential regression bug.

Copy link
Member

Choose a reason for hiding this comment

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

This unit test is essentially "did source code change" and it will break on many non-regressions.

expect(accountSaga.toString()).toBe(`
  export function* accountSaga() {
    yield* fork(refreshAccountOnTransaction)
    yield* takeLatest(actions.fetchAccount, loadAccount)
  }
`)

testSaga becomes interesting when unit testing with different inputs in .next(_), so it tests behavior, instead of implementation.

})
})
8 changes: 4 additions & 4 deletions src/app/state/account/saga.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { PayloadAction } from '@reduxjs/toolkit'
import { addressToPublicKey, parseRpcBalance } from 'app/lib/helpers'
import { all, call, fork, join, put, select, take, takeEvery } from 'typed-redux-saga'
import { all, call, fork, join, put, select, take, takeLatest } from 'typed-redux-saga'

import { accountActions as actions } from '.'
import { getExplorerAPIs, getOasisNic } from '../network/saga'
Expand All @@ -13,7 +13,7 @@ import { selectAccountAddress } from './selectors'
* Waits for a LoadAccount action with a specific address,
* and hydrate the state accordingly
*/
function* loadAccount(action: PayloadAction<string>) {
export function* loadAccount(action: PayloadAction<string>) {
const address = action.payload

yield* put(actions.setLoading(true))
Expand Down Expand Up @@ -65,7 +65,7 @@ function* loadAccount(action: PayloadAction<string>) {
* When a transaction is done, and it is related to the account we currently have in state
* refresh the data.
*/
function* refreshAccountOnTransaction() {
export function* refreshAccountOnTransaction() {
while (true) {
const { payload } = yield* take(transactionActions.transactionSent)
const from = yield* select(selectAddress)
Expand All @@ -88,5 +88,5 @@ function* refreshAccountOnTransaction() {

export function* accountSaga() {
yield* fork(refreshAccountOnTransaction)
yield* takeEvery(actions.fetchAccount, loadAccount)
yield* takeLatest(actions.fetchAccount, loadAccount)
}
23 changes: 23 additions & 0 deletions src/app/state/network/saga.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
import { testSaga } from 'redux-saga-test-plan'
import { networkActions } from '.'
import { networkSaga, selectNetwork } from './saga'

describe('Network Sagas', () => {
const env = process.env

afterEach(() => {
process.env = { ...env }
})

test('networkSaga', () => {
delete process.env.REACT_APP_LOCALNET

testSaga(networkSaga)
.next()
.takeLatest(networkActions.selectNetwork, selectNetwork)
.next()
.put(networkActions.selectNetwork('mainnet'))
.next()
.isDone()
})
})
4 changes: 2 additions & 2 deletions src/app/state/network/saga.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import * as oasis from '@oasisprotocol/client'
import { PayloadAction } from '@reduxjs/toolkit'
import { config } from 'config'
import { call, put, select, takeEvery } from 'typed-redux-saga'
import { call, put, select, takeLatest } from 'typed-redux-saga'
import { backend, backendApi } from 'vendors/backend'

import { networkActions } from '.'
Expand Down Expand Up @@ -50,7 +50,7 @@ export function* selectNetwork({ payload: network }: PayloadAction<NetworkType>)
}

export function* networkSaga() {
yield* takeEvery(networkActions.selectNetwork, selectNetwork)
yield* takeLatest(networkActions.selectNetwork, selectNetwork)

if (process.env.REACT_APP_LOCALNET) {
yield* put(networkActions.selectNetwork('local'))
Expand Down
21 changes: 19 additions & 2 deletions src/app/state/staking/saga.test.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import * as oasis from '@oasisprotocol/client'
import { StakingDebondingDelegationInfo, StakingDelegationInfo } from '@oasisprotocol/client/dist/types'
import { expectSaga } from 'redux-saga-test-plan'
import { expectSaga, testSaga } from 'redux-saga-test-plan'
import * as matchers from 'redux-saga-test-plan/matchers'
import { EffectProviders, StaticProvider } from 'redux-saga-test-plan/providers'
import { select } from 'redux-saga/effects'
Expand All @@ -9,7 +9,14 @@ import { RootState } from 'types'
import { initialState, stakingActions, stakingReducer } from '.'
import { getExplorerAPIs, getOasisNic } from '../network/saga'
import { selectEpoch } from '../network/selectors'
import { fetchAccount, getMainnetDumpValidators, refreshValidators, now, stakingSaga } from './saga'
import {
fetchAccount,
getMainnetDumpValidators,
getValidatorDetails,
refreshValidators,
now,
stakingSaga,
} from './saga'
import { StakingState, Validator } from './types'

const qty = (number: number) => oasis.quantity.fromBigInt(BigInt(number))
Expand Down Expand Up @@ -200,4 +207,14 @@ describe('Staking Sagas', () => {
.run()
})
})

test('stakingSaga', () => {
testSaga(stakingSaga)
.next()
.takeLatest(stakingActions.fetchAccount, fetchAccount)
.next()
.takeLatest(stakingActions.validatorSelected, getValidatorDetails)
.next()
.isDone()
})
})
8 changes: 4 additions & 4 deletions src/app/state/staking/saga.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import { SchedulerValidator } from '@oasisprotocol/client/dist/types'
import { PayloadAction } from '@reduxjs/toolkit'
import { addressToPublicKey, publicKeyToAddress } from 'app/lib/helpers'
import { NetworkType } from 'app/state/network/types'
import { call, put, select, takeEvery } from 'typed-redux-saga'
import { call, put, select, takeLatest } from 'typed-redux-saga'
import { sortByStatus } from 'vendors/helpers'

import { stakingActions } from '.'
Expand Down Expand Up @@ -153,7 +153,7 @@ export async function getMainnetDumpValidators() {
return await import('vendors/oasisscan/dump_validators.json')
}

function* getValidatorDetails({ payload: address }: PayloadAction<string>) {
export function* getValidatorDetails({ payload: address }: PayloadAction<string>) {
const nic = yield* call(getOasisNic)
const publicKey = yield* call(addressToPublicKey, address)
const account = yield* call([nic, nic.stakingAccount], { owner: publicKey, height: 0 })
Expand Down Expand Up @@ -200,6 +200,6 @@ export function* fetchAccount({ payload: address }: PayloadAction<string>) {
}

export function* stakingSaga() {
yield* takeEvery(stakingActions.fetchAccount, fetchAccount)
yield* takeEvery(stakingActions.validatorSelected, getValidatorDetails)
yield* takeLatest(stakingActions.fetchAccount, fetchAccount)
yield* takeLatest(stakingActions.validatorSelected, getValidatorDetails)
}