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

fix: display runepool name and rune asset #7540

Merged
merged 5 commits into from
Aug 15, 2024
Merged

fix: display runepool name and rune asset #7540

merged 5 commits into from
Aug 15, 2024

Conversation

NeOMakinG
Copy link
Collaborator

Description

We are displaying THORChain instead of RUNEPool in the opportunities

We are displaying Need some THORChain? instead of Need some RUNE? in the overview

I added some patches so if it's runepool, we are displaying the right names, while it might not be the best solution, it's actually the best quickfix I could find

Pull Request Type

  • 🐛 Bug fix (Non-breaking Change: Fixes an issue)
  • 🛠️ Chore (Non-breaking Change: Doc updates, pkg upgrades, typos, etc..)
  • 💅 New Feature (Breaking/Non-breaking Change)

Issue (if applicable)

closes #7526

Risk

Very low

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

Testing

  • Go to the earn opportunity list
  • See that RUNEPool is displayed instead of THORChain
  • If you don't have any liquidity in it, open the overview page by clicking the THORChain row and see Need some RUNE? instead of Need some THORChain?

Engineering

n/a

Operations

n/a

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

Screenshots (if applicable)

image
image

@NeOMakinG NeOMakinG requested a review from a team as a code owner August 12, 2024 07:59
Copy link
Contributor

@gomesalexandre gomesalexandre left a comment

Choose a reason for hiding this comment

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

While this does fix things according to #7526, I'm not sure about this one and we should probably strive for consistency rather than following what may or may not have been a misunderstanding from ops 🙏🏽

  • Asset rows are now inconsistent, i.e what should be the asset name is now the opportunity name (monkey patched as RUNEPool)
image
  • THORChain is still shown as a reward asset vs. RUNE
image
  • Overview shows "RUNEPool - THORChain savers" which is technically wrong - the only reason why RUNEPool is part of THORChain savers opportunity is because of our own code reuse, however, savers !== RUNEPool. Though this one may not be tackled now (and possibly never) - we would need to explicitly decouple savers and RUNEPool and having RUNEPool as part of savers may actually be "sufficiently fine" from a user-perspective (i.e whether RUNEPool is part of savers or not is a UX nitpick and one could argue there's nothing wrong as showing it like so, as if it looks like savers, walks like savers and quacks like savers, it effectively is savers).
image

@NeOMakinG think we should probably take a second before adding more monkey patches here, and think of a way to follow our DeFi abstraction conventions:

  • as @kaladinlight mentioned, we should make consistent use of asset symbol vs. name across the abstraction (which would solve the THORChain vs. RUNE issue)
  • thankfully, StakingEarnOpportunityType already has opportunityName which leverages opportunity.name (which itself uses symbols). Not sure why we don't make consistent use of this, but this should realistically be all we need. See fix: display runepool name and rune asset #7540 (comment)

@NeOMakinG
Copy link
Collaborator Author

I'm align with your idea @gomesalexandre, can we double check with @shapeshift/product of they are too?

I've done it in 0ec463b

And here is the final result
image

Copy link
Contributor

@gomesalexandre gomesalexandre left a comment

Choose a reason for hiding this comment

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

Tested locally, confirmed this does what it says on the box:

  • RUNEPool
image image image
  • Other savers
image image image
  • Other DeFi opportunities
image image

Two suggestions here before we land this:

  1. use RUNE symbol for consistency vs. RUNEPool, see https://github.com/shapeshift/web/pull/7540/files#r1718488923
  2. Remove the Vault suffix to keep naming aligned with the current develop one
    name: `${underlyingAsset.symbol} Vault`,

@NeOMakinG NeOMakinG enabled auto-merge (squash) August 15, 2024 14:58
@NeOMakinG NeOMakinG merged commit f662a49 into develop Aug 15, 2024
3 checks passed
@NeOMakinG NeOMakinG deleted the 7526 branch August 15, 2024 15:09
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.

RUNEPool listed as THORChain in Savers
3 participants