-
Notifications
You must be signed in to change notification settings - Fork 784
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
Redb slasher backend impl #4529
Conversation
so I got all the slasher test cases to pass for the redb impl EXCEPT The code I wrote probably has a lot of room for improvement. I'm still sort of new to Rust and am having lots of fun with the borrow checker :). In general theres a few things I wanted to mention:
If you get the chance, I could use a review of my changes up to this point. It would be great to get some general feedback so that I can understand if my implementation is at least trending in the right direction |
ready for a review |
slasher/src/database/redb_impl.rs
Outdated
|
||
impl Environment { | ||
pub fn new(config: &Config) -> Result<Environment, Error> { | ||
let db_path = match config.database_path.join(BASE_DB).as_path().to_str() { |
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.
do we definitely need to join the BASE_DB
here? I think the caller takes care of that already when constructing the config.database_path
…db-slasher-backend-impl
I just pushed a minor optimisation that avoids collecting into an intermediate Vec: 6b0a81f |
Also just back-merged unstable so the version reads 5.2 |
…ev/lighthouse into redb-slasher-backend-impl
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.
Just pushed one change to tidy up the DB filename, which we would have trouble changing later.
@Mergifyio queue |
✅ The pull request has been merged automaticallyThe pull request has been merged automatically at 70bcba1 |
Issue Addressed
#4424
Proposed Changes
add redb implementation to the slasher backend
Additional Info