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(earn): Add EarnHome screen #5656

Open
wants to merge 19 commits into
base: main
Choose a base branch
from

Conversation

finnian0826
Copy link
Contributor

@finnian0826 finnian0826 commented Jul 18, 2024

Description

Move over necessary multi-pool earn stuff from earnV2. Two pools shown for testing purposes but the PR only includes the single Aave pool currently supported.

Will add analytics and hook stuff up in follow-up PRs

Test plan

all.mp4
Open pools My pools Bottom sheet
open my bottom-sheet

Related issues

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)

Copy link

codecov bot commented Jul 18, 2024

Codecov Report

Attention: Patch coverage is 82.77512% with 36 lines in your changes missing coverage. Please review.

Project coverage is 86.74%. Comparing base (98966fa) to head (caeba91).

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #5656      +/-   ##
==========================================
- Coverage   86.77%   86.74%   -0.04%     
==========================================
  Files         774      779       +5     
  Lines       31807    32015     +208     
  Branches     5216     5243      +27     
==========================================
+ Hits        27602    27770     +168     
- Misses       4157     4197      +40     
  Partials       48       48              
Files Coverage Δ
src/components/FilterChipsCarousel.tsx 100.00% <100.00%> (ø)
src/earn/EarnTabBar.tsx 100.00% <100.00%> (ø)
src/earn/PoolList.tsx 100.00% <100.00%> (ø)
src/earn/types.ts 100.00% <100.00%> (ø)
src/navigator/Screens.tsx 100.00% <100.00%> (ø)
src/components/TokenBottomSheet.tsx 96.02% <66.66%> (-0.60%) ⬇️
src/earn/PoolCard.tsx 89.74% <89.74%> (ø)
src/earn/EarnHome.tsx 73.50% <73.50%> (ø)

... and 2 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 98966fa...caeba91. Read the comment docs.

@finnian0826 finnian0826 marked this pull request as ready for review July 19, 2024 21:16
@@ -240,8 +240,6 @@ function DappsScreen({ navigation }: Props) {
<FilterChipsCarousel
chips={filterChips}
onSelectChip={handleToggleFilterChip}
primaryColor={Colors.successDark}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This defaults to gray/black buttons, Kayla mentioned wanting this standard across the app here

Comment on lines 44 to 49
primaryColor?: colors
primaryBorderColor?: colors
primaryTextColor?: colors
secondaryColor?: colors
secondaryBorderColor?: colors
secondaryTextColor?: colors
Copy link
Contributor

Choose a reason for hiding this comment

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

can we drop these params if there's no usage of customizing? The less options the better. Also, would be nice if the styling change for this component is in a separate PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

moved to #5672

@@ -33,6 +34,7 @@ export enum TokenPickerOrigin {
CashIn = 'CashIn',
CashOut = 'CashOut',
Spend = 'Spend',
EarnFilter = 'EarnFilter',
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
EarnFilter = 'EarnFilter',
Earn = 'Earn',

?

]

describe('EarnHome', () => {
it('renders correctly', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

needs tests for changing tabs / filtering

@@ -538,6 +539,7 @@ const generalScreens = (Navigator: typeof Stack) => (

const earnScreens = (Navigator: typeof Stack) => (
<>
<Navigator.Screen name={Screens.EarnHome} component={EarnHome} options={headerWithBackButton} />
Copy link
Contributor

Choose a reason for hiding this comment

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

use custom header over nav header to avoid the header not greying out when the bottom sheet is open

Copy link
Contributor Author

@finnian0826 finnian0826 Jul 23, 2024

Choose a reason for hiding this comment

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

I'm having some trouble getting a custom header to work with the useScrollAwareHeader to have scrolling work and the title show up on the top. Is there any example of getting these to work together that we have, I'm having trouble finding one. @satish-ravi @MuckT

Copy link
Contributor

Choose a reason for hiding this comment

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

useScrollAwareHeader is meant to work with nav header and not custom header. We'll likely need a new wrapper for custom headers, which is beyond the scope of this. Let's make an issue

src/earn/PoolCard.tsx Show resolved Hide resolved
src/earn/PoolCard.tsx Show resolved Hide resolved
{
poolId: 'aArbUSDCn',
networkId: NetworkId['arbitrum-one'],
tokens: [`${NetworkId['arbitrum-one']}:0xaf88d065e77c8cc2239327c5edb3a432268e5831`],
Copy link
Contributor

@satish-ravi satish-ravi Jul 20, 2024

Choose a reason for hiding this comment

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

For now I think we are going to support pools where this list is going to be the same as the deposit token id. I guess this is needed for more complex pools, @jeanregisser do you see value in keeping this?

Copy link
Member

Choose a reason for hiding this comment

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

It's fine, it'll be part of the position anyway. I'll update this as I start working on the task to connect it to getEarnPositions.

import { EarnTabType } from 'src/earn/types'
import Colors from 'src/styles/colors'

describe('AssetTabBar', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

EarnTabBar

jest.clearAllMocks()
})

it('renders all items if positions is enabled', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

this test name doesn't seem right

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated, had copied over these tests from AssetTabBar and missed updating title

<View style={styles.buttonContainer}>
<Button
onPress={() => {
navigate(Screens.EarnCollectScreen, { depositTokenId, poolTokenId })
Copy link
Contributor

Choose a reason for hiding this comment

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

needs test

/>
<Button
onPress={() => {
navigate(Screens.EarnEnterAmount, { tokenId: depositTokenId })
Copy link
Contributor

Choose a reason for hiding this comment

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

needs test

expect(getByTestId(`PoolCard/${pools[0].poolId}`)).toBeTruthy()
})

it('correctly shows correct networks, tokens under filters', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

can we also add tests for changing filters?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This might be difficult with the current hardcoded single pool. There isn't any way to specify for the home screen to show more pools, so the pools being shown wouldn't change at all.

@@ -215,6 +217,9 @@ function TokenBottomSheet({
if (isNetworkChip(filter)) {
return filter.filterFn(token, filter.selectedNetworkIds)
}
if (isTokenSelectChip(filter)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

is this just to make TS happy? the token bottom sheet itself isn't going to have a token filter chip right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is just copied over from Jacob's implementation in Troopo, I can remove if not necessary, the token bottom sheet won't have filters I don't think

@@ -538,6 +539,7 @@ const generalScreens = (Navigator: typeof Stack) => (

const earnScreens = (Navigator: typeof Stack) => (
<>
<Navigator.Screen name={Screens.EarnHome} component={EarnHome} options={headerWithBackButton} />
Copy link
Contributor

Choose a reason for hiding this comment

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

useScrollAwareHeader is meant to work with nav header and not custom header. We'll likely need a new wrapper for custom headers, which is beyond the scope of this. Let's make an issue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants