sequencer-validation: type errors #375
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
github-actions
bot
added
the
sequencer-relayer
SuperFluffy
force-pushed
the
superfluffy/type-sequencer-validation-errors
branch
from
00a5ce9
to
cc3d495
Compare
noot
approved these changes
Sep 17, 2023
|
@noot I like to keep errors close to their use so that the logic is one coherent unit especially if it's so few lines and also only used here. Makes sense especially for bigger errors that are used throughout a codebase though. |
steezeburger
approved these changes
Sep 18, 2023
dereksione
approved these changes
Sep 18, 2023
SuperFluffy
deleted the
superfluffy/type-sequencer-validation-errors
branch
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
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 untyped errors in sequencer-validation by typed errors.
Background
sequencer-validationhas to be made a dependency ofsequencer-client, which only uses typed errors. Thereforesequencer-validationmust have typed errors (because otherwisesequencer-clientwould be "infected" by eyre, which would require changing all of sequencer-client to also use eyre :-/ ).Changes
eyrereports by typed errorsMerkleTree::prove_inclusionto actually return an error: the docs said it would error, but it actually panicked ifindexwas out of bounds.Testing
Added a test to check for out of bounds inclusion proof index. The other error type is just a wrapper.
Related Issues
This is done as part of #332, which now requires extra trait methods in the
SequencerClientExtandSequencerSubscriptionClientExtextension traits.