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 asset details for aaveUSDC #5403

Merged
merged 51 commits into from May 13, 2024
Merged

feat: earn asset details for aaveUSDC #5403

merged 51 commits into from May 13, 2024

Conversation

MuckT
Copy link
Collaborator

@MuckT MuckT commented May 9, 2024

Description

Expands on the work in #5400 to allow the multiple button types and pools in EarnCard.tsx. This abstraction should now work for any Aave v3 pools on any network and sets up the ability to handle multiple pools with only layout changes needed when designs are ready.

Test plan

  • Tested locally on iOS
  • Tested locally on Android
  • Unit tests added and updated

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)

MuckT added 30 commits May 7, 2024 12:56
Copy link

codecov bot commented May 9, 2024

Codecov Report

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

Project coverage is 86.14%. Comparing base (93f2ab1) to head (1e73a5d).
Report is 2 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #5403      +/-   ##
==========================================
+ Coverage   86.12%   86.14%   +0.01%     
==========================================
  Files         744      744              
  Lines       30267    30291      +24     
  Branches     5171     5181      +10     
==========================================
+ Hits        26068    26093      +25     
+ Misses       3971     3970       -1     
  Partials      228      228              
Files Coverage Δ
src/analytics/Events.tsx 100.00% <100.00%> (ø)
src/analytics/Properties.tsx 100.00% <ø> (ø)
src/dappsExplorer/TabDiscover.tsx 95.83% <100.00%> (ø)
src/earn/poolInfo.ts 86.36% <100.00%> (+1.36%) ⬆️
src/tokens/TokenDetails.tsx 100.00% <100.00%> (ø)
src/earn/EarnCard.tsx 95.65% <95.65%> (ø)
src/earn/EarnActivePool.tsx 89.09% <81.48%> (+7.27%) ⬆️

... 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 93f2ab1...1e73a5d. Read the comment docs.

Base automatically changed from tomm/act-1179-2 to main May 9, 2024 21:06
src/earn/EarnActivePool.tsx Outdated Show resolved Hide resolved
src/tokens/TokenDetails.test.tsx Outdated Show resolved Hide resolved
src/earn/EarnCard.tsx Outdated Show resolved Hide resolved
src/earn/EarnActivePool.tsx Outdated Show resolved Hide resolved
return fetchAavePoolInfo({
assetAddress: depositToken.address,
contractAddress: networkConfig.arbAavePoolV3ContractAddress,
network: Network.Arbitrum,
Copy link
Contributor

Choose a reason for hiding this comment

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

this can be inferred from the depositToken

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added in f9cc578. I wonder if all the type guards for fetchAavePoolInfo are a bit overkill instead of typecasting and using optional chaining having fetchAavePoolInfo throw instead. Now whenever someone wants to use fetchAavePoolInfo they'll need to use about ~25 lines of type guards to use this function. It might make sense to make these guards reusable the next time we use fetchAavePoolInfo somewhere.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Made these top checks a bit shorter in 27c08ed. It might be worthwhile to create a isValidToken util function to cover the remaining checks if we are finding we are reusing them frequently.

<View style={styles.buttonContainer}>
<Button
onPress={() => {
navigate(Screens.TabDiscover)
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 add an analytics event here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added in 03b7636 and tested in 1e73a5d!

src/earn/EarnActivePool.tsx Outdated Show resolved Hide resolved
src/earn/EarnActivePool.tsx Show resolved Hide resolved
src/earn/EarnActivePool.tsx Outdated Show resolved Hide resolved
src/earn/EarnActivePool.tsx Outdated Show resolved Hide resolved
@MuckT MuckT added this pull request to the merge queue May 13, 2024
Merged via the queue into main with commit d6f9a38 May 13, 2024
16 checks passed
@MuckT MuckT deleted the tomm/act-1185 branch May 13, 2024 22:09
shottah pushed a commit to zed-io/kolektivo that referenced this pull request May 15, 2024
### Description

Expands on the work in valora-inc#5400 to allow the multiple button types and
pools in `EarnCard.tsx`. This abstraction should now work for any Aave
v3 pools on any network and sets up the ability to handle multiple pools
with only layout changes needed when designs are ready.

### Test plan

- [x] Tested locally on iOS
- [x] Tested locally on Android
- [x] Unit tests added and updated

### Related issues

- Fixes ACT-1185

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