refactor(consensus): switch ValidatorId to a plain Felt #2137
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. Join @matan-starkware and the rest of your teammates on |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2137 +/- ##
==========================================
- Coverage 65.62% 65.56% -0.07%
==========================================
Files 135 135
Lines 17875 17875
Branches 17875 17875
==========================================
- Hits 11730 11719 -11
- Misses 4859 4869 +10
- Partials 1286 1287 +1 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Reviewed 5 of 5 files at r1, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @matan-starkware)
a discussion (no related file):
Why is this PR needed? What's wrong with contract address?
crates/papyrus_protobuf/src/consensus.rs line 8 at r1 (raw file):
pub struct Proposal { pub height: u64, pub proposer: StarkHash,
Why StarkHash and not felt? Is the proposer representing some hash?
crates/sequencing/papyrus_consensus/src/types.rs line 15 at r1 (raw file):
/// signatures. // TODO(matan): Determine the actual type of NodeId. pub type ValidatorId = StarkHash;
Same here. Why Hash and not Felt?
There was a problem hiding this comment.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @ShahakShama)
a discussion (no related file):
Previously, ShahakShama wrote…
Why is this PR needed? What's wrong with contract address?
Well it's not actually certain that we will be using ContractAddress. Also ContractAddress didn't have Display, which is nice for readable printing. So I decided to just go to a more basic type (ContractAddress is basically just an alias for Felt anyways).
crates/papyrus_protobuf/src/consensus.rs line 8 at r1 (raw file):
Previously, ShahakShama wrote…
Why StarkHash and not felt? Is the proposer representing some hash?
Done.
crates/sequencing/papyrus_consensus/src/types.rs line 15 at r1 (raw file):
Previously, ShahakShama wrote…
Same here. Why Hash and not Felt?
Done.
matan-starkware
force-pushed
the
matan/consensus_validator_id_felt
branch
from
77789e7 to
688b677
Compare
matan-starkware
changed the base branch from
06-19-refactor_consensus_add_test_util_to_more_cleanly_create_validator_ids
to
main
|
Please add @ittaysw (FYI) in proto/specs related PRs |
There was a problem hiding this comment.
Reviewable status: 2 of 5 files reviewed, 3 unresolved discussions (waiting on @ShahakShama)
a discussion (no related file):
Previously, matan-starkware wrote…
Well it's not actually certain that we will be using ContractAddress. Also ContractAddress didn't have Display, which is nice for readable printing. So I decided to just go to a more basic type (ContractAddress is basically just an alias for Felt anyways).
- ContractAddress is just a few layers of abstraction over Felt
- The current thought is to use contract address, but it isn't actually set in stone yet
- ContractAddress didn't have the Display trait and given (2) I didn't want to invest in adding it to an external crate
Dan said the Debug for ContractAddress is being fixed so there is no need to do this. Holding off on this PR for now.
matan-starkware
force-pushed
the
matan/consensus_validator_id_felt
branch
from
688b677 to
394f4c5
Compare
This change is