-
Notifications
You must be signed in to change notification settings - Fork 92
feat(supervisor/storage): chaindb and chaindb factory #1864
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR introduces new functionality for managing chain-specific databases by adding a Chaindb implementation and a corresponding factory, as well as exposing a derivation storage trait for supervisor storage.
- Added the DerivationStorage trait
- Introduced new modules: ChainDb and ChainDbFactory
- Updated error handling to include a new database initialization error
Reviewed Changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.
Show a summary per file
File | Description |
---|---|
crates/supervisor/storage/src/traits.rs | Added the derivation storage interface |
crates/supervisor/storage/src/providers/mod.rs | Cleaned up unused imports |
crates/supervisor/storage/src/providers/derivation_provider.rs | Removed dead_code attributes from functions |
crates/supervisor/storage/src/lib.rs | Exposed new ChainDb and ChainDbFactory modules |
crates/supervisor/storage/src/error.rs | Added a DatabaseInit error variant |
crates/supervisor/storage/src/chaindb_factory.rs | Introduced chain database factory logic |
crates/supervisor/storage/src/chaindb.rs | Implemented ChainDb and integrated DerivationStorage |
crates/supervisor/storage/Cargo.toml | Added dependency configuration for eyre |
Cargo.toml | Added eyre dependency |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR introduces ChainDb and ChainDbFactory implementations, along with a new DerivationStorage trait to integrate database operations for block derivation. It also adjusts provider annotations, updates error handling with a new DatabaseInit variant, and includes related dependency changes in Cargo.toml.
- Added the DerivationStorage trait and its implementation for ChainDb.
- Introduced ChainDbFactory for managing per-chain databases.
- Updated Cargo.toml files to include the eyre dependency for improved error reporting.
Reviewed Changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
crates/supervisor/storage/src/traits.rs | Added a new trait defining methods for block derivation storage. |
crates/supervisor/storage/src/providers/mod.rs | Removed unused import annotations for provider usage. |
crates/supervisor/storage/src/providers/derivation_provider.rs | Removed dead-code attribute from provider methods. |
crates/supervisor/storage/src/lib.rs | Re-exported modules and traits to integrate new functionalities. |
crates/supervisor/storage/src/error.rs | Added a new error variant (DatabaseInit) for initialization errors. |
crates/supervisor/storage/src/chaindb_factory.rs | Introduced a factory to manage per-chain ChainDb instances with tests. |
crates/supervisor/storage/src/chaindb.rs | Implemented ChainDb with database initialization and trait support. |
crates/supervisor/storage/Cargo.toml | Added the eyre dependency to the workspace. |
Cargo.toml | Added the eyre dependency for error handling in the main project. |
} | ||
|
||
// Not found, create and insert | ||
let mut dbs = self.dbs.write().unwrap(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider handling potential lock poisoning on the write lock instead of using unwrap(), to ensure graceful error recovery in concurrent scenarios.
Copilot uses AI. Check for mistakes.
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found.
☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
/// Finds a [`BlockInfo`] by a specific [`Log`]. | ||
/// | ||
/// # Arguments | ||
/// * `block_number` - The block number to search for the log. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we need to pass block number as param, wouldn't it be better to simply get block info by number? Or am I misunderstanding something here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method is more like searching the block by logs information. This will return the block only if it contains the provided log.
I have added a todo on the method. I think we need to structure it better for the understanding.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have updated the doc as well to explain this.
Closes #1843
Ref #1684