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

Governance support for pindexer #4708

Merged
merged 19 commits into from
Aug 15, 2024
Merged

Governance support for pindexer #4708

merged 19 commits into from
Aug 15, 2024

Conversation

plaidfinch
Copy link
Collaborator

@plaidfinch plaidfinch commented Jul 14, 2024

Describe your changes

This adds a governance module to pindexer, along with tiny changes to pd's governance events to fully support it.

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:

    The only changes to pd are changes to events emitted, which do not break consensus.

Copy link
Collaborator Author

@plaidfinch plaidfinch left a comment

Choose a reason for hiding this comment

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

This self-review attempts to explain the changes made, largely focusing on the changes made to pd, as changes to pindexer are self-contained and still in testing/development.

crates/core/component/governance/src/component.rs Outdated Show resolved Hide resolved
crates/core/component/governance/src/component.rs Outdated Show resolved Hide resolved
crates/core/component/governance/src/component/view.rs Outdated Show resolved Hide resolved
crates/core/component/governance/src/proposal.rs Outdated Show resolved Hide resolved
Copy link
Member

@hdevalence hdevalence left a comment

Choose a reason for hiding this comment

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

There are two high-level changes that need to be made before this can be merged:

  • There should be no changes, even refactorings, to consensus-critical code along the way to adding indexer support. At most, new data can be emitted in events. We don't have bandwidth to carefully review changes to consensus-critical logic along the way to adding this logic.

  • The data produced by the AppView implementation is expected to be consumed by third-party clients. Thus, the AppView impl should not be defining its own data formats ad-hoc. It should be using the canonical data formats we already have, which can be processed using standardized tooling from the Buf registry.

Copy link
Collaborator Author

@plaidfinch plaidfinch left a comment

Choose a reason for hiding this comment

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

All changes, even refactors, to consensus-critical code have been backed out, excepting alterations to emitted events.

Copy link
Collaborator Author

@plaidfinch plaidfinch left a comment

Choose a reason for hiding this comment

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

More self-review on the latest iteration, which I believe resolves the outstanding concerns above.

@hdevalence
Copy link
Member

Running this, I get

Error: error returned from database: bind message supplies 0 parameters, but prepared statement "sqlx_s_2" requires 1

Caused by:
    bind message supplies 0 parameters, but prepared statement "sqlx_s_2" requires 1

@plaidfinch
Copy link
Collaborator Author

plaidfinch commented Jul 17, 2024 via email

@zbuc
Copy link
Member

zbuc commented Jul 22, 2024

LGTM. @plaidfinch is this ready to merge?

@zbuc zbuc self-requested a review July 22, 2024 16:57
@hdevalence
Copy link
Member

This code should have been merged several days ago when @plaidfinch and I pushed it up to the finish line and it was reviewed and approved. It needs to go in as-is, not be rewritten again.

This fixes a bug where only one of the tables was edited.
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.

Tested locally in dev env, works well. My earlier comments about the proto namespaces have been addressed. I see no reason not to ship this. Separately let's follow up with pindexer-focused docs that stipulate the version-matching and genesis-preservation reqs.

@conorsch conorsch mentioned this pull request Aug 15, 2024
4 tasks
@cronokirby cronokirby merged commit d6422bf into main Aug 15, 2024
13 checks passed
@cronokirby cronokirby deleted the pindexer-governance branch August 15, 2024 02:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

5 participants