feat(jumpstart): add reclaim bottom sheet #5125

dievazqu · 2024-03-19T06:15:50Z

Description

This PR adds the reclaim button to the escrow deposits.
It checks if the escrow was claimed and open the bottom sheet, but it doesn't execute the transaction.

Test plan

Emulator

Related issues

Fixes RET-1003

Backwards compatibility

Y

Network scalability

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

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

…ality

kathaypacific

i wasn't sure if this PR is ready for review (it's not in draft) but i left some early comments anyway 🙈 looking good!

kathaypacific · 2024-03-19T09:02:37Z

src/navigator/Navigator.tsx

@@ -685,6 +686,10 @@ function nativeBottomSheets(BottomSheet: typeof RootStack) {
        name={Screens.FiatExchangeCurrencyBottomSheet}
        component={FiatExchangeCurrencyBottomSheet}
      />
+      <BottomSheet.Screen


curious why you chose a screen over a bottom sheet component directly inside the transaction details component? it seems like the only place that needs to launch this bottom sheet is the details screen (the components we have registered as screens so far are ones that can launched from many places...)

kathaypacific · 2024-03-19T09:04:54Z

src/transactions/feed/detailContent/JumpstartReclaimBottomSheet.tsx

+
+  const transactionString = JSON.stringify(reclaimTx)
+
+  async function onConfirm() {


we haven't been terribly consistent about this but i think it is a bad pattern to trigger async functions on button presses because then we don't handle rejected promises well (and don't necessarily await the outcome before moving on). i started using the useAsyncCallback hook to handle this, maybe we can do something similar here? it would also enable the loading state out of the box.

wallet/src/jumpstart/JumpstartEnterAmount.tsx

Line 68 in 0af2960

const handleProceed = useAsyncCallback(

kathaypacific · 2024-03-19T09:05:12Z

src/transactions/feed/detailContent/JumpstartReclaimBottomSheet.tsx

+    Logger.debug(TAG, 'Reclaiming', { reclaimTx, networkId, tokenAmount })
+    // TODO: Send transaction
+    navigateBack()
+    navigateHome()


maybe just navigateHome is okay here?

kathaypacific · 2024-03-19T09:08:51Z

src/transactions/feed/detailContent/JumpstartContent.tsx

+  const walletAddress = useSelector(walletAddressSelector)
+  const jumpstartContractAddress = getJumpstartContractAddress(transfer.networkId)
+
+  const fetchEscrowData = useAsync(


nit: can we be consistent with how we call this feature 😅 jumpstart rather than escrow?

Suggested change

const fetchEscrowData = useAsync(

const fetchJumpstartDepositData = useAsync(

kathaypacific · 2024-03-19T09:19:59Z

src/transactions/feed/detailContent/JumpstartContent.tsx

+    })
+  }
+
+  const ReclaimButton = () => (


i think declaring new components with this syntax is bad for performance because these are recreated on every re-render...

Suggested change

const ReclaimButton = () => (

const ReclaimButton = (

alternatively, why not just use this render block directly inside the render function? i'd argue that it's easier to read the render function this way, because you don't need to jump around to understand the components

kathaypacific · 2024-03-19T09:21:12Z

src/transactions/feed/detailContent/JumpstartContent.tsx

+      ) : (
+        <Button
+          showLoading={fetchEscrowData.loading}
+          disabled={fetchEscrowData.loading}


we need a separate loading button right? otherwise when loading whether a user can reclaim, the button will show "Claim" + loader?

kathaypacific · 2024-03-19T09:23:13Z

src/transactions/feed/detailContent/JumpstartContent.tsx

  if (!token) {
    // should never happen
    Logger.error(TAG, 'Token is undefined')
    return null
  }

-  // TODO: Add reclaim button
+  const ClaimedLabel = () => (


i wonder if you can use the Button component with disabled and touchableStyle set to the green background? maybe you also need to set the color of the font with fontStyle but then it'd ensure that this disabled button won't look different from the enabled / loading buttons?

…ality

codecov · 2024-03-21T05:01:29Z

Codecov Report

Attention: Patch coverage is 86.74699% with 22 lines in your changes are missing coverage. Please review.

Project coverage is 85.78%. Comparing base (8f08231) to head (b7eef06).
Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5125      +/-   ##
==========================================
- Coverage   85.80%   85.78%   -0.02%     
==========================================
  Files         742      745       +3     
  Lines       30104    30217     +113     
  Branches     5189     5210      +21     
==========================================
+ Hits        25831    25922      +91     
- Misses       4037     4058      +21     
- Partials      236      237       +1

Files	Coverage Δ
src/abis/IWalletJumpstart.ts	`100.00% <ø> (ø)`
src/navigator/Screens.tsx	`100.00% <100.00%> (ø)`
src/transactions/feed/TransactionDetails.tsx	`100.00% <100.00%> (ø)`
src/transactions/feed/TransactionDetailsScreen.tsx	`90.80% <100.00%> (-2.12%)`	⬇️
src/transactions/transferFeedUtils.ts	`88.19% <100.00%> (-0.15%)`	⬇️
src/transactions/utils.ts	`100.00% <ø> (ø)`
src/web3/networkConfig.ts	`100.00% <100.00%> (ø)`
src/transactions/feed/TransferFeedItem.tsx	`89.13% <69.23%> (-5.32%)`	⬇️
...actions/feed/JumpstartTransactionDetailsScreen.tsx	`93.67% <93.67%> (ø)`
src/jumpstart/fetchClaimStatus.ts	`35.00% <35.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 8f08231...b7eef06. Read the comment docs.

kathaypacific

i think this is looking great, just small nit comments (but the blocking one is really the test coverage 😅)

kathaypacific · 2024-03-21T08:22:55Z

src/transactions/feed/JumpstartTransactionDetailsScreen.test.tsx

+    storeOverrides?: RecursivePartial<RootState>
+    transaction: TokenTransfer
+  }) {
+    const store = createMockStore({


nit: we don't really need to create a fancy mock store right? the identity and recipients stores are not relevant for this test?

kathaypacific · 2024-03-21T08:23:31Z

src/transactions/feed/JumpstartTransactionDetailsScreen.test.tsx

+    expect(getElementText(amountValue)).toBe('10.00 cUSD')
+  })
+
+  it('handles jumpstart received transactions correctly', async () => {


can we add some tests for the claim button states?

kathaypacific · 2024-03-21T08:25:22Z

src/transactions/feed/JumpstartTransactionDetailsScreen.tsx

+
+  const claimed = erc20Claim[3]
+
+  Logger.debug(`Reward deposited in ${transactionHash} ${claimed ? 'was' : 'was NOT'} claimed`)


nit: to make the logs easier to read, can we add a TAG in all the log statements?

kathaypacific · 2024-03-21T08:33:09Z

locales/base/translation.json

@@ -2219,5 +2221,6 @@
      "contactSupport": "Contact Support",
      "dismiss": "Dismiss"
    }
-  }
+  },
+  "jumpstartReclaimDescription": "Reclaiming your link incurs a network gas fee, would you like to proceed?"


nit: could we create an object to group the translations under? i think you'll need to add error messages (the generic fetch + the failed to reclaim ones) and it'd be easier to understand where those errors belong to if they're grouped within a jumpstartReclaim key?

… the next PR

### Description This PR adds the reclaim button to the escrow deposits. It checks if the escrow was claimed and open the bottom sheet, but it doesn't execute the transaction. ### Test plan Emulator ### Related issues - Fixes RET-1003 ### Backwards compatibility Y ### Network scalability If a new NetworkId and/or Network are added in the future, the changes in this PR will: - [y] Continue to work without code changes, OR trigger a compilation error (guaranteeing we find it when a new network is added) --------- Co-authored-by: Kathy Luo <kathyluo18@gmail.com>

dievazqu added 18 commits March 18, 2024 09:43

Cleaning jumpstart feed items

0a27654

Cleaning jumpstart feed items

c8bd9b9

Merge branch 'diego/jumpstart-feed-items' into diego/reclaim-function…

5e5dc5d

…ality

Adding tests

e9cc178

Clean code + improve tests

66ce5e2

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

30be6b8

Typo in test name

7edaff4

Fix tests

fac9fec

Merge branch 'diego/jumpstart-feed-items' into diego/reclaim-function…

a8e540b

…ality

Fix style

195f2d1

Improve style

eb99862

Improve style

589c92d

Merge branch 'diego/jumpstart-feed-items' into diego/reclaim-function…

a703323

…ality

reclaim botton sheet

3ed19f7

Fix button and interaction

b451563

Improve feed item

19e0afb

Merge branch 'diego/jumpstart-feed-items' into diego/reclaim-function…

fb0f159

…ality

Clean code

c43d7cd

dievazqu changed the title ~~Diego/reclaim functionality~~ feat(escrow): Reclaim button sheet Mar 19, 2024

dievazqu changed the title ~~feat(escrow): Reclaim button sheet~~ feat(escrow): Reclaim botton sheet Mar 19, 2024

dievazqu changed the title ~~feat(escrow): Reclaim botton sheet~~ feat(escrow): Reclaim bottom sheet Mar 19, 2024

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

0af2960

kathaypacific reviewed Mar 19, 2024

View reviewed changes

dievazqu added 7 commits March 19, 2024 15:07

First pass refactor

27ee575

Merge branch 'diego/jumpstart-feed-items' into diego/reclaim-function…

df1ff64

…ality

Fixing tests

6d582f2

Merge branch 'diego/jumpstart-feed-items' into diego/reclaim-function…

74e1b0d

…ality

Remove unused JumpstarFeedItem + move tests

9264943

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

9a66c6b

Merge branch 'diego/jumpstart-feed-items' into diego/reclaim-function…

e93b54c

…ality

dievazqu added 3 commits March 20, 2024 20:50

fix minor detail

242b5e3

Adding missing translation

05f6cb7

update RootStateSchema

fa872f4

kathaypacific reviewed Mar 21, 2024

View reviewed changes

Base automatically changed from diego/jumpstart-feed-items to main March 21, 2024 17:00

dievazqu and others added 11 commits March 26, 2024 02:35

Merge branch 'main' into diego/reclaim-functionality

57b6b14

CR changes + tests

9d9478c

Remove unused function

033853e

Merge branch 'main' into diego/reclaim-functionality

04f664d

Fixing button type name

96f789f

Merge branch 'main' into diego/reclaim-functionality

6b67f59

Merge branch 'main' into diego/reclaim-functionality

572e6b7

chore: remove button variation

d69cce0

chore: add logo shadow

d96b71c

chore: prevent introducing new state variable that will be removed in…

b7ce766

… the next PR

fix: act warnings in tests

b7eef06

kathaypacific approved these changes Mar 28, 2024

View reviewed changes

kathaypacific enabled auto-merge March 28, 2024 10:22

kathaypacific added this pull request to the merge queue Mar 28, 2024

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

kathaypacific added this pull request to the merge queue Mar 28, 2024

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

kathaypacific changed the title ~~feat(escrow): Reclaim bottom sheet~~ feat(jumpstart): Reclaim bottom sheet Mar 28, 2024

kathaypacific added this pull request to the merge queue Mar 28, 2024

kathaypacific changed the title ~~feat(jumpstart): Reclaim bottom sheet~~ feat(jumpstart): add reclaim bottom sheet Mar 28, 2024

Merged via the queue into main with commit 94ea0c4 Mar 28, 2024
18 checks passed

kathaypacific deleted the diego/reclaim-functionality branch March 28, 2024 13:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(jumpstart): add reclaim bottom sheet #5125

feat(jumpstart): add reclaim bottom sheet #5125

dievazqu commented Mar 19, 2024 •

edited

Loading

kathaypacific left a comment

kathaypacific Mar 19, 2024

kathaypacific Mar 19, 2024

kathaypacific Mar 19, 2024

kathaypacific Mar 19, 2024

kathaypacific Mar 19, 2024

kathaypacific Mar 19, 2024

kathaypacific Mar 19, 2024

codecov bot commented Mar 21, 2024 •

edited

Loading

kathaypacific left a comment

kathaypacific Mar 21, 2024

kathaypacific Mar 21, 2024

kathaypacific Mar 21, 2024

kathaypacific Mar 21, 2024


		const transactionString = JSON.stringify(reclaimTx)

		async function onConfirm() {

	const fetchEscrowData = useAsync(
	const fetchJumpstartDepositData = useAsync(


		const claimed = erc20Claim[3]

		Logger.debug(`Reward deposited in ${transactionHash} ${claimed ? 'was' : 'was NOT'} claimed`)

feat(jumpstart): add reclaim bottom sheet #5125

feat(jumpstart): add reclaim bottom sheet #5125

Conversation

dievazqu commented Mar 19, 2024 • edited Loading

Description

Test plan

Related issues

Backwards compatibility

Network scalability

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

codecov bot commented Mar 21, 2024 • edited Loading

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

dievazqu commented Mar 19, 2024 •

edited

Loading

codecov bot commented Mar 21, 2024 •

edited

Loading