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

Make pindexer more easily embeddable as a library #4712

Merged
merged 7 commits into from
Jul 23, 2024
Merged

Conversation

cronokirby
Copy link
Contributor

Describe your changes

We don't expose cometindex as a library, but we do pindexer, and this adds a few tweaks to make pindexer possible to run inside an external piece of Rust code. This is useful, because it allows people to colocate their indexer and their block explorer, for example.

Checklist before requesting a review

  • 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:

    Doesn't touch the core protocol.

Self {
opts: Options::parse(),
opts,
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 not sure this is desirable -- the original API was structured that way so that downstream consumers didn't have control over the CLI. Everyone gets the same binary, the only difference is which AppViews are hooked into it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you want to hook in different AppViews you have to consumer pindexer as a library, in which case having control over your CLI seems much better to me, and doesn't cause odd spooky behavior.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't fully understand the disagreement about exposing CLI opts: in the interest of making pindexer more accessible, e.g. via importing into other rust code, I'm biased toward making this change and dealing with the consequences later.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, this change should be rolled back, it's an explicit design goal to not allow control over the CLI, so that every Penumbra indexer has the same interface.

Necessary to allow implementing AppView
Makes it much more convenient to implement AppView
This allows indexer components to read the cometbft database, if
necessary. This is desirable for more advanced use cases.

As a safeguard, we set the connection to read only. This can be
overwritten by a savvy user, but this prevents basic accidental mistakes
from modifying the cometbft database.
This is better than parsing inside the library
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, to unblock ongoing pindexer workstreams. There is still a lingering disagreement about how to handle CLI args wrt to downstream consumers. Let's figure that out as we go, and not block here on it!

@cronokirby cronokirby merged commit a2ffd8a into main Jul 23, 2024
13 checks passed
@cronokirby cronokirby deleted the pindexer-tweaks branch July 23, 2024 17:55
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.

3 participants