-
Notifications
You must be signed in to change notification settings - Fork 316
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
pindexer(dex_ex): index batch swap execution traces #4990
Conversation
To review, I've kicked off a reindex of a mainnet db using this binary. As long as that completes without error, I'll call it good enough, and get it in. |
Encountered an error:
Investigating... |
Thanks, downloading the mainnet db, it's taking a second |
asset_start, | ||
asset_end, | ||
price_float, |
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.
asset_start, | |
asset_end, | |
price_float, | |
price_float, | |
asset_start, | |
asset_end, |
to match the order stuff gets passed in later.
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.
Thanks!
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.
I think there's currently a bug in the data insertion method.
asset_end, | ||
asset_hops, | ||
amount_hops, | ||
position_id_hops, |
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.
position_id_hops, | |
position_id_hops |
I think this should fix the SQL error (no trailing commas allowed :()
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.
Tested again locally on e079188 (the most recent commit), and this time I was able to complete the reindex based on mainnet events. So LGTM, with the changes that have been added.
Intentionally not merging until @cronokirby has had a chance to look closely, and updates their requested-changes status. |
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.
All good for me!
Describe your changes
This PR adds a
dex_ex_batch_swap_traces
table that tracks batch swap execution traces.We do not (yet) track individual position ids for each subtrace hops, so this is left empty.
Notably, the schema contains a price float, but no input/output amount floats for now. This is good to have, but we can add it later and it's not immediately useful to the intended consumer of this data.
The schema:
Checklist before requesting a review
I have added guiding text to explain how a reviewer should test these changes.
If this code contains consensus-breaking changes, I have added the "consensus-breaking" label. Otherwise, I declare my belief that there are not consensus-breaking changes, for the following reason: