-
Notifications
You must be signed in to change notification settings - Fork 18
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(test): Add blocks proto #812
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## main #812 +/- ##
==========================================
- Coverage 76.50% 76.22% -0.28%
==========================================
Files 169 172 +3
Lines 14358 14487 +129
==========================================
+ Hits 10984 11042 +58
- Misses 3374 3445 +71
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
||
message DeclareV0 { | ||
Address sender = 1; | ||
Signature signature = 3; | ||
Hash class_hash = 4; | ||
} |
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.
We already have a dummy tx type so we can remove this I think, it's a starknet-specific tx type.
/// Transaction | ||
#[derive(Clone, PartialEq, Eq, Ord, PartialOrd)] | ||
pub struct Transaction { | ||
data: Bytes, |
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.
Let's have the data be an enum with the Dummy tx as a variant, so that we can add more interesting tx types later down the road.
|
||
/// Compute the hash of a transaction | ||
/// | ||
/// TODO: Use hash function from Context |
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.
We can remove this todo, it's actually not needed since hashing only happens on the app side and the Context does not have to care about it.
@@ -0,0 +1,53 @@ | |||
use crate::{hash::BlockHash, transaction::Transactions, Height}; |
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.
Let's re-order these imports
|
||
use malachitebft_proto; | ||
|
||
pub type MessageHash = Hash; |
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.
This is not needed I think.
use rand::rngs::StdRng; | ||
use rand::{Rng, SeedableRng}; | ||
use sha3::Digest; | ||
use tracing::{debug, error}; | ||
|
||
use crate::store::{DecidedValue, Store}; |
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.
Let's re-order these imports
// Simplified value creation. In a real application, use the whole hash. | ||
let mut block_hash_short = [0; 8]; | ||
block_hash_short.copy_from_slice(&block.block_hash.as_bytes()[0..8]); | ||
let value = Value::new(u64::from_be_bytes(block_hash_short)); |
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.
Since this code appears twice, let's perhaps pull it out in a free-standing function?
// Simplified value creation. In a real application, use the whole hash. | ||
let mut block_hash_short = [0; 8]; | ||
block_hash_short.copy_from_slice(&block.block_hash.as_bytes()[0..8]); | ||
let value = Value::new(u64::from_be_bytes(block_hash_short)); |
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.
Let's add a TODO to change Value
to hold a byte array in a follow-up PR.
@@ -223,7 +226,11 @@ impl State { | |||
assert_eq!(round, self.current_round); | |||
|
|||
// We create a new value. | |||
let value = self.make_value(); | |||
let block = self.fetch_block(height); |
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.
since the block doesn't exist:
let block = self.fetch_block(height); | |
let block = self.make_block(height); |
also the block should be saved in some UNDECIDED_BLOCK table. And we should stream here the block and not the proposal (as it's done now here). The ProposalPart
in context is also wrong now. And reusing the test protos (ProposalData
should be something like Transactions transactions ..
or bytes) doesn't work.
Closes: #XXX
This PR expands the test crate with the Block proto struct so example apps and tests can handle block transfers over the wire.
PR author checklist
For all contributors
For external contributors