-
Notifications
You must be signed in to change notification settings - Fork 21
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
feat(batcher, mempool_node): add local storage to the batcher #982
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.
Reviewed 9 of 9 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @dafnamatsry, @dan-starkware, @lev-starkware, and @yair-starkware)
config/mempool/default_config.json
line 1 at r1 (raw file):
{
why is this file called mempool/default_config?
should it be sequencer/default_config?
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.
Reviewed 9 of 9 files at r1, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @dan-starkware, @lev-starkware, and @yair-starkware)
config/mempool/default_config.json
line 52 at r1 (raw file):
"value": "StateOnly" }, "chain_id": {
Shouldn't we use the same chain ID already configured in the storage.db_config
?
crates/batcher/src/batcher.rs
line 22 at r1 (raw file):
config: BatcherConfig, mempool_client: SharedMempoolClient, local_storage: Arc<dyn BatcherStorageTrait>,
Suggestion:
storage
crates/batcher/src/batcher.rs
line 38 at r1 (raw file):
#[cfg_attr(test, automock)] pub trait BatcherStorageTrait: Send + Sync {}
Is it intended to be only a reader? If so, please rename to StorageReaderTrait
Code quote:
BatcherStorageTrait
crates/mempool_node/src/config/mod.rs
line 41 at r1 (raw file):
pub static CONFIG_POINTERS: LazyLock<ConfigPointers> = LazyLock::new(|| { vec![( ser_pointer_target_param("chain_id", &ChainId::Mainnet, "The chain to follow."),
Is it possible to somehow put pointer value (ChainId::Mainnet
) in the config json file?
It is a little confusing that some values are defined there and some here...
639ef1f
to
7ff3ff1
Compare
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.
Reviewable status: 8 of 9 files reviewed, 5 unresolved discussions (waiting on @dafnamatsry, @dan-starkware, @lev-starkware, and @Yael-Starkware)
config/mempool/default_config.json
line 1 at r1 (raw file):
Previously, Yael-Starkware (YaelD) wrote…
why is this file called mempool/default_config?
should it be sequencer/default_config?
This is not related to this PR...
config/mempool/default_config.json
line 52 at r1 (raw file):
Previously, dafnamatsry wrote…
Shouldn't we use the same chain ID already configured in the
storage.db_config
?
It does - I made the storage.db_config.chain_id
point to this param so other components that need the chain ID will share the same value.
crates/batcher/src/batcher.rs
line 22 at r1 (raw file):
config: BatcherConfig, mempool_client: SharedMempoolClient, local_storage: Arc<dyn BatcherStorageTrait>,
Done.
crates/batcher/src/batcher.rs
line 38 at r1 (raw file):
Previously, dafnamatsry wrote…
Is it intended to be only a reader? If so, please rename to
StorageReaderTrait
Done.
crates/mempool_node/src/config/mod.rs
line 41 at r1 (raw file):
Previously, dafnamatsry wrote…
Is it possible to somehow put pointer value (
ChainId::Mainnet
) in the config json file?
It is a little confusing that some values are defined there and some here...
The json file is generated automatically from the ComponentConfig
struct and this pointer mapping.
You can see in the json that there is already a chain_id
param with the default value and that the storage.db_config.chain_id
points to that value.
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.
Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @dan-starkware, @lev-starkware, and @Yael-Starkware)
crates/mempool_node/src/config/mod.rs
line 41 at r1 (raw file):
Previously, yair-starkware (Yair) wrote…
The json file is generated automatically from the
ComponentConfig
struct and this pointer mapping.
You can see in the json that there is already achain_id
param with the default value and that thestorage.db_config.chain_id
points to that value.
Ok, I misunderstood how it works. Thanks.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #982 +/- ##
==========================================
- Coverage 74.18% 7.72% -66.47%
==========================================
Files 359 168 -191
Lines 36240 16551 -19689
Branches 36240 16551 -19689
==========================================
- Hits 26886 1279 -25607
- Misses 7220 15225 +8005
+ Partials 2134 47 -2087
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
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.
Reviewed 8 of 9 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @dan-starkware and @Yael-Starkware)
This change is