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.

Sign up for GitHub

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

Jump to bottom

fix(yield): show green banner only when vampire attack is ready #5064

Merged
merged 12 commits into from
Nov 21, 2024
Merged

fix(yield): show green banner only when vampire attack is ready #5064

merged 12 commits into from
Nov 21, 2024

Conversation

shoom3301
Copy link
Collaborator

@shoom3301 shoom3301 commented Nov 5, 2024 •
edited
Loading

Summary

  1. To avoid the banner state flickering, we should wait till we have all necessary data (lp balances and pools data) for its displaying.
image
  1. Make APR label green only when:
  • both lp-tokens are comparable (have the same assets in the pool)
  • COW-AMM lp-token has bigger APR
image image

To Test

  1. Open Yield widget
  • the banner is not displayed
  1. Wait till LP token balances are loaded
  • the banner should appear

@shoom3301 shoom3301 requested review from a team November 5, 2024 09:47
@shoom3301 shoom3301 self-assigned this Nov 5, 2024
@vercel Vercel
Copy link

vercel bot commented Nov 5, 2024 •
edited
Loading

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
cosmos ✅ Ready (Inspect) Visit Preview Nov 21, 2024 0:01am
cowfi ✅ Ready (Inspect) Visit Preview Nov 21, 2024 0:01am
explorer-dev ✅ Ready (Inspect) Visit Preview Nov 21, 2024 0:01am
swap-dev ✅ Ready (Inspect) Visit Preview Nov 21, 2024 0:01am
widget-configurator ✅ Ready (Inspect) Visit Preview Nov 21, 2024 0:01am

@elena-zh
Copy link
Contributor

elena-zh commented Nov 5, 2024 •
edited
Loading

Hey @shoom3301 , thanks :)

I have some questions/issues:

  1. I'd keep the banner to a not connected account
    image
    Develop:
    image
  2. When I change an account, I can see that the banner info might be changed according to a newly connected account's data. Might it be possible to hide the banner until the data is loaded?
  3. I've noticed that banner shows data from a previously connected account. See: I don't have balances for these pools in the currently connected account, but the banner suggests me to swap my assets
    previous account

Thanks!

@shoom3301
Copy link
Collaborator Author

@elena-zh thanks! Fixed all

@elena-zh
Copy link
Contributor

elena-zh commented Nov 5, 2024

Hey @shoom3301 , thank you, but the 3rd case is still here :(
image

@elena-zh
Copy link
Contributor

Hey @shoom3301 , case 2 is fixed, but case 3 is still here :(
Since it is a low-prio issue and less likely will happen on Prod, I can open a separate task for this to address it later

@shoom3301 shoom3301 force-pushed the fix/cow-amm-inline-banner-condition branch from 6b13f03 to 4d9cd0c Compare November 21, 2024 11:31
elena-zh
elena-zh approved these changes Nov 21, 2024
View reviewed changes
Copy link
Contributor

@elena-zh elena-zh left a comment

Choose a reason for hiding this comment

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

Works now!!!

@shoom3301 shoom3301 merged commit cd9f2e1 into develop Nov 21, 2024
11 of 12 checks passed
@shoom3301 shoom3301 deleted the fix/cow-amm-inline-banner-condition branch November 21, 2024 12:05
@github-actions github-actions bot locked and limited conversation to collaborators Nov 21, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@elena-zh elena-zh elena-zh approved these changes

@alfetopito alfetopito alfetopito approved these changes

Assignees

@shoom3301 shoom3301

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@shoom3301 @elena-zh @alfetopito