-
Notifications
You must be signed in to change notification settings - Fork 35
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
chore(consensus): add PapyrusConsensusConfig struct #3377
chore(consensus): add PapyrusConsensusConfig struct #3377
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
f1cfc9e
to
2c57fef
Compare
dc0dd26
to
14909c5
Compare
2c57fef
to
298f522
Compare
28f6ed1
to
296594f
Compare
298f522
to
3f08f67
Compare
296594f
to
8d996ae
Compare
3f08f67
to
764afbb
Compare
8d996ae
to
9ac69ae
Compare
764afbb
to
1760a5b
Compare
9ac69ae
to
e864fff
Compare
1760a5b
to
7fa81cf
Compare
e864fff
to
59490b8
Compare
7fa81cf
to
fed4b22
Compare
59490b8
to
1aca5ea
Compare
fed4b22
to
b7e9378
Compare
1aca5ea
to
9902c7b
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.
splitting the consensus config struct is a good step. let's also focus on naming. clear and concise names for each struct and its params will make the code much easier to understand:
ContextConfig: chain_id + num_validators 👍
how did u split the rest?
Reviewed 2 of 4 files at r1, 2 of 2 files at r2, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @guy-starkware and @matan-starkware)
crates/papyrus_node/src/config/mod.rs
line 142 at r2 (raw file):
#[derive(Debug, Clone, Deserialize, Serialize, PartialEq, Validate)] pub struct PapyrusConsensusConfig {
are u planning to update the ConsensusManagerConfig to use this new struct? or did u forget?
crates/papyrus_node/src/run.rs
line 209 at r2 (raw file):
debug!("Context configuration: {context_config:?}"); let papyrus_consensus_config = papyrus_consensus_config.clone(); debug!("Papyrus consensus configuration: {papyrus_consensus_config:?}");
Suggestion:
let consensus_config = consensus_config.clone();
let context_config = context_config.clone();
let papyrus_consensus_config = papyrus_consensus_config.clone();
debug!("Consensus configuration: {consensus_config:?}");
debug!("Context configuration: {context_config:?}");
debug!("Papyrus consensus configuration: {papyrus_consensus_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.
I'm not sure what you mean. I generally followed the blueprint Matan sent me for what fields should be in each config.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @asmaastarkware and @matan-starkware)
crates/papyrus_node/src/config/mod.rs
line 142 at r2 (raw file):
Previously, asmaastarkware (asmaa-starkware) wrote…
are u planning to update the ConsensusManagerConfig to use this new struct? or did u forget?
If I understand Matan's plan correctly, the idea is that ConsensusManagerConfig is used for the manager in starknet_sequencer while PapyrusConsensusConfig is used only in papyrus_node.
crates/papyrus_node/src/run.rs
line 209 at r2 (raw file):
debug!("Context configuration: {context_config:?}"); let papyrus_consensus_config = papyrus_consensus_config.clone(); debug!("Papyrus consensus configuration: {papyrus_consensus_config:?}");
Done.
b7e9378
to
3454b9e
Compare
9902c7b
to
f4c4d38
Compare
3454b9e
to
06d06b3
Compare
f4c4d38
to
c533198
Compare
f449128
to
279df6d
Compare
This creates a new config struct called PapyrusConsensusConfig, which has some fields necessary for running a papyrus node.
Some of it similar to the content of ConsensusManagerConfig, but relevant for the papyrus node instead of the sequencer node.