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: add dex block summary #5063

Open
wants to merge 17 commits into
base: main
Choose a base branch
from
Open

Conversation

erwanor
Copy link
Member

@erwanor erwanor commented Feb 6, 2025

Describe your changes

WIP pairing

Issue ticket number and link

penumbra-zone/dex-explorer#338

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:

    REPLACE THIS TEXT WITH RATIONALE (CAN BE BRIEF)

@erwanor erwanor removed their assignment Feb 10, 2025
@conorsch
Copy link
Contributor

@JasonMHasperhoven and I paired a bit to debug the failing smoke test. To investigate, I pulled down this branch, ran just dev to spin up the fullnode indexing config, and quickly identified the specific error that pindexer was showing:

pindexer-debug-2

That error, in text, is:

Error: error returned from database: column "batch_swaps" is of type batch_swap_summary[] but expression is of type jsonb
Caused by:
    column "batch_swaps" is of type batch_swap_summary[] but expression is of type jsonb

Clear enough: we've got to massage the types a bit. Typically we use JSON blobs to store Penumbra types in postgres via pindexer, but as the spec in penumbra-zone/dex-explorer#338 indicates, we don't want to do that here. Instead, we want a full custom type inside the db. I took a stab at implementing sqlx::Encode for the BatchSwapSummary type, but wasn't able to finish it. The build-time error I'm getting now is:

error[E0277]: the trait bound `for<'a> &'a [BatchSwapSummary]: sqlx::Encode<'_, Postgres>` is not satisfied
    --> crates/bin/pindexer/src/dex_ex/mod.rs:1270:16
     |
1270 |         .bind(&batch_swap_summaries)
     |          ----  ^^^^^^^^^^^^^^^^^^^^ the trait `for<'a> sqlx::Encode<'_, Postgres>` is not implemented for `&'a [BatchSwapSummary]`, which is required by `&Vec<BatchSwapSummary>: sqlx::Encode<'_, Postgres>`

Subsequent work should pick up from there. Take the Encode impl with a grain of salt, @JasonMHasperhoven , because this code isn't actually working yet! But hopefully this is one step closer.

@erwanor erwanor assigned erwanor and unassigned JasonMHasperhoven Feb 12, 2025
-- Primary key
rowid SERIAL PRIMARY KEY,
-- The height of the block.
height INTEGER NOT NULL,
Copy link
Member Author

Choose a reason for hiding this comment

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

@cronokirby noticed that some inserts fail if we are adding a UNIQUE constraint on height, but my understanding is that this part of the pipeline should have exactly once semantics on delivery. is that right?

wondering if this is caused by the catchup logic: https://github.com/penumbra-zone/penumbra/blob/erwan/pindexer_block_summary/crates/util/cometindex/src/indexer/indexing_state.rs#L271 (<= vs. <). unsure.

@erwanor erwanor marked this pull request as ready for review February 12, 2025 23:03
@erwanor
Copy link
Member Author

erwanor commented Feb 12, 2025

AFAICT this is good to go, the integration test seems to timeout because of the commitment source addition.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: 🏗 In progress
Development

Successfully merging this pull request may close these issues.

3 participants