feat(papyrus_protobuf): add BlockInfo to proto #3878
Conversation
|
This change is |
This stack of pull requests is managed by Graphite. Learn more about stacking. |
There was a problem hiding this comment.
Reviewed 2 of 3 files at r1, all commit messages.
Reviewable status: 2 of 3 files reviewed, 2 unresolved discussions (waiting on @asmaastarkware and @matan-starkware)
crates/papyrus_protobuf/src/proto/p2p/proto/consensus/consensus.proto line 53 at r1 (raw file):
message BlockInfo { uint64 height = 1;
I think in the previous PR you called this block_number. Whichever you choose, should be consistent.
crates/papyrus_protobuf/src/converters/consensus.rs line 222 at r1 (raw file):
let block_timestamp = value.timestamp; let sequencer_address = value .sequencer
Isn't this supposed to be sequencer_address?
There was a problem hiding this comment.
Reviewable status: 2 of 3 files reviewed, 2 unresolved discussions (waiting on @guy-starkware and @matan-starkware)
crates/papyrus_protobuf/src/converters/consensus.rs line 222 at r1 (raw file):
Previously, guy-starkware wrote…
Isn't this supposed to be sequencer_address?
like in other places (for example proposer at ProposalInit)..
crates/papyrus_protobuf/src/proto/p2p/proto/consensus/consensus.proto line 53 at r1 (raw file):
Previously, guy-starkware wrote…
I think in the previous PR you called this block_number. Whichever you choose, should be consistent.
I will answer here:
good question!
Height is a consensus term, while block_number is used by the batcher.
for now this BlockInfo struct is specific to consensus and won't be passed to the batcher, it makes sense to use height.
- we should talk with the batcher with starknet_api::block::BlockInfo which will count on this new stuct for now..
WDYT? + @matan-starkware WDYT?
There was a problem hiding this comment.
Reviewable status: 2 of 3 files reviewed, 2 unresolved discussions (waiting on @asmaastarkware and @matan-starkware)
crates/papyrus_protobuf/src/converters/consensus.rs line 222 at r1 (raw file):
Previously, asmaastarkware (asmaa-starkware) wrote…
like in other places (for example proposer at ProposalInit)..
I don't mind that you call it sequencer, but I think it should be consistent (there are other places where you use sequencer_address).
I actually prefer sequencer address but I don't think it is super important either way.
crates/papyrus_protobuf/src/proto/p2p/proto/consensus/consensus.proto line 53 at r1 (raw file):
Previously, asmaastarkware (asmaa-starkware) wrote…
I will answer here:
good question!
Height is a consensus term, while block_number is used by the batcher.
for now this BlockInfo struct is specific to consensus and won't be passed to the batcher, it makes sense to use height.
- we should talk with the batcher with starknet_api::block::BlockInfo which will count on this new stuct for now..
WDYT? + @matan-starkware WDYT?
I see. This is indeed very confusing.
There was a problem hiding this comment.
Reviewed 3 of 3 files at r1, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @asmaastarkware and @guy-starkware)
crates/papyrus_protobuf/src/converters/consensus.rs line 222 at r1 (raw file):
Previously, guy-starkware wrote…
I don't mind that you call it sequencer, but I think it should be consistent (there are other places where you use sequencer_address).
I actually prefer sequencer address but I don't think it is super important either way.
My feeling is that we have the type ContractAddress so we don't need to also name the type address, but I don't feel strongly.
crates/papyrus_protobuf/src/proto/p2p/proto/consensus/consensus.proto line 53 at r1 (raw file):
Previously, guy-starkware wrote…
I see. This is indeed very confusing.
I prefer that we use the term height for consensus. This struct (proto and rust) is only used by consensus. The Batcher will define the struct they want to receive and the context there can do block_number: height.
crates/papyrus_protobuf/src/converters/consensus.rs line 223 at r1 (raw file):
let sequencer_address = value .sequencer .ok_or(ProtobufConversionError::MissingField { field_description: "sequencer" })?
This repeats so much.
Can you make another PR where we use a wrapper fn, then the lines become smaller:
fn missing(field_description: &str) -> ProtobufConversionError {
ProtobufConversionError::MissingField { field_description: "voter" }
}
Suggestion:
.ok_or(missing("sequencer"))?crates/papyrus_protobuf/src/consensus.rs line 62 at r1 (raw file):
#[derive(Clone, Debug, PartialEq)] pub struct BlockInfo { pub block_number: BlockNumber,
Suggestion:
pub height: BlockNumber,crates/papyrus_protobuf/src/consensus.rs line 63 at r1 (raw file):
pub struct BlockInfo { pub block_number: BlockNumber, pub block_timestamp: u64,
Suggestion:
pub timestamp: u64,crates/papyrus_protobuf/src/consensus.rs line 64 at r1 (raw file):
pub block_number: BlockNumber, pub block_timestamp: u64, pub sequencer_address: ContractAddress,
Suggestion:
pub builder: ContractAddress,
There was a problem hiding this comment.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @guy-starkware and @matan-starkware)
crates/papyrus_protobuf/src/converters/consensus.rs line 223 at r1 (raw file):
Previously, matan-starkware wrote…
This repeats so much.
Can you make another PR where we use a wrapper fn, then the lines become smaller:
fn missing(field_description: &str) -> ProtobufConversionError { ProtobufConversionError::MissingField { field_description: "voter" } }
ok 👍
asmaastarkware
force-pushed
the
asmaa/block_info/add_to_proto
branch
from
e5ee6c5 to
ad455a3
Compare
There was a problem hiding this comment.
Reviewed 3 of 3 files at r2, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @guy-starkware)
There was a problem hiding this comment.
Reviewed all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @asmaastarkware)
There was a problem hiding this comment.
Reviewed 3 of 3 files at r2, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @asmaastarkware)
There was a problem hiding this comment.
Reviewable status:
complete! all files reviewed, all discussions resolved (waiting on @asmaastarkware)
No description provided.