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

chore(proto): move optimistic block protos to v1 #1707

Merged
merged 1 commit into from
Nov 19, 2024

Conversation

itamarreif
Copy link
Contributor

@itamarreif itamarreif commented Oct 21, 2024

Summary

This updates the protos used for the auctioneer to be based on the new v1 protos.

Background

We bumped our proto definition versions from v1alpha_ to v1.

Changes

  • Moved the sequencerblock/optimistic_block file into its own subpackage in order to have sequencerblock.optimisticblock.v1alpha1
  • Updated to use FilteredSequencerBlock from v1

Breaking Changelist

  • This shouldn't break anything as none of these protos are used by anything in main currently.

closes #1767

@itamarreif itamarreif self-assigned this Oct 21, 2024
@itamarreif itamarreif requested review from a team as code owners October 21, 2024 23:42
@github-actions github-actions bot added the proto pertaining to the Astria Protobuf spec label Oct 21, 2024
@itamarreif itamarreif marked this pull request as draft October 21, 2024 23:44
@itamarreif itamarreif marked this pull request as ready for review October 21, 2024 23:49
@itamarreif itamarreif force-pushed the itamarreif/auctioneer/protos-v1-update branch from b1711e8 to f27c8d6 Compare October 23, 2024 16:23
@itamarreif itamarreif force-pushed the itamarreif/auctioneer/protos-v1-update branch from f27c8d6 to 00b2ae6 Compare October 23, 2024 16:44
Copy link
Collaborator

@noot noot left a comment

Choose a reason for hiding this comment

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

auctioneer isn't shipped yet, maybe this should stay v1alpha1 for now?

@itamarreif
Copy link
Contributor Author

auctioneer isn't shipped yet, maybe this should stay v1alpha1 for now?

The auctioneer stuff remains v1alpha1 here, it is just updated to use the correct (v1) version of everything else.

see: https://github.com/astriaorg/astria/pull/1707/files#diff-816642b078e8e91f48aba99b3f04ffaed46960ef63a4c8d22ae068ffebd9d6f7R3

@SuperFluffy
Copy link
Member

Have to push back on this naming convention. v1 sequencer block with v1alpha1 optimistic block? We should have probably not nested the optimistic block bits under sequencerblock in this way, but provided a structure like in the protocol APIs.

I am not sure how to best tackle this, but I feel like going with astria.optimisticblock.v1alpha1 (i.e. a sibling instead of a child) that imports astria.sequencerblock.v1 is more natural.

Would like to pull @joroshiba into this.

Copy link
Member

@joroshiba joroshiba left a comment

Choose a reason for hiding this comment

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

LGTM, think this is good sane versioning for the proto. Agreed with Janis on naming service.proto as a nit.

@itamarreif itamarreif added this pull request to the merge queue Nov 19, 2024
Merged via the queue into main with commit 43349db Nov 19, 2024
46 checks passed
@itamarreif itamarreif deleted the itamarreif/auctioneer/protos-v1-update branch November 19, 2024 06:38
@SuperFluffy
Copy link
Member

SuperFluffy commented Nov 19, 2024

This unfortunately imports the wrong generated code and doesn't name it according to our convention.

This PR should not have been merged.

(It also lacks a CHANGELOG).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auctioneer proto pertaining to the Astria Protobuf spec
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update Auctioneer protos to use v1
4 participants