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
@itamarreif itamarreif requested review from Fraser999 and noot 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.

@SuperFluffy SuperFluffy requested a review from joroshiba October 29, 2024 13:52
@itamarreif itamarreif force-pushed the itamarreif/auctioneer/protos-v1-update branch from 9a65f20 to b9e041c Compare October 30, 2024 16:49
@itamarreif itamarreif force-pushed the itamarreif/auctioneer/protos-v1-update branch from b9e041c to 76f6587 Compare October 30, 2024 16:52
@itamarreif itamarreif force-pushed the itamarreif/auctioneer/protos-v1-update branch from 76f6587 to 711bba5 Compare October 30, 2024 16:53
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 force-pushed the itamarreif/auctioneer/protos-v1-update branch from 711bba5 to d1edce9 Compare November 7, 2024 19:49
@itamarreif itamarreif force-pushed the itamarreif/auctioneer/protos-v1-update branch from d1edce9 to 4287b94 Compare November 18, 2024 03:09
@itamarreif itamarreif requested a review from joroshiba November 19, 2024 06:23
@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).

github-merge-queue bot pushed a commit that referenced this pull request Dec 5, 2024
…c match (#1825)

## Summary
Deletes and regenerates all code in `astria-core/src/generated`. Fixes
bad imports by nesting generated Astria APIs under a new `astria`
module. Fixes badly named modules.

## Background
#1236 noticed that some protobuf
files were skipped during code generation because of a
`tonic_build::Builder::extern_path` statement. This showed up again when
reworking the protobuf compilation tool to first clear the target
directory for storing the generated rust files, where now an `include!`
statement in `astria_core::generated` failed because the file was not in
fact generated. This was not detected because the file was still
committed to the repository at an earlier point.

#1707 then added new generated
code under a wrongly named `optimistic_block` module, which should have
been named `optimistic` to be in line with its protobuf package
counterpart.

The present change is primarily to the `tools/protobuf-compiler` binary,
which first purges the output directory (minus the handwritten `mod.rs`)
before generating new code.

## Changes
- Update `tools/protobuf-compiler` to clear `astria-core/src/generated`
prior to repopulating it from `proto/`
- Remove the module `sequencerblock::optimisticblock`
- Add module `sequencerblock::optimistic`
- Update all services that import Astria APIs from
`astria_core::generated` to `astria_core::generated::astria`.

## Testing
This is just code organization. Import paths were updated, code still
compiles and tests pass.

## Changelogs
Changelogs updated.

## Breaking Changelist
- This is a breaking change for all consumers of `astria_core` that now
need to import many items from `astria_core::generated::astria` that
were directly under `astria_core::generated` before.

## Override Freeze
This code touches services by changing their imports
(`astria_core::generated -> astria_core::generated::astria`) but does
change any implementation details.

## Related Issues
Fixes and amends #1707.
Supersedes #1824
Closes #1823
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