apollo_propeller: add tree generation error type#11050
Conversation
guy-starkware
left a comment
There was a problem hiding this comment.
@guy-starkware reviewed 1 file.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @noamsp-starkware, @ShahakShama, and @sirandreww-starkware).
ShahakShama
left a comment
There was a problem hiding this comment.
@ShahakShama reviewed all commit messages and made 2 comments.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @noamsp-starkware and @sirandreww-starkware).
a discussion (no related file):
It's hard for me to review these errors without seeing them used. I think that you should start with no variants and add each variant in the PR where you use it for the first time
crates/apollo_propeller/src/types.rs line 74 at r1 (raw file):
impl std::error::Error for ShardSignatureVerificationError {} // ****************************************************************************
remove this
sirandreww-starkware
left a comment
There was a problem hiding this comment.
@sirandreww-starkware made 3 comments.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @guy-starkware, @noamsp-starkware, and @ShahakShama).
a discussion (no related file):
Previously, ShahakShama wrote…
It's hard for me to review these errors without seeing them used. I think that you should start with no variants and add each variant in the PR where you use it for the first time
Please see previous PR
crates/apollo_propeller/src/types.rs line 74 at r1 (raw file):
Previously, ShahakShama wrote…
remove this
see previous PR
crates/apollo_propeller/src/types.rs line 106 at r1 (raw file):
Previously, guy-starkware wrote…
same comment as previous PR
see previous PR
5540d79 to
f8c70d8
Compare
30c3c0a to
60f511a
Compare
guy-starkware
left a comment
There was a problem hiding this comment.
@guy-starkware reviewed 1 file and all commit messages, and resolved 1 discussion.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @noamsp-starkware and @ShahakShama).
ShahakShama
left a comment
There was a problem hiding this comment.
@ShahakShama reviewed all commit messages, made 6 comments, and resolved 1 discussion.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @noamsp-starkware and @sirandreww-starkware).
crates/apollo_propeller/src/types.rs line 79 at r2 (raw file):
#[derive(Debug, Clone, PartialEq, Eq)] pub enum TreeGenerationError {
When is this error used? Where do you generate trees in propeller? Maybe we can find a better name than TreeGeneration
crates/apollo_propeller/src/types.rs line 80 at r2 (raw file):
#[derive(Debug, Clone, PartialEq, Eq)] pub enum TreeGenerationError { PublisherNotFound {
Rename to PublisherNotInChannel
crates/apollo_propeller/src/types.rs line 84 at r2 (raw file):
publisher: PeerId, }, LocalPeerNotInPeerWeights,
Same here. LocalPeerNotInChannel
crates/apollo_propeller/src/types.rs line 84 at r2 (raw file):
publisher: PeerId, }, LocalPeerNotInPeerWeights,
It's not clear what local peer means
crates/apollo_propeller/src/types.rs line 91 at r2 (raw file):
} impl std::fmt::Display for TreeGenerationError {
Remove this and use thiserror instead. You can just have a TODO for now
crates/apollo_propeller/src/types.rs line 110 at r2 (raw file):
} impl std::error::Error for TreeGenerationError {}
Same here
ShahakShama
left a comment
There was a problem hiding this comment.
@ShahakShama resolved 1 discussion.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @noamsp-starkware and @sirandreww-starkware).
f8c70d8 to
78e61e7
Compare
60f511a to
1f407b8
Compare
78e61e7 to
6a3e6f6
Compare
1f407b8 to
73b7be3
Compare
sirandreww-starkware
left a comment
There was a problem hiding this comment.
@sirandreww-starkware made 6 comments.
Reviewable status: 0 of 1 files reviewed, 6 unresolved discussions (waiting on @guy-starkware, @noamsp-starkware, and @ShahakShama).
crates/apollo_propeller/src/types.rs line 79 at r2 (raw file):
Previously, ShahakShama wrote…
When is this error used? Where do you generate trees in propeller? Maybe we can find a better name than TreeGeneration
so, TreeGenerationError is the error you get when trying to gauge the topology of the channel. Actions that may return this error are:
- you receive shard x and you want to know who is the peer that should send you this shard
- you want to see what shard you are responsible for
crates/apollo_propeller/src/types.rs line 80 at r2 (raw file):
Previously, ShahakShama wrote…
Rename to PublisherNotInChannel
Done.
crates/apollo_propeller/src/types.rs line 84 at r2 (raw file):
Previously, ShahakShama wrote…
It's not clear what local peer means
What would be a better name for "me"?
crates/apollo_propeller/src/types.rs line 84 at r2 (raw file):
Previously, ShahakShama wrote…
Same here. LocalPeerNotInChannel
Done.
crates/apollo_propeller/src/types.rs line 91 at r2 (raw file):
Previously, ShahakShama wrote…
Remove this and use thiserror instead. You can just have a TODO for now
not sure what this is referring to or if it is outdated. Please resolve if it is outdated.
crates/apollo_propeller/src/types.rs line 110 at r2 (raw file):
Previously, ShahakShama wrote…
Same here
not sure what this is referring to or if it is outdated. Please resolve if it is outdated.
ShahakShama
left a comment
There was a problem hiding this comment.
@ShahakShama reviewed 1 file and all commit messages, and resolved 6 discussions.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @noamsp-starkware).
73b7be3 to
d18e3df
Compare
ShahakShama
left a comment
There was a problem hiding this comment.
@ShahakShama reviewed 1 file and all commit messages, and made 1 comment.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @noamsp-starkware).
6a3e6f6 to
1523328
Compare
d18e3df to
0dc41cb
Compare
0dc41cb to
a1be91d
Compare
Merge activity
|

No description provided.