apollo_propeller: add core unit struct with accessors#11054
Conversation
guy-starkware
left a comment
There was a problem hiding this comment.
@guy-starkware reviewed 1 file and all commit messages, and made 2 comments.
Reviewable status: 1 of 2 files reviewed, 2 unresolved discussions (waiting on @noamsp-starkware, @ShahakShama, and @sirandreww-starkware).
crates/apollo_propeller/src/unit.rs line 67 at r1 (raw file):
self.root } }
Do we really need all these getters? I haven't seen other structs get everything re-routed into getters in other places.
Code quote:
pub fn channel(&self) -> Channel {
self.channel
}
pub fn publisher(&self) -> PeerId {
self.publisher
}
pub fn signature(&self) -> &[u8] {
&self.signature
}
pub fn index(&self) -> ShardIndex {
self.index
}
pub fn shard(&self) -> &[u8] {
&self.shard
}
pub fn shard_mut(&mut self) -> &mut Vec<u8> {
&mut self.shard
}
pub fn proof(&self) -> &MerkleProof {
&self.proof
}
pub fn root(&self) -> MessageRoot {
self.root
}
}crates/apollo_propeller/src/unit.rs line 70 at r1 (raw file):
#[cfg(test)] mod tests {
You should move these to a different file, then you would have a file named unit_test which is apt
guy-starkware
left a comment
There was a problem hiding this comment.
@guy-starkware made 1 comment.
Reviewable status: 1 of 2 files reviewed, 2 unresolved discussions (waiting on @noamsp-starkware, @ShahakShama, and @sirandreww-starkware).
crates/apollo_propeller/src/unit.rs line 70 at r1 (raw file):
Previously, guy-starkware wrote…
You should move these to a different file, then you would have a file named unit_test which is apt
Although the tests seems pretty trivial IMHO, I would be ok removing these altogether. But that's for your consideration.
3069acd to
e6dbf88
Compare
b9190d4 to
194c67b
Compare
sirandreww-starkware
left a comment
There was a problem hiding this comment.
@sirandreww-starkware made 2 comments.
Reviewable status: 1 of 2 files reviewed, 2 unresolved discussions (waiting on @guy-starkware, @noamsp-starkware, and @ShahakShama).
crates/apollo_propeller/src/unit.rs line 67 at r1 (raw file):
Previously, guy-starkware wrote…
Do we really need all these getters? I haven't seen other structs get everything re-routed into getters in other places.
These are used throughout the code to do validation and checking, I could have made these fields public but this way I control the mutability of the object very tightly. I added a TODO to consider making all fields public in the future.
crates/apollo_propeller/src/unit.rs line 70 at r1 (raw file):
Previously, guy-starkware wrote…
Although the tests seems pretty trivial IMHO, I would be ok removing these altogether. But that's for your consideration.
I removed the test
guy-starkware
left a comment
There was a problem hiding this comment.
@guy-starkware reviewed 2 files and all commit messages, and resolved 2 discussions.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @noamsp-starkware and @ShahakShama).
ShahakShama
left a comment
There was a problem hiding this comment.
@ShahakShama reviewed all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @noamsp-starkware).
e6dbf88 to
d526a1d
Compare
194c67b to
48fbc99
Compare
48fbc99 to
f5eb4f8
Compare
d526a1d to
270ea07
Compare
270ea07 to
81bf74b
Compare
1a26016 to
8078cb0
Compare
81bf74b to
2768240
Compare
2768240 to
5e29cdb
Compare
8078cb0 to
8654325
Compare
|
Artifacts upload workflows: |
sirandreww-starkware
left a comment
There was a problem hiding this comment.
@sirandreww-starkware reviewed 6 files and all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @noamsp-starkware).
2653db8

No description provided.