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

pindexer: dex-explorer: implement low high in summary #4931

Merged
merged 2 commits into from
Nov 18, 2024

Conversation

cronokirby
Copy link
Contributor

Describe your changes

Closes #4929.

This was surprisingly simple, we were already doing an aggregate calculation over the window (summing values) so this was just a matter of adding a low and a high.

To test, index against main and check that nothing else has changed besides the addition of two new fields.

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:

    indexing

@conorsch
Copy link
Contributor

To test, index against main and check that nothing else has changed besides the addition of two new fields.

Locally I've run pindexer against a cometbft db based on most recent HEAD of this repo, then again building pindexer from this PR branch, and both completed without errors. Ensuring the diff between the two dbs is as minimal as described is difficult, so I'm going to approve and keep moving: shortly, after landing a few more pindexer fixes, I'll update the db backing the testnet dex-explorer, which gives us more context to analyze the change here.

@@ -443,7 +443,7 @@ mod summary {
SELECT
COALESCE(SUM(direct_volume), 0.0) AS direct_volume_over_window,
COALESCE(SUM(swap_volume), 0.0) AS swap_volume_over_window,
COALESCE(SUM(trades), 0.0) AS trades_over_window
COALESCE(SUM(trades), 0.0) AS trades_over_window,
Copy link
Contributor

Choose a reason for hiding this comment

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

N.B. if we moved these large queries to a discrete text file and used include_str on them, then we could lint them with sqlfluff cf. #4759 (comment). I tried on this specific error and the tool reported:

L:  40 | P:   5 |  PRS | Line 40, Position 5: Found unparsable section:
                       | 'COALESCE(MIN(price), 0.0) AS low,\n    CO...'

Not convinced that it's worth moving the queries out of the code just to aid a linter, though. I'd thought that sqlx would catch that syntax error at compile time, but no.

@conorsch conorsch self-requested a review November 18, 2024 21:39
Copy link
Contributor

@conorsch conorsch left a comment

Choose a reason for hiding this comment

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

Approving and merging, along with a batch of other pindexer improvements like #4909 & #4932.

@conorsch conorsch merged commit 744e253 into main Nov 18, 2024
14 checks passed
@conorsch conorsch deleted the 4929-dex-summary-high-low branch November 18, 2024 21:40
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.

pindexer: dex-explorer: missing low and high in summary table
2 participants