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

Display external incentive APR #886

Merged
merged 14 commits into from
Oct 21, 2022
Merged

Conversation

srph
Copy link
Contributor

@srph srph commented Oct 6, 2022

This is for the pool gauges in the pool page (/pool/[id]):

image

For Stride, we'd like to display the sum of external and internal incentives.

This is how it looks like with the mentioned changes:

image

Relevant changes I've introduced are in packages/web/pages/pool/[id].tsx. In addition, I thought not every pool might like this change, so I've thought of putting this configuration behind a flag.

I'd like to kindly ask for some pointers on how to implement this properly. Apparently, I've hard-coded some values (like dividing the values by 1m); and I'm not very familiar with Keplr's Dec and RatePretty as well.

If there are variables or files I should take note of - that would be helpful.

I'll mark this as a draft PR once I have more idea on how to implement this properly

@srph srph requested a review from JeremyParish69 as a code owner October 6, 2022 21:06
@vercel
Copy link

vercel bot commented Oct 6, 2022

@srph is attempting to deploy a commit to the OsmoLabs Team on Vercel.

A member of the Team first needs to authorize it.

@jonator jonator linked an issue Oct 7, 2022 that may be closed by this pull request
@vercel
Copy link

vercel bot commented Oct 7, 2022

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

Name Status Preview Updated
osmosis-frontend ✅ Ready (Inspect) Visit Preview Oct 20, 2022 at 9:34PM (UTC)
osmosis-frontier ✅ Ready (Inspect) Visit Preview Oct 20, 2022 at 9:34PM (UTC)

@jonator
Copy link
Member

jonator commented Oct 7, 2022

I'd def appreciate it if you could implement this, as my plate has been quite full as of recently.

I've started work on calculating external incentive here:

readonly computeExternalIncentiveGaugeApr = computedFn(

You could continue my work there. I'd like for it to be a callable function that we could then use anywhere.

@srph
Copy link
Contributor Author

srph commented Oct 11, 2022

Hey @jonator thanks for advice. I have 2 questions:

  • Are we expecting this to replace the current APRs in the PoolGaugeCards?
  • Is computeExternalIncentiveGaugeApr complete? If so, are there any specific variables we had in mind that should be passed to it?

@srph
Copy link
Contributor Author

srph commented Oct 11, 2022

I have another question:

So, we have a variable called allowedGauges which is an array that displays the "external incentives" (in our case STRD) which is this:

image

Which we'll use to add to the APRs:

image

What I did was pretty simple - add allowedGauges[0].rewardAmount to each APR gauge (1/7/14).

What about when there are more than allowedGauges?

@srph
Copy link
Contributor Author

srph commented Oct 12, 2022

@jonator Hey, thanks for the discussion earlier!

I've updated the PR based on the direction I understood we were taking. It's likely I missed something - let me know if there's anything to change or improve.

Here's how the UI looks now for pool 803:

image

}

const amount = externalGauge.rewardAmount.moveDecimalPointLeft(
mintCurrency.coinDecimals
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Diving the value by 1_000_000 gives me the value I expect, but I figured hard-coding doesn't make sense. So I took this from computeAPYForDuration. Let me know I should be getting this value elsewhere.

@jonator
Copy link
Member

jonator commented Oct 12, 2022

for pool 803, I believe the 1 day and 7 day gauges should be 0% since there's no incentives for those gauges.

The only gauge I expect to see an APR for external incentives is the 14 day gauge.

packages/web/config/feature-flag.ts Outdated Show resolved Hide resolved
@@ -118,6 +118,52 @@ export class ObservableQueryIncentivizedPools extends ObservableChainQuery<Incen
}
);

readonly computeAPYWithExternalIncentives = computedFn(
Copy link
Member

Choose a reason for hiding this comment

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

As Aidan and I agreed in the meeting, IMO we should calculate APR for a duration for external gauges separately from internal gauges.

Copy link
Member

Choose a reason for hiding this comment

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

Then, we add them in the view layer.

This way, we would be able to separately display the APR for just external gauges of a certain duration.

Copy link
Contributor Author

@srph srph Oct 13, 2022

Choose a reason for hiding this comment

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

Then, we add them in the view layer.

@jonator Hmm, I'm not sure I'm able to follow. Can you help me recall this bit?

  • Are we looking to add another gauge UI below the internal incentives?
  • Are we looking to do add something similar to superfluidApr? Like 0.2% + 18% + 5%?
  • Or instead do we simply want to refactor so we're calling this new function in the pool page like so:
queryOsmosis.queryIncentivizedPools.computeAPY(...).add(
  queryOsmosis.queryIncentivizedPools.computeExternalIncentiveForDuration(...)
)

Copy link
Member

Choose a reason for hiding this comment

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

Yeah we can use that code snippet for our current needs, but we'd also have those bullet points available as an option.

Copy link
Member

Choose a reason for hiding this comment

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

Reminder: we're currently redesigning the pool detail page in a major way, so flexibility in our APIs is extra critical.

I know for a fact we will be displaying external incentive APRs in a different way.

@srph
Copy link
Contributor Author

srph commented Oct 13, 2022

@jonator Thanks for the notes. I've updated the PR again based on what I understood. Let me know if we're headed in the right direction or if there's anything else to improve.

@JeremyParish69
Copy link
Collaborator

This is exciting!
Due top the temporal nature of external incentives, I'd hope that the numbers aren't simply just added together, but rather that each gauge gets it's own APR figures. Might be confusing to advertise just a 200% APR when the 190% APR external incentive gauge will end after just one more epoch. A way to show internal and external separately would be better for users.

@srph
Copy link
Contributor Author

srph commented Oct 13, 2022

@JeremyParish69 Yea, I think that should be the effect when this PR is merged. This is the effect at least for pool 803:

Before After
image image

Given that it only has one external incentive:

image

I could update it to do something similar to superfluid pools and show something like 0.2% + 18% if we think that's a better option (haha a bit unsure which is preferable).

@srph srph requested review from jonator and removed request for JeremyParish69 October 13, 2022 21:59
Copy link
Member

@jonator jonator left a comment

Choose a reason for hiding this comment

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

@srph It appears there's build errors in [id].tsx

@srph
Copy link
Contributor Author

srph commented Oct 17, 2022

@jonator Should be fixed now; I verified it by running yarn build:frontier. Let me know if there's anything I may have missed.

@srph srph requested a review from jonator October 17, 2022 19:27
@jonator
Copy link
Member

jonator commented Oct 19, 2022

What was the rationale for how you calculated the APR? Seems like a clever approach.

@srph
Copy link
Contributor Author

srph commented Oct 19, 2022

@jonator I don't think I have a solid rationale about it. Last time I tried without notes, (existing APR from computeApy) + (rewardAmount / 1_000_000) gave me the value we expected.

I figured 1_000_000 came from coinDecimals, and closest I found where I could get it was mintCurrency (I took references from computeAPYForSpecificDuration).

Also to be honest, I'm not too familiar with CoinPretty/RatePretty in practice. If there's a better way to do this, let me know!

@jonator
Copy link
Member

jonator commented Oct 19, 2022

https://github.com/chainapsis/keplr-wallet/blob/b4560027b8a955e092a2ff14e34281ad1812370a/packages/stores/src/query/cosmos/supply/inflation.ts#L176

This is the code Keplr uses to compute stake APR. May be a helpful reference. I know it helped my fix an issue with calculating swap fee APR.

@srph
Copy link
Contributor Author

srph commented Oct 20, 2022

@jonator I consulted with the folks at Stride and used computeApy as reference. How does this look @jonator?

image

packages/web/pages/pool/[id].tsx Outdated Show resolved Hide resolved
@srph srph requested a review from jonator October 20, 2022 19:15
@srph
Copy link
Contributor Author

srph commented Oct 20, 2022

@jonator Oops, good catch. Should be fixed now.

I left the formula as comment for the time being - I think it makes sense to have it there. Should I remove it as well?

Copy link
Member

@jonator jonator left a comment

Choose a reason for hiding this comment

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

LGTM

@jonator jonator changed the title Pool Gauge: Display sum of external & internal incentives Display external incentive APR Oct 21, 2022
@jonator jonator merged commit 726fc68 into osmosis-labs:master Oct 21, 2022
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.

Display external incentive APRs for whitelisted gauges
3 participants