sequencer-types: type all errors #376
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
noot
reviewed
Sep 18, 2023
SuperFluffy
force-pushed
the
superfluffy/type-sequencer-types-errors
branch
from
eb71599
to
0289103
Compare
Base automatically changed from
superfluffy/type-sequencer-validation-errors
to
main
September 18, 2023 16:10
github-actions
bot
added
the
sequencer-relayer
github-actions
bot
removed
the
sequencer-relayer
joroshiba
approved these changes
Sep 18, 2023
joroshiba
approved these changes
Sep 18, 2023
steezeburger
added a commit
that referenced
this pull request
Sep 18, 2023
steezeburger
added a commit
that referenced
this pull request
Sep 19, 2023
* main: update ct-merkle dependency (#377) Add just commands for lint, update readme (#367) sequencer-types: type all errors (#376) sequencer-validation: type errors (#375) conductor: remove defaults from configuration (#380) Update Conductor config management (#329) fix(sequencer): remove unneeded `AppHash` alias (#362) remove `Namespace` from `SequencerBlockData`, use `ChainId` instead (#346) feat(sequencer): implement generating and verifying chain ID commitment (#338) sequencer-relayer: update config to astria standard (#336) Conductor: More structured logging (#335) proto: regenerate (#350) proto: remove concept of head from execution api (#344) feat(sequencer-types): verify `last_commit_hash` when constructing `SequencerBlockData` (#327) feat(conductor): implement `RollupNamespaceData` verification inside conductor (#290) remove alert actor from conductor (#324) sequencer: remove todo for issue that's done (#326)
SuperFluffy
added a commit
that referenced
this pull request
Sep 20, 2023
…blocks (#382) ## Summary Add a `SequencerSubscriptionClientExt` trait with a `subscribe_new_block_data` method. ## Background Removing gossipnet from conductor as part of #332 requires it to read new blocks directly from cometbft / sequencer. This patch extends the sequencer-client crate to provide that functionality using a convenience function so that conductor need not know how to convert comertbft new-block events to sequencer block data. ## Changes - Add a `SequencerSubscriptionClientExt` trait to the sequencer-client crate. - Reorganize the sequencer-client crate to export items under the appropriate features. - warn users if they set neither the `http` nor `websocket` features. ## Testing Testing this client is out of scope for this PR. We are relying on the underlying tendermint-rpc WebSocketClient being correctly implemented. It would be great to extend wiremock or a similar crate to provide a quick way to test websocket APIs. An alternative is to derive a jsonrpc server with jsonrpsee, but that's a lot more work. ## Breaking Changelist None, only functionality was added crates depending on sequencer-client were not touched. ## Related Issues Part of #332 #375 and #376 are precursor PRs unblocking this patch. --------- Co-authored-by: noot <36753753+noot@users.noreply.github.com>
SuperFluffy
added a commit
that referenced
this pull request
Sep 20, 2023
…383) ## Summary This patch replaces gossipnet by a websocket stream of new blocks directly from sequencer. ## Background Adding a p2p layer adds a lot of complexity to astria services. With this patch conductor reads new blocks directly off of cometbft/sequencer, making it much simpler. For background reading, see this issue: #325 ## Changes - Replace gossipnet directly by a stream of new blocks using the cometbft subscrition RPC. - No other changes in regard to how blocks are further processed has been made. ## Testing We are relying on the rust tendermint-rpc crate functioning properly. As a follow-up it makes sense to create blackbox tests for conductor, which are currently absent. ## Breaking Changelist - Reading over p2p is no longer supported. Deployments must be updated and wired to read off of cometbft/sequenceer. ## Related Issues Precursor PRs unblocking this work: #375 #376 #382 closes #332 --------- Co-authored-by: Emilia Hane <emiliaha95@gmail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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-typeshas to be made a dependency ofsequencer-client, which only uses typed errors. Thereforesequencer-typesmust have typed errors (because otherwisesequencer-clientwould be "infected" by eyre, which would require changing all of sequencer-client to also use eyre.Changes
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
Related Issues
#12
#86
#366
#332