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: make lp opportunities great #8888

Merged
merged 15 commits into from
Feb 20, 2025
Merged

feat: make lp opportunities great #8888

merged 15 commits into from
Feb 20, 2025

Conversation

gomesalexandre
Copy link
Contributor

@gomesalexandre gomesalexandre commented Feb 19, 2025

Description

Many birds, one stone:

  • Fixes Portals parsing so that LP read-only opportunities have their LP meta actually upserted in opportunities.lp.byId (they weren't before, and all were upserted as staking)
  • Fixes LP opportunities selectors so that the correct ID is used for introspection, i.e underlyingAssetId (the top-level asset, basically an LP token) vs. the opportunityId (a composite AssetId of sorts).
    Using said composite ID means we were trying to introspect assets[compositeId], failing, early returning, and effectively not having the opportunity as part of LP ones. The reason it "worked" previously was accidental, as a direct consequence of the above.
    First of all, it did not work before. It only worked in "By Provider", not in "By Asset" but we'd never noticed. The reason it worked there is because "By Provider" is more dumb and doesn't use staking/id indexes for rendering but simply rendered all opportunitie indexed by provider, with no regard for their DefiType.
    In comparison, the 'By Asset' view has two "Staking" and "LP" tables, which uses each index and then introspects things, which is where this fix comes in
  • Adds the same "View"/"Manage" logic as below in feat: savers read-only opportunities #8878, this time for LP, not staking
  • Ensures that both LP and staking tables do not render any button (View/Manage) if there's no link, which read-only opportunies do not have following the switch to Portals

Issue (if applicable)

Risk

High Risk PRs Require 2 approvals

Low

What protocols, transaction types, wallets or contract interactions might be affected by this PR?

Testing

  • Test with a wallet that has read-only opportunities (any degen wallet will work) or your own if you have LP tokens (can swap to some using TraderJoe if you don't)
  • Ensure that you can see your LP read-only opportunities in the DeFi section, vs. empty when expanding previously
  • No LP opportunities are displayed as Eligible in the dashboard
  • There is no dead link button in opportunities (View/Manage)

Engineering

  • ^

Operations

  • 🏁 My feature is behind a flag and doesn't require operations testing (yet)
  • ^

Screenshots (if applicable)

develop (right) vs. this diff (left)

Screenshot 2025-02-19 at 15 02 27 Screenshot 2025-02-19 at 15 02 39 Screenshot 2025-02-19 at 15 03 06

@gomesalexandre gomesalexandre requested a review from a team as a code owner February 19, 2025 14:18
@gomesalexandre gomesalexandre changed the title feat: make lp opportunities great again feat: make lp opportunities great ~~again~~ Feb 19, 2025
@gomesalexandre gomesalexandre changed the title feat: make lp opportunities great ~~again~~ feat: make lp opportunities great Feb 19, 2025
Copy link
Member

@0xApotheosis 0xApotheosis left a comment

Choose a reason for hiding this comment

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

✅ Ensure that you can see your LP read-only opportunities in the DeFi section, vs. empty when expanding previously
✅ No LP opportunities are displayed as Eligible in the dashboard
✅ There is no dead link button in opportunities (View/Manage)

Noting that we still have the wagmi issue (I think due to missing assetReference for some assets), but that is not a regression in this PR.

Screenshot 2025-02-20 at 16 16 22

Also 2 nitpicks I'd prefer to see fixed.

Base automatically changed from feat_by_asset_2 to develop February 20, 2025 10:02
@gomesalexandre gomesalexandre enabled auto-merge (squash) February 20, 2025 10:13
@gomesalexandre gomesalexandre merged commit a1c5161 into develop Feb 20, 2025
3 checks passed
@gomesalexandre gomesalexandre deleted the feat_by_asset_3 branch February 20, 2025 10:19
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.

2 participants