feat(jumpstart): Adding jumpstart transaction detection #5120

dievazqu · 2024-03-18T13:22:38Z

Description

This PR handles jumpstart transaction home feed and common details screen.

Test plan

Unit test + emulator

Related issues

Fixes RET-996

Backwards compatibility

Y

Video recording

Simulator.Screen.Recording.-.iPhone.SE.3rd.generation.-.2024-03-18.at.10.03.23.mp4

emerge-tools · 2024-03-18T14:06:07Z

1 build increased size

Name	Version	Download	Change	Install	Change	Approval
⚠️ Celo (test) org.celo.mobile.test	1.81.0 (146)	26.6 MB	⬆️ 2.4 MB (9.8%)	63.1 MB	⬆️ 2.7 MB (4.45%)	N/A

Celo (test) 1.81.0 (146)
org.celo.mobile.test

⚖️ Compare build
📦 Install build
⏱️ Analyze build performance

Total install size change: ⬆️ 2.7 MB (4.45%)
Total download size change: ⬆️ 2.4 MB (9.8%)

Largest size changes

Item	Install Size Change
📝 splashBackground@3x.jpg	⬆️ 600.2 kB
📝 background@3x.jpg	⬆️ 368.6 kB
📝 boost-rewards@3x.png	⬆️ 188.4 kB
📝 background@2x.jpg	⬆️ 176.1 kB
📝 boost-rewards@2x.png	⬆️ 90.1 kB

View Treemap

🛸 Powered by Emerge Tools

codecov · 2024-03-18T17:31:14Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 85.71%. Comparing base (6118c1e) to head (49d943e).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5120      +/-   ##
==========================================
+ Coverage   85.70%   85.71%   +0.01%     
==========================================
  Files         729      729              
  Lines       29834    29854      +20     
  Branches     5155     5160       +5     
==========================================
+ Hits        25568    25589      +21     
+ Misses       4031     4030       -1     
  Partials      235      235

Files	Coverage Δ
src/transactions/feed/TransactionFeed.tsx	`93.10% <100.00%> (ø)`
src/transactions/feed/TransactionFeedItemImage.tsx	`93.10% <100.00%> (+0.79%)`	⬆️
src/transactions/feed/TransferFeedItem.tsx	`94.44% <100.00%> (+0.15%)`	⬆️
src/transactions/transferFeedUtils.ts	`88.34% <100.00%> (+0.60%)`	⬆️
src/transactions/utils.ts	`100.00% <100.00%> (ø)`
test/values.ts	`100.00% <100.00%> (ø)`

... and 1 file 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 6118c1e...49d943e. Read the comment docs.

kathaypacific

looking great!! ✨

kathaypacific · 2024-03-19T08:34:46Z

src/jumpstart/selectors.ts

@@ -11,3 +15,8 @@ export const showJumstartClaimError = (state: RootState) => {
 export const jumpstartSendStatusSelector = (state: RootState) => {
  return state.jumpstart.depositStatus
 }
+
+export const getJumpstartContractAddress = (networkId: NetworkId) => {


this feels a little out of place in a selector file since it doesn't relate to redux. i see it is only used in the utils file, can we remove this function here and use the logic directly in the util?

Sure, I can move this function.
I saw this example:

wallet/src/tokens/selectors.ts

Line 522 in 409ace7

const getJumpstartEnabledNetworkIds = () =>

So I thought it was ok to leave this method here.

kathaypacific · 2024-03-19T08:39:43Z

src/jumpstart/utils.ts

@@ -0,0 +1,7 @@
+import { getJumpstartContractAddress } from 'src/jumpstart/selectors'


i think we are trying to not create more general "utils" files because they become a dumping ground for random functions (sometimes poorly thought out abstractions). perhaps we can rename this file to something like "transactionClassificationHelpers" or something? or perhaps we can add to the mess in the transactions folder, there is already a utils file there that this function might make sense in :)

kathaypacific · 2024-03-19T08:42:04Z

src/transactions/feed/JumpstartFeedItem.tsx

+  const { amount, type } = transfer
+
+  const openTransferDetails = () => {
+    navigate(Screens.TransactionDetailsScreen, { transaction: transfer })


since we already know that this transaction is a jumpstart transaction here, can we add this information as a nav parameter? then the TransactionDetailsScreen would already receive this information as a boolean, and wouldn't need to call isJumpstartTransaction at all. (and then isJumpstartTransaction would only need to be called once in the TransactionFeed, and perhaps can live as an unexported function in that file.

kathaypacific · 2024-03-19T08:47:40Z

src/transactions/feed/JumpstartFeedItem.tsx

+      ? t('feedItemJumpstartReceivedSubtitle')
+      : t('feedItemJumpstartSentSubtitle')
+
+  return (


reading this component, it is so similar to the existing TransferFeedItem...did you consider sharing code between the components? in particular, i think the whole render block could be shared? it seems like the only areas of customisation are the image, title, and subtitle?

Yes! I considered it but the code was a bit ugly because how the TransactionFeedItemImage was used.
And I went with the quick solution, but now I changed TransactionFeedItemImage implementation, so I think I can merge this feed item to the transfer one.
Thanks for the call out!

kathaypacific · 2024-03-19T08:57:40Z

src/transactions/feed/TransactionFeed.test.tsx


+jest.mock('src/jumpstart/utils')


perhaps we can mock the return value of statsig rather than mocking this module? there are no unit tests for this module so kind of nice to not exclude this file intentionally

kathaypacific

LGTM!! i didn't really look at the changes in the TransactionDetailsScreen / TransactionDetailsScreen.tests though since it looks like the changes are reverted in the next PRs. it'd be nice to remove the unnecessary changes from this PR before merging? i think it's okay if the tx details screen for jumpstart just shows as payment sent / recieved

kathaypacific · 2024-03-20T13:32:01Z

src/transactions/transferFeedUtils.ts

@@ -123,6 +123,7 @@ export function useTransferFeedDetails(transfer: FeedTokenTransfer) {
  const tokenInfo = useTokenInfoByAddress(transfer.amount.tokenAddress)
  const coinbasePaySenders = useSelector(coinbasePaySendersSelector)
  const fcTransferDisplayInfo = useFiatConnectTransferDisplayInfo(transfer)
+  const isJumpstart = isJumpstartTransaction(transfer)


Suggested change

const isJumpstart = isJumpstartTransaction(transfer)

const isJumpstartTransaction = isJumpstartTransaction(transfer)

super nit 😅

I can't use that name, because the function is called like that. 😅
I hope leaving isJumpstart is fine.

kathaypacific

🚀

) ### Description This PR handles jumpstart transaction home feed and common details screen. ### Test plan Unit test + emulator ### Related issues - Fixes RET-996 ### Backwards compatibility Y ### Video recording https://github.com/valora-inc/wallet/assets/12504748/4686fa6d-0bca-4268-be92-6d60c9b098e8

dievazqu added 4 commits March 18, 2024 10:00

Cleaning jumpstart feed items

c8bd9b9

Adding tests

e9cc178

Clean code + improve tests

66ce5e2

Merge branch 'main' into diego/jumpstart-feed-items

30be6b8

dievazqu mentioned this pull request Mar 18, 2024

feat(jumpstart): Adding support for jumpstart transactions #5038

Closed

Typo in test name

7edaff4

Fix tests

fac9fec

dievazqu added 3 commits March 18, 2024 14:37

Improve style

eb99862

Improve feed item

19e0afb

Merge branch 'main' into diego/jumpstart-feed-items

0af2960

kathaypacific reviewed Mar 19, 2024

View reviewed changes

dievazqu added 6 commits March 19, 2024 15:07

First pass refactor

27ee575

Fixing tests

6d582f2

Remove unused JumpstarFeedItem + move tests

9264943

Merge branch 'main' into diego/jumpstart-feed-items

9a66c6b

Remove unused type

41713fa

Fixing type unused

0bc87ca

kathaypacific approved these changes Mar 20, 2024

View reviewed changes

dievazqu force-pushed the diego/jumpstart-feed-items branch from cd9d904 to 95e638e Compare March 20, 2024 23:36

Remove unnecessary changes

cf02ff1

dievazqu force-pushed the diego/jumpstart-feed-items branch from 95e638e to cf02ff1 Compare March 20, 2024 23:36

kathaypacific approved these changes Mar 21, 2024

View reviewed changes

Merge branch 'main' into diego/jumpstart-feed-items

49d943e

dievazqu enabled auto-merge March 21, 2024 16:26

dievazqu added this pull request to the merge queue Mar 21, 2024

Merged via the queue into main with commit bf0b169 Mar 21, 2024
16 checks passed

dievazqu deleted the diego/jumpstart-feed-items branch March 21, 2024 17:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(jumpstart): Adding jumpstart transaction detection #5120

feat(jumpstart): Adding jumpstart transaction detection #5120

dievazqu commented Mar 18, 2024 •

edited

emerge-tools bot commented Mar 18, 2024 •

edited

codecov bot commented Mar 18, 2024 •

edited

kathaypacific left a comment

kathaypacific Mar 19, 2024

dievazqu Mar 19, 2024

kathaypacific Mar 19, 2024

kathaypacific Mar 19, 2024

kathaypacific Mar 19, 2024

dievazqu Mar 19, 2024

kathaypacific Mar 19, 2024

kathaypacific left a comment

kathaypacific Mar 20, 2024

dievazqu Mar 20, 2024

kathaypacific left a comment

		@@ -0,0 +1,7 @@
		import { getJumpstartContractAddress } from 'src/jumpstart/selectors'

	const isJumpstart = isJumpstartTransaction(transfer)
	const isJumpstartTransaction = isJumpstartTransaction(transfer)


		jest.mock('src/jumpstart/utils')

feat(jumpstart): Adding jumpstart transaction detection #5120

feat(jumpstart): Adding jumpstart transaction detection #5120

Conversation

dievazqu commented Mar 18, 2024 • edited

Description

Test plan

Related issues

Backwards compatibility

Video recording

emerge-tools bot commented Mar 18, 2024 • edited

1 build increased size

Celo (test) 1.81.0 (146)org.celo.mobile.test

codecov bot commented Mar 18, 2024 • edited

Codecov Report

kathaypacific left a 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

kathaypacific left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kathaypacific left a comment

Choose a reason for hiding this comment

dievazqu commented Mar 18, 2024 •

edited

emerge-tools bot commented Mar 18, 2024 •

edited

Celo (test) 1.81.0 (146)
org.celo.mobile.test

codecov bot commented Mar 18, 2024 •

edited