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

Add up all external incentive APYs for a specific duration #916

Merged
merged 7 commits into from
Oct 24, 2022

Conversation

srph
Copy link
Contributor

@srph srph commented Oct 21, 2022

In continuation of #886, this gets the sum of all external incentive APYs for the given duration rather than just calculating it from the first incentive.

image

@vercel
Copy link

vercel bot commented Oct 21, 2022

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

A member of the Team first needs to authorize it.

@vercel
Copy link

vercel bot commented Oct 21, 2022

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

Name Status Preview Updated
osmosis-frontier ✅ Ready (Inspect) Visit Preview Oct 21, 2022 at 8:02PM (UTC)

@jonator
Copy link
Member

jonator commented Oct 21, 2022

Actually- can we change it to find the incentives for a specific external gauge (given an ID), rather than adding them together for the given duration?

Switch the API to accept a single gauge in place of the duration, and that also means you no longer need to pass in externalGauges.

Our new design breaks down the APRs by gauge:
Screen Shot 2022-10-21 at 3 38 43 PM

Please call it computeExternalIncentiveGaugeAPR

Thanks

@srph
Copy link
Contributor Author

srph commented Oct 21, 2022

@jonator Some updates:

  • Replaced allowedGauges with gaugeId and denom. Seems denom is needed but not sure how to get it without having to explicitly pass it as a parameter.
  • Moved sum computation in the pool page component.

All in all, I think it should've kept the first commit's behavior:

image

Let me know if there's anything I may have missed. Thanks!

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.

Looks great!

@srph srph requested a review from jonator October 23, 2022 23:17
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.

Sorry noticed one more thing. Should be good after that.

const observableGauge = this.queryGauge.get(gaugeId);

if (
duration.asMilliseconds() !==
Copy link
Member

Choose a reason for hiding this comment

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

I'm realizing that since we take in the gaugeId, there's no real reason to take in the duration as well and perform this check. Can we remove the duration from the params?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jonator Good catch, working on it =D

Copy link
Member

Choose a reason for hiding this comment

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

Please make sure the comment/docs is updated as well.

Copy link
Contributor Author

@srph srph Oct 24, 2022

Choose a reason for hiding this comment

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

I think the main reason why we need duration is - on the pool page - we're simply iterating through the ExternalIncentiveGaugeAllowList[pool.id] (which would contain all gauges regardless of gauge)

Would you like me to move the duration checking there?

Copy link
Member

@jonator jonator Oct 24, 2022

Choose a reason for hiding this comment

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

Yeah we'd need to. I think we agree though it's a pointless constraint on anyone who wants to use this API to find the gauge's duration before being able to use the API.

@srph srph requested a review from jonator October 24, 2022 16:51
@srph
Copy link
Contributor Author

srph commented Oct 24, 2022

Thanks for the review. Let me know if there's anything else I might have missed @jonator

image

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.

Great work! Appreciate it

@jonator jonator merged commit 00415a3 into osmosis-labs:master Oct 24, 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.

2 participants