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

proto: extract sequencer and sequencer-relayer types as protobuf #355

Closed
wants to merge 9 commits into from

Conversation

SuperFluffy
Copy link
Member

Summary

This patch defines protobuf versions of various types that are currently only defined in Rust and exchange as json-serialized text.

Background

astria-proto is intended to be the single source of truth for the wire format of data that is exchanged between services. At the moment SequencerBlockData, SequencerNamespaceData, RollupNamespaceData and InclusionProof are only defined in Rust. Some older versions of these exist as protobuf but were never used in the public interfaces.

Changes

  • Define a sequencer v1alpha2 API for sequencer block, inclusion proof, and celestia blobs (called CelestiaItem and CelestiaHeader).

Testing

  • N/A; currently these are just protobuf definitions.

Related Issues

#12 originally defined sequencer blocks and transactions as protobuf, but these were never actually implemented.

#86 is the reason this effort was started.

noot and others added 4 commits September 8, 2023 21:20
This patch extracts Rust types that are used in communication between
sequencer, sequencer-relayer, conductor, and celestia (as the
data availability layer). The resulting protobuf types are intended to
be the only way that services exchange data, making the current json-based
wire format obsolete.

The protobuf types defined in v1alpha1 of the sequencer API are deprecated
(and were also never used).
@github-actions github-actions bot added conductor pertaining to the astria-conductor crate proto pertaining to the Astria Protobuf spec sequencer-relayer pertaining to the astria-sequencer-relayer crate labels Sep 12, 2023
@SuperFluffy SuperFluffy temporarily deployed to BUF September 12, 2023 14:00 — with GitHub Actions Inactive
@SuperFluffy
Copy link
Member Author

Blocked on #346 being merged

@SuperFluffy SuperFluffy added the sequencer pertaining to the astria-sequencer crate label Sep 12, 2023
@SuperFluffy SuperFluffy requested a review from a team September 12, 2023 14:01
@SuperFluffy SuperFluffy changed the title proto: extract sequencer and sequencer-relayer types are protobuf proto: extract sequencer and sequencer-relayer types as protobuf Sep 13, 2023
@github-actions github-actions bot removed the sequencer pertaining to the astria-sequencer crate label Sep 13, 2023
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.

is this PR just adding the new proto types? can you also delete the duplicate/unused protos inside astria/sequencer/v1alpha1/block.proto and data.proto? i don't think the protos in either of those files are used

@SuperFluffy
Copy link
Member Author

is this PR just adding the new proto types? can you also delete the duplicate/unused protos inside astria/sequencer/v1alpha1/block.proto and data.proto? i don't think the protos in either of those files are used

It is, but I wanted to move this to a v1alpha2 instead to show the evolution of the API. Not a big fan of just removing or changing protos. :-/

@SuperFluffy SuperFluffy temporarily deployed to BUF September 18, 2023 10:14 — with GitHub Actions Inactive
@SuperFluffy SuperFluffy temporarily deployed to BUF September 18, 2023 10:16 — with GitHub Actions Inactive
@noot
Copy link
Collaborator

noot commented Sep 18, 2023

@SuperFluffy I'm quite sure the protos currently in astria/sequencer/v1alpha1/block.proto and data.proto are not used anywhere at all, so you can remove them safely as it doesn't actually affect v1alpha1

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.

can you merge main in? diff looks like it has unrelated changes. otherwise looks good!

SuperFluffy added a commit that referenced this pull request Sep 18, 2023
## Summary
Replace all implicit eyre errors by context specific typed errors

## Background
Same reasoning as for #375 as
part of the work for #332:

`sequencer-types` has to be made a dependency of `sequencer-client`,
which only uses typed errors. Therefore `sequencer-types` must have
typed errors (because otherwise `sequencer-client` would be "infected"
by eyre, which would require changing all of sequencer-client to also
use eyre.

## Changes
- Introduced one error variant per distinct eyre error.

## Testing
N/A. Nothing changed on the code level. Testing that the errors are
actually triggered could be done in a follow-up PR, but big parts of the
code will be replaced by the work in
#355 and follow-up PRs to it,
moving the types exchanged over RPCs to the astria-proto crate.

## Breaking Changelist
- No observable breaking changes in the astria services. Breaking
changes in the library crates because their public API changed (errors),
but we are not considering them right now

## Related Issues
#12
#86
#366
#332
@SuperFluffy
Copy link
Member Author

Something broke github. After squashing and rebasing on top of recent main this PR is no longer in sync with its branch. Opening a new PR.

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

Successfully merging this pull request may close these issues.

3 participants