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

sviderock · 2024-10-06T19:47:40Z

Description

1st PR for RET-1207. It adds RTK-Query and api slice for the new Transaction Feed V2.

All the reducers up to this point were always manually maintained/changed by us. For this purpose, we have Redux migrations which are covered with tests to ensure compatibility between different versions of Redux state. With introduction of RTK-Query we introduce library-managed api reducers, which must not be manually tweaked as library can change how it stores its structure (api reducer is a cache for API endpoint).

For this purpose, it is mandatory to omit api reducers completely from the flow of typescript-json-schema generation. This implemenation moves existing reducers to a separate file as generating schema with api reducers throws an error of unsupported type from @reduxjs/tookit/query/react.

This new Transaction Feed V2 (that is gonna be implemented over a few PRs) is intended to completely replace existing TransactionFeed, queryHelper and corresponding test files.
It will be closed behind the feature gate from Statsig for the initial release.

Test plan

With this implementation, all the existing tests are working and there is no issues with persistence as the api reducers are completely omitted from the persistence flow. Transactions persistence for Transaction Feed V2 will be implemented in the follow-up PRs.

Related issues

Relates to RET-1207

Backwards compatibility

Yes

Network scalability

If a new NetworkId and/or Network are added in the future, the changes in this PR will:

Continue to work without code changes, OR trigger a compilation error (guaranteeing we find it when a new network is added)

codecov · 2024-10-07T09:30:40Z

Codecov Report

Attention: Patch coverage is 92.38579% with 15 lines in your changes missing coverage. Please review.

Project coverage is 88.75%. Comparing base (1aa94d6) to head (2dad608).
Report is 6 commits behind head on main.

Files with missing lines	Patch %	Lines
src/transactions/feed/TransactionFeedV2.tsx	88.28%	14 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6134      +/-   ##
==========================================
- Coverage   88.75%   88.75%   -0.01%     
==========================================
  Files         727      727              
  Lines       30806    30752      -54     
  Branches     5323     5624     +301     
==========================================
- Hits        27343    27295      -48     
+ Misses       3419     3259     -160     
- Partials       44      198     +154

Files with missing lines	Coverage Δ
src/redux/api.ts	`100.00% <100.00%> (ø)`
src/redux/apiReducersList.ts	`100.00% <100.00%> (ø)`
src/redux/migrations.ts	`97.05% <100.00%> (+<0.01%)`	⬆️
src/redux/reducers.ts	`83.33% <100.00%> (-10.96%)`	⬇️
src/redux/reducersList.ts	`100.00% <100.00%> (ø)`
src/redux/store.ts	`80.00% <100.00%> (+1.31%)`	⬆️
src/transactions/NoActivity.tsx	`100.00% <ø> (ø)`
src/transactions/api.ts	`100.00% <100.00%> (ø)`
src/transactions/apiTestHelpers.ts	`100.00% <100.00%> (ø)`
src/transactions/reducer.ts	`85.21% <100.00%> (ø)`
... and 5 more

... and 88 files with indirect coverage changes

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1aa94d6...2dad608. Read the comment docs.

jeanregisser

Great! 👍

jeanregisser · 2024-10-08T16:11:37Z

src/redux/store.test.ts

@@ -25,6 +27,16 @@ const resetStateOnInvalidStoredAccount = jest.spyOn(

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

+const getNonApiReducers = <R = Omit<RootState, ApiReducersKeys>>(state: RootState): R => {


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.

jeanregisser · 2024-10-08T16:15:05Z

src/redux/store.test.ts

@@ -25,6 +27,16 @@ const resetStateOnInvalidStoredAccount = jest.spyOn(

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

+const getNonApiReducers = <R = Omit<RootState, ApiReducersKeys>>(state: RootState): R => {
+  const apiReducersKeys: string[] = ['transactionFeedV2Api'] satisfies ApiReducersKeys[]


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)

jeanregisser · 2024-10-08T16:16:39Z

src/redux/store.test.ts

+  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)


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

@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!

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.

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

@jeanregisser refactored to for loop

jeanregisser · 2024-10-08T16:35:22Z

src/transactions/apiTestHelpers.ts

+import { RecursivePartial } from 'test/utils'
+
+// https://medium.com/@johnmcdowell0801/testing-rtk-query-with-jest-cdfa5aaf3dc1
+export function setupApiStore<


Could we use/extend our existing mockStore instead?

wallet/test/utils.ts

Lines 76 to 82 in 1aa94d6

const mockStore = configureMockStore<RootState>()

/* Create a mock store with some reasonable default values */

export type RecursivePartial<T> = { [P in keyof T]?: RecursivePartial<T[P]> }

export function createMockStore(overrides: RecursivePartial<RootState> = {}) {

return mockStore(getMockStoreData(overrides))

}

So it's not specific to this reducer.

This way we'll respect one of the Redux testing principle:

Prefer writing integration tests with everything working together.

https://redux.js.org/usage/writing-tests

@jeanregisser Initially, I've tried to use this store and I couldn't get it to work.

To test RTK-Query it is required to have api middlewares in the mocked store. configureMockStore accepts an array of middlewares as an argument and I've tried to pass it was triggering for all the other tests to fail with error saying something similar to: "Actions must not be dispatched but passed as a plain object". Considering, that RTK-Query was released in 2021 and the last update for redux-mock-store was in 2018, it seems like there were no changes made to the library to make is able to work with RTK-Query.

I was looking for the solution and found this article (I've also put it in a comment there so there's some reference) that pointed me in a direction to try to use the helper function Redux team uses themselves to create a testable store. I've slightly modified it but it seemed to do the job.

P.S. Here's the quote from the article explaining the potential reason of why redux-mock-store doesn't work with RTK-Query:
"Typically we might reach for a tool like redux-mock-store but this library doesn’t work when testing RTK Query because dispatching RTK Query actions relies on there being a reducer for the API service and redux-mock-store doesn’t allow the use of reducers."

@jeanregisser I've now added all the data from getMockStoreData to it so it's identical. Sorry, definitely missed this initially!
The two mocked stores should be identical now in terms of preloaded data.

jeanregisser · 2024-10-08T16:42:42Z

src/transactions/api.test.tsx

I wonder if we could/should skip these, given it looks like we're mostly testing the behavior of the library.
Which we can probably trust it's working as expected.

Instead we could focus on the higher level testing with the transaction feed tests which will exercise these anyway and we'll get coverage, right?

@jeanregisser I totally agree. I've only added those to hide the codecov report warnings. Will remove!

jeanregisser · 2024-10-08T16:45:15Z

src/transactions/apiTestHelpers.ts

+        ...extraReducers,
+      }),
+      middleware: (gdm) =>
+        gdm({ serializableCheck: false, immutableCheck: false }).concat(api.middleware),


what's gdm?

@jeanregisser Ah, that's getDefaultMiddleware. I've copied most of this function from this helper function and missed to rename this abbreviation. Will fix!

jeanregisser · 2024-10-08T16:52:17Z

src/transactions/api.ts

+      /**
+       * Even though payload is types as RootState - redux-persist can evaluate it as undefined.
+       */
+      return action.payload?.[reducerPath]


So this means we'll persist and restore the API slice, right?

If that's correct, how will we handle changes to the API response that would require a client side migration?

@jeanregisser Sorry, I will remove the whole extractRehydrationInfo part!

When I initially added RTK-Query, I was trying a lot of variations to try to keep api persisted so I've added this handler but forgot to remove it once it was no longer needed.

jeanregisser

🚀

jeanregisser · 2024-10-09T08:52:16Z

knip.ts

+    'src/utils/inputValidation.ts',
+    'src/utils/country.json',
+    'src/redux/reducersForSchemaGeneration.ts',
+    'src/transactions/apiTestHelpers.ts',


We'll use it in a follow up PR?

@jeanregisser yes, already removed it in the next PR! Only added here to fix the knip error.

jeanregisser · 2024-10-09T08:57:35Z

src/redux/reducersForSchemaGeneration.ts

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

@jeanregisser yes, will add!

### Description 2nd PR for RET-1207. Implements the following list of basic Transactions Feed functionality: - Adds fetching of all the pages for the wallet address up to the point when there are no transaction to fetch anymore (not showing "no transactions" toast yet, will be added in the follow-up PR). - Cursor for the next page is the timestamp of the last transaction from the current page. If next page includes the same transaction - it gets deduplicated. - Re-uses pending transactions from `pendingStandByTransactionsSelector` - Polls first page every 10 seconds - Sorts transactions to show approvals at the top if the corresponding transaction has identical timestamp More comments for different parts of the pagination flow are added as comments in the file in the [next PR](#6136). ### Test plan 10 out of 16 tests from `TransactionFeed.test.ts` were re-used and adjusted to the new data fetching flow. Other tests will be added in the follow-up PRs purely for the sake of trying to keep the PRs smaller. ### Related issues - Relates to RET-1207 ### Backwards compatibility Yes ### Network scalability If a new NetworkId and/or Network are added in the future, the changes in this PR will: - [x] Continue to work without code changes, OR trigger a compilation error (guaranteeing we find it when a new network is added)

sviderock added 5 commits October 6, 2024 22:34

add RTK-query, add api slice for Transaction Feed V2

0e06735

remove unused export

fff449a

Remove api reducers from schema-checking

0168d7f

Move api middlewares to separate variable

22984cb

fix tests

48d5003

sviderock added 2 commits October 7, 2024 15:53

ingnore new file for schema generation

0fa8fc5

Add tests

24b6321

jeanregisser reviewed Oct 8, 2024

View reviewed changes

sviderock added 6 commits October 9, 2024 10:38

fix as per review comments

f976bec

remove test file for api reducer

07c4f48

add missing data from original mocked store to setupApiStore helper

eb72b45

knip fixes

652faf7

remove isRehydrateAcation

28c343c

add apiTestHelpers toknip for now

f2adff1

sviderock changed the title ~~feat(feed): Add RTK-Query, add api slice for Transaction Feed V2~~ feat(feed): Add RTK-Query, add api slice for Transaction Feed Oct 9, 2024

sviderock changed the title ~~feat(feed): Add RTK-Query, add api slice for Transaction Feed~~ feat(feed): Add RTK-Query, add api slice for Transaction Feed V2 Oct 9, 2024

jeanregisser approved these changes Oct 9, 2024

View reviewed changes

sviderock added 2 commits October 9, 2024 12:19

add an explanation comment for reducersForSchemaGeneration.ts

5fd96e0

refactor .reduce usage for readability

fb7f7dd

sviderock added this pull request to the merge queue Oct 10, 2024

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 10, 2024

sviderock added this pull request to the merge queue Oct 11, 2024

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 11, 2024

sviderock added this pull request to the merge queue Oct 11, 2024

Merged via the queue into main with commit 720ae00 Oct 11, 2024
15 checks passed

sviderock deleted the slava/add-rtk-query branch October 11, 2024 12:10

sviderock changed the title ~~feat(feed): Add RTK-Query, add api slice for Transaction Feed V2~~ feat(TransactionFeedV2): Add RTK-Query, add api slice for Transaction Feed V2 Oct 15, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

sviderock commented Oct 6, 2024 •

edited

Loading

codecov bot commented Oct 7, 2024 •

edited

Loading

jeanregisser left a comment

jeanregisser Oct 8, 2024 •

edited

Loading

jeanregisser Oct 8, 2024

jeanregisser Oct 8, 2024

sviderock Oct 9, 2024

jeanregisser Oct 9, 2024

sviderock Oct 9, 2024

sviderock Oct 9, 2024

jeanregisser Oct 8, 2024

sviderock Oct 9, 2024

sviderock Oct 9, 2024

jeanregisser Oct 8, 2024

sviderock Oct 9, 2024

jeanregisser Oct 8, 2024

sviderock Oct 9, 2024

jeanregisser Oct 8, 2024 •

edited

Loading

sviderock Oct 9, 2024

jeanregisser left a comment

jeanregisser Oct 9, 2024

sviderock Oct 9, 2024 •

edited

Loading

jeanregisser Oct 9, 2024

sviderock Oct 9, 2024

		@@ -25,6 +27,16 @@ const resetStateOnInvalidStoredAccount = jest.spyOn(

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

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

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

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

	const mockStore = configureMockStore<RootState>()

	/* Create a mock store with some reasonable default values */
	export type RecursivePartial<T> = { [P in keyof T]?: RecursivePartial<T[P]> }
	export function createMockStore(overrides: RecursivePartial<RootState> = {}) {
	return mockStore(getMockStoreData(overrides))
	}

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

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

Conversation

sviderock commented Oct 6, 2024 • edited Loading

Description

Test plan

Related issues

Backwards compatibility

Network scalability

codecov bot commented Oct 7, 2024 • edited Loading

Codecov Report

jeanregisser left a comment

Choose a reason for hiding this comment

jeanregisser Oct 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeanregisser Oct 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeanregisser left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sviderock Oct 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sviderock commented Oct 6, 2024 •

edited

Loading

codecov bot commented Oct 7, 2024 •

edited

Loading

jeanregisser Oct 8, 2024 •

edited

Loading

jeanregisser Oct 8, 2024 •

edited

Loading

sviderock Oct 9, 2024 •

edited

Loading