fix: use datastore-level by default #8503
Merged
Conversation
This file contains hidden or 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
Contributor
Summary of ChangesHello @wemeetagain, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request resolves a regression by restoring Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
Contributor
There was a problem hiding this comment.
Code Review
This pull request addresses a performance regression by switching the default datastore implementation from datastore-fs to datastore-level, while retaining datastore-fs for the Bun environment. This is achieved through conditional exports in package.json, which is a clean approach. The changes in datastore.ts are consistent with this new abstraction.
My review includes a suggestion to improve the clarity of the new Bun-specific wrapper file. More importantly, I've identified that the unit tests for Eth2PeerDataStore have not been updated to reflect the change in the underlying datastore dependency. This is a high-priority issue that should be addressed to ensure the correctness and reliability of the code.
| */ | ||
| export class Eth2PeerDataStore extends BaseDatastore { | ||
| private _dbDatastore: FsDatastore; | ||
| private _dbDatastore: LevelDatastore; |
Contributor
There was a problem hiding this comment.
The unit tests for Eth2PeerDataStore in datastore.test.ts appear to be outdated after this change. They still mock datastore-fs and use FsDatastore, but the code under test now uses #datastore-wrapper, which resolves to datastore-level in the test environment (Node.js). The tests should be updated to mock #datastore-wrapper and use the correct LevelDatastore type to ensure the component is tested correctly with its new dependency.
Comment on lines
+1
to
+3
| import {FsDatastore} from "datastore-fs"; | ||
|
|
||
| export const LevelDatastore = FsDatastore; |
Contributor
There was a problem hiding this comment.
For better maintainability and to avoid confusion, it would be helpful to add a file-level comment explaining why FsDatastore is being re-exported as LevelDatastore. This file acts as a compatibility layer for the Bun environment, and a comment would clarify its purpose.
Suggested change
| import {FsDatastore} from "datastore-fs"; | |
| export const LevelDatastore = FsDatastore; | |
| /** | |
| * In the Bun environment, `datastore-fs` is used for performance reasons. | |
| * This module re-exports `FsDatastore` as `LevelDatastore` to provide a | |
| * consistent interface for `Eth2PeerDataStore`, which expects `LevelDatastore`. | |
| * The default Node.js environment uses the actual `datastore-level`. | |
| */ | |
| import {FsDatastore} from "datastore-fs"; | |
| export const LevelDatastore = FsDatastore; | |
Contributor
Performance Report✔️ no performance regression detected Full benchmark results
|
nflaig
approved these changes
Oct 6, 2025
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## unstable #8503 +/- ##
============================================
- Coverage 52.24% 52.23% -0.02%
============================================
Files 852 852
Lines 65004 65004
Branches 4774 4771 -3
============================================
- Hits 33962 33955 -7
- Misses 30973 30980 +7
Partials 69 69 🚀 New features to boost your workflow:
|
Member
Author
|
🎉 This PR is included in v1.35.0 🎉 |
AbolareRoheemah
pushed a commit
to AbolareRoheemah/lodestar
that referenced
this pull request
Oct 14, 2025
**Motivation** - Review of ChainSafe#8468 metrics - In ChainSafe#8449, use of `datastore-level` was unilaterally removed in favor of the bun-supported `datastore-fs` - This caused a regression **Description** - use `datastore-level` by default, only use `datastore-fs` in bun --------- Co-authored-by: Nico Flaig <nflaig@protonmail.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.
Motivation
datastore-levelwas unilaterally removed in favor of the bun-supporteddatastore-fsDescription
datastore-levelby default, only usedatastore-fsin bun