-
Notifications
You must be signed in to change notification settings - Fork 316
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
feat: lp indexing #4771
feat: lp indexing #4771
Conversation
This is a good start, but I think we want to compute some more derived information for use in the explorer, for example the price, so that the explorer can easily sort and fetch by price. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's clean up the accidental formatting changes and make sure that we have all the information we need to implement the APIs.
Downgrading to "draft", mostly to signal that we don't intend to block on upcoming point-release until this lands. Feel free to promote back to ready for review whenever, @vacekj! |
A few more things I noticed:
|
@cronokirby I addressed your feedback:
Question about #: I might be misunderstanding, but the price here should just be reserves_1/reserves_2 right? I think we can calculate this on the fly in queries, but I can also add it to the view if necessary. let me know how this looks to you @conorsch @cronokirby also, who else should i be tagging ƒor reviews? don't wanna overload just aubrey, conor and lucas 😃 |
The problem with calculating the price as based on reserves is that it changes over time, as the reserves change, but in fact the price is static. It's based on https://buf.build/penumbra-zone/penumbra/docs/main:penumbra.core.component.dex.v1#penumbra.core.component.dex.v1.BareTradingFunction as present in the position. If you're trading asset1 into asset2, the price is p / q, and if you're doing the opposite, it's q / p. For the effective price, which is what the dex explorer uses, you also need to take fees into account, so you get a worse price. |
@cronokirby this leads me to believe we then have all we need in the data? Can we proceed to review and merge? |
I think we want:
but more broadly with this one we can actually test that it works by modifying the dex explorer raw indexing database queries to use it, and we should do that. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would also be nice if we didn't create a diff over the parts of the database we haven't changed, we can format in a separate PR
1784abb
to
ec84a54
Compare
ec84a54
to
031493d
Compare
This restructures the dex tables to be a bit richer and de-duplicated
031493d
to
992dbaa
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me now, did some brief testing as well
Because it's unitless, we need decimals, woops.
992dbaa
to
1848b6e
Compare
0c70025
to
6139479
Compare
6139479
to
5b774fc
Compare
Describe your changes
implements lp positions and execution indexing
TODO from reviews:
Issue ticket number and link
fixes #4739
Checklist before requesting a review
indexer-only changes