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

feat(TransactionFeedV2): Add RTK-Query, add api slice for Transaction Feed V2 #6134

Merged
merged 16 commits into from
Oct 11, 2024
Merged
Show file tree
Hide file tree
Changes from 7 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
6 changes: 5 additions & 1 deletion knip.ts
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,11 @@ const config: KnipConfig = {
'@types/jest',
'husky',
],
ignore: ['src/utils/inputValidation.ts', 'src/utils/country.json'],
ignore: [
'src/utils/inputValidation.ts',
'src/utils/country.json',
'src/redux/reducersForSchemaGeneration.ts',
],
}

export default config
2 changes: 1 addition & 1 deletion scripts/update_root_state_schema.sh
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ set -euo pipefail

root_state_schema="test/RootStateSchema.json"

typescript-json-schema ./tsconfig.json RootState --include src/redux/reducers.ts --ignoreErrors --required --noExtraProps > "$root_state_schema"
typescript-json-schema ./tsconfig.json RootStateForSchemaGeneration --include src/redux/reducersForSchemaGeneration.ts --ignoreErrors --required --noExtraProps > "$root_state_schema"

if git diff --exit-code "$root_state_schema"; then
echo "$root_state_schema is up to date"
Expand Down
20 changes: 20 additions & 0 deletions src/redux/api.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
import { fetchBaseQuery } from '@reduxjs/toolkit/query/react'
import { REHYDRATE } from 'redux-persist'
import type { Action } from 'redux-saga'
import type { RootState } from 'src/redux/reducers'
import networkConfig from 'src/web3/networkConfig'

export const baseQuery = fetchBaseQuery({
baseUrl: networkConfig.blockchainApiUrl,
headers: {
Accept: 'application/json',
},
})

export function isRehydrateAction(action: Action): action is Action<typeof REHYDRATE> & {
key: string
payload: RootState
err: unknown
} {
return action.type === REHYDRATE
}
9 changes: 9 additions & 0 deletions src/redux/apiReducersList.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
import { transactionFeedV2Api } from 'src/transactions/api'

export type ApiReducersKeys = keyof typeof apiReducersList

export const apiReducersList = {
[transactionFeedV2Api.reducerPath]: transactionFeedV2Api.reducer,
} as const

export const apiMiddlewares = [transactionFeedV2Api.middleware]
1 change: 1 addition & 0 deletions src/redux/migrations.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1916,4 +1916,5 @@ export const migrations = {
...state,
app: _.omit(state.app, 'numberVerified'),
}),
233: (state: any) => state,
}
62 changes: 6 additions & 56 deletions src/redux/reducers.ts
Original file line number Diff line number Diff line change
@@ -1,61 +1,11 @@
import { Action, combineReducers } from '@reduxjs/toolkit'
import { PersistState } from 'redux-persist'
import { Actions, ClearStoredAccountAction } from 'src/account/actions'
import { reducer as account } from 'src/account/reducer'
import { reducer as alert } from 'src/alert/reducer'
import { appReducer as app } from 'src/app/reducers'
import dappsReducer from 'src/dapps/slice'
import earnReducer from 'src/earn/slice'
import { reducer as fiatExchanges } from 'src/fiatExchanges/reducer'
import fiatConnectReducer from 'src/fiatconnect/slice'
import { homeReducer as home } from 'src/home/reducers'
import i18nReducer from 'src/i18n/slice'
import { type Action, combineReducers } from '@reduxjs/toolkit'
import { type PersistState } from 'redux-persist'
import { Actions, type ClearStoredAccountAction } from 'src/account/actions'
import { reducer as identity } from 'src/identity/reducer'
import { reducer as imports } from 'src/import/reducer'
import jumpstartReducer from 'src/jumpstart/slice'
import keylessBackupReducer from 'src/keylessBackup/slice'
import { reducer as localCurrency } from 'src/localCurrency/reducer'
import { reducer as networkInfo } from 'src/networkInfo/reducer'
import nftsReducer from 'src/nfts/slice'
import pointsReducer from 'src/points/slice'
import positionsReducer from 'src/positions/slice'
import priceHistoryReducer from 'src/priceHistory/slice'
import { recipientsReducer as recipients } from 'src/recipients/reducer'
import { sendReducer as send } from 'src/send/reducers'
import swapReducer from 'src/swap/slice'
import tokenReducer from 'src/tokens/slice'
import { reducer as transactions } from 'src/transactions/reducer'
import { reducer as walletConnect } from 'src/walletConnect/reducer'
import { reducer as web3 } from 'src/web3/reducer'
import { apiReducersList } from 'src/redux/apiReducersList'
import { reducersList } from 'src/redux/reducersList'

const appReducer = combineReducers({
app,
i18n: i18nReducer,
networkInfo,
alert,
send,
home,
transactions,
web3,
identity,
account,
recipients,
localCurrency,
imports,
fiatExchanges,
walletConnect,
tokens: tokenReducer,
dapps: dappsReducer,
fiatConnect: fiatConnectReducer,
swap: swapReducer,
positions: positionsReducer,
keylessBackup: keylessBackupReducer,
nfts: nftsReducer,
priceHistory: priceHistoryReducer,
jumpstart: jumpstartReducer,
points: pointsReducer,
earn: earnReducer,
})
const appReducer = combineReducers({ ...reducersList, ...apiReducersList })

const rootReducer = (state: RootState | undefined, action: Action): RootState => {
if (action.type === Actions.CLEAR_STORED_ACCOUNT && state) {
Expand Down
8 changes: 8 additions & 0 deletions src/redux/reducersForSchemaGeneration.ts
Copy link
Member

Choose a reason for hiding this comment

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

nit: Maybe we should add a little comment about why we're doing this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jeanregisser yes, will add!

Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
import { combineReducers } from 'redux'
import { PersistState } from 'redux-persist'
import { reducersList } from 'src/redux/reducersList'

const reducerForSchemaGeneration = combineReducers(reducersList)
export type RootStateForSchemaGeneration = ReturnType<typeof reducerForSchemaGeneration> & {
_persist: PersistState
}
55 changes: 55 additions & 0 deletions src/redux/reducersList.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
import { reducer as account } from 'src/account/reducer'
import { reducer as alert } from 'src/alert/reducer'
import { appReducer as app } from 'src/app/reducers'
import dappsReducer from 'src/dapps/slice'
import earnReducer from 'src/earn/slice'
import { reducer as fiatExchanges } from 'src/fiatExchanges/reducer'
import fiatConnectReducer from 'src/fiatconnect/slice'
import { homeReducer as home } from 'src/home/reducers'
import i18nReducer from 'src/i18n/slice'
import { reducer as identity } from 'src/identity/reducer'
import { reducer as imports } from 'src/import/reducer'
import jumpstartReducer from 'src/jumpstart/slice'
import keylessBackupReducer from 'src/keylessBackup/slice'
import { reducer as localCurrency } from 'src/localCurrency/reducer'
import { reducer as networkInfo } from 'src/networkInfo/reducer'
import nftsReducer from 'src/nfts/slice'
import pointsReducer from 'src/points/slice'
import positionsReducer from 'src/positions/slice'
import priceHistoryReducer from 'src/priceHistory/slice'
import { recipientsReducer as recipients } from 'src/recipients/reducer'
import { sendReducer as send } from 'src/send/reducers'
import swapReducer from 'src/swap/slice'
import tokenReducer from 'src/tokens/slice'
import { reducer as transactions } from 'src/transactions/reducer'
import { reducer as walletConnect } from 'src/walletConnect/reducer'
import { reducer as web3 } from 'src/web3/reducer'

export const reducersList = {
app,
i18n: i18nReducer,
networkInfo,
alert,
send,
home,
transactions,
web3,
identity,
account,
recipients,
localCurrency,
imports,
fiatExchanges,
walletConnect,
tokens: tokenReducer,
dapps: dappsReducer,
fiatConnect: fiatConnectReducer,
swap: swapReducer,
positions: positionsReducer,
keylessBackup: keylessBackupReducer,
nfts: nftsReducer,
priceHistory: priceHistoryReducer,
jumpstart: jumpstartReducer,
points: pointsReducer,
earn: earnReducer,
} as const
18 changes: 15 additions & 3 deletions src/redux/store.test.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
import Ajv from 'ajv'
import { spawn, takeEvery } from 'redux-saga/effects'
import { ApiReducersKeys } from 'src/redux/apiReducersList'
import * as createMigrateModule from 'src/redux/createMigrate'
import { migrations } from 'src/redux/migrations'
import { RootState } from 'src/redux/reducers'
import { rootSaga } from 'src/redux/sagas'
import { _persistConfig, setupStore } from 'src/redux/store'
import * as accountCheckerModule from 'src/utils/accountChecker'
Expand All @@ -25,6 +27,16 @@ const resetStateOnInvalidStoredAccount = jest.spyOn(

const loggerErrorSpy = jest.spyOn(Logger, 'error')

const getNonApiReducers = <R = Omit<RootState, ApiReducersKeys>>(state: RootState): R => {
Copy link
Member

@jeanregisser jeanregisser Oct 8, 2024

Choose a reason for hiding this comment

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

nit: prefer function instead of const arrow functions, as it's easier to spot it's a function when parsing the file visually. Plus it's a few less chars :D

Suggested change
const getNonApiReducers = <R = Omit<RootState, ApiReducersKeys>>(state: RootState): R => {
function getNonApiReducers<R = Omit<RootState, ApiReducersKeys>>(state: RootState): R {

I know we haven't been super consistent with this in this codebase though.

const apiReducersKeys: string[] = ['transactionFeedV2Api'] satisfies ApiReducersKeys[]
Copy link
Member

Choose a reason for hiding this comment

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

Could we do this instead? to avoid having to update multiple files when we update them.

Suggested change
const apiReducersKeys: string[] = ['transactionFeedV2Api'] satisfies ApiReducersKeys[]
const apiReducersKeys: string[] = Object.keys(apiReducersList)

return Object.entries(state).reduce((acc, [reducerKey, value]) => {
const key = reducerKey as keyof R
if (apiReducersKeys.includes(reducerKey)) return acc
acc[key] = value as unknown as any
return acc
}, {} as R)
Copy link
Member

Choose a reason for hiding this comment

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

nit: probably ok here because it's short, but I try to avoid reduce as it can often lead to hard to read code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jeanregisser there are also a few .reduce usages in the follow-up PRs as well. Though the reason is not as applicable in this particular case, but I'm trying to reduce the amount of iterations the paginated data gets processed over. Considering, the Transaction Feed screen is one of the main screens with potentially lots of data I thought it made sense to try to minimize iterations so it is as optimized as possible.

I might be overthinking this so please let me know if that's the case. I've tried to make all the .reduce usages (the ones you'll see later in other PRs) fully descriptive but please let me know if there's anything that can be improved in that regard!

Copy link
Member

Choose a reason for hiding this comment

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

Ok, I think we should optimize for readability first, and avoid reduce in general. Given the number of elements we're dealing with, iterating once, twice or more times usually won't have much real world impact.
And we can usually refactor to iterate once with a for loop, if performance is measured to be a problem.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jeanregisser Got it, I'll tweak code in the other PRs to follow this approach!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jeanregisser refactored to for loop

}

beforeEach(() => {
jest.clearAllMocks()
// For some reason createMigrate.mockRestore doesn't work, so instead we manually reset it to the original implementation
Expand Down Expand Up @@ -81,9 +93,9 @@ describe('store state', () => {
})
})

const data = store.getState()
const data = getNonApiReducers(store.getState())

const ajv = new Ajv({ allErrors: true })
const ajv = new Ajv({ allErrors: true, allowUnionTypes: true })
const schema = require('test/RootStateSchema.json')
const validate = ajv.compile(schema)
const isValid = validate(data)
Expand All @@ -98,7 +110,7 @@ describe('store state', () => {
{
"_persist": {
"rehydrated": true,
"version": 232,
"version": 233,
},
"account": {
"acceptedTerms": false,
Expand Down
19 changes: 16 additions & 3 deletions src/redux/store.ts
Original file line number Diff line number Diff line change
@@ -1,15 +1,18 @@
import AsyncStorage from '@react-native-async-storage/async-storage'
import { configureStore, Middleware } from '@reduxjs/toolkit'
import { setupListeners } from '@reduxjs/toolkit/query'
import { getStoredState, PersistConfig, persistReducer, persistStore } from 'redux-persist'
import FSStorage from 'redux-persist-fs-storage'
import autoMergeLevel2 from 'redux-persist/lib/stateReconciler/autoMergeLevel2'
import createSagaMiddleware from 'redux-saga'
import AppAnalytics from 'src/analytics/AppAnalytics'
import { PerformanceEvents } from 'src/analytics/Events'
import { apiMiddlewares } from 'src/redux/apiReducersList'
import { createMigrate } from 'src/redux/createMigrate'
import { migrations } from 'src/redux/migrations'
import rootReducer, { RootState as ReducersRootState } from 'src/redux/reducers'
import { rootSaga } from 'src/redux/sagas'
import { transactionFeedV2Api } from 'src/transactions/api'
import { resetStateOnInvalidStoredAccount } from 'src/utils/accountChecker'
import Logger from 'src/utils/Logger'
import { ONE_DAY_IN_MILLIS } from 'src/utils/time'
Expand All @@ -21,10 +24,17 @@ const persistConfig: PersistConfig<ReducersRootState> = {
key: 'root',
// default is -1, increment as we make migrations
// See https://github.com/valora-inc/wallet/tree/main/WALLET.md#redux-state-migration
version: 232,
version: 233,
keyPrefix: `reduxStore-`, // the redux-persist default is `persist:` which doesn't work with some file systems.
storage: FSStorage(),
blacklist: ['networkInfo', 'alert', 'imports', 'keylessBackup', 'jumpstart'],
blacklist: [
'networkInfo',
'alert',
'imports',
'keylessBackup',
'jumpstart',
transactionFeedV2Api.reducerPath,
],
stateReconciler: autoMergeLevel2,
migrate: async (...args) => {
const migrate = createMigrate(migrations)
Expand Down Expand Up @@ -103,7 +113,8 @@ export const setupStore = (initialState?: ReducersRootState, config = persistCon
)
},
})
const middlewares: Middleware[] = [sagaMiddleware]

const middlewares: Middleware[] = [sagaMiddleware, ...apiMiddlewares]

if (__DEV__ && !process.env.JEST_WORKER_ID) {
const createDebugger = require('redux-flipper').default
Expand Down Expand Up @@ -172,3 +183,5 @@ export { persistor, store }

export type RootState = ReturnType<typeof store.getState>
export type AppDispatch = typeof store.dispatch

setupListeners(store.dispatch)
6 changes: 6 additions & 0 deletions src/swap/SwapScreen.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -1328,6 +1328,9 @@ describe('SwapScreen', () => {
status: 'started',
},
},

// as per test/utils.ts, line 105
transactionFeedV2Api: undefined,
})

update(
Expand Down Expand Up @@ -1373,6 +1376,9 @@ describe('SwapScreen', () => {
status: 'error',
},
},

// as per test/utils.ts, line 105
transactionFeedV2Api: undefined,
})

update(
Expand Down
Loading
Loading