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: savers read-only opportunities #8878

Merged
merged 13 commits into from
Feb 20, 2025
Merged

feat: savers read-only opportunities #8878

merged 13 commits into from
Feb 20, 2025

Conversation

gomesalexandre
Copy link
Contributor

@gomesalexandre gomesalexandre commented Feb 18, 2025

Description

Makes THORChain savers opportunity view-only and balance-only: can only see them if you have a balance, and can't interact with them through the app's modal but instead are linked to THORChain's X for keeping up-to-date with updates.

Issue (if applicable)

closes #8866

Risk

High Risk PRs Require 2 approvals

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

Low, risk of borked opportunities, though should be pretty easy to test given we've removed the "By Provider" view

Testing

  • If there's no THORChain savers balance, no THORChain savers opportunity should be displayed
  • Savers opportunities are displayed if there's an active balance only, and manage is a read-only link to THORCHain's X

Engineering

  • ^

Operations

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

Screenshots (if applicable)

https://jam.dev/c/3bda7b99-8dbe-4e2d-a3e6-dd70d9e7aa71
https://jam.dev/c/fab5c32b-fa34-4b74-af29-d76eabe8a614

@gomesalexandre gomesalexandre requested a review from a team as a code owner February 18, 2025 18:10
Base automatically changed from feat_by_asset to develop February 19, 2025 02:53
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.

First functional pass:

We now have an external link icon that does nothing:

Screenshot 2025-02-19 at 18 27 37

Clicking "Manage" does indeed open the X link, but also opens the modal, which feels really odd.

https://jam.dev/c/47a4391d-b223-4f9e-8dc5-28a7cbc09dc4

@gomesalexandre
Copy link
Contributor Author

gomesalexandre commented Feb 19, 2025

Thanks for the great functional pass @0xApotheosis!

4 birds, one stone https://jam.dev/c/ec57988d-6cd1-4f78-93ea-5cf32b20e4b9 :

  • Remove the useless read-only icon from left side: 98fc80a
  • Rename "Manage" to "View" for read-only: 98fc80a (felt weird to call this manage but this isn't necessarily true, and definitely is not in the case of THORChain savers)
  • Fix the "Manage" modal opening when it should not for read-only: 98fc80a
  • Add migration to ensure all clients have the updated opportunities meta/data: e109d0a

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.

One potential edge-case bug, but happy to get this in in the name of progress.

@gomesalexandre gomesalexandre enabled auto-merge (squash) February 20, 2025 09:56
@gomesalexandre gomesalexandre merged commit dc15c0d into develop Feb 20, 2025
3 checks passed
@gomesalexandre gomesalexandre deleted the feat_by_asset_2 branch February 20, 2025 10:02
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.

Clean up earn page
2 participants