apollo_propeller: add ShardSignatureVerificationError#11049
Conversation
guy-starkware
left a comment
There was a problem hiding this comment.
@guy-starkware reviewed 3 files and all commit messages, and made 1 comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @noamsp-starkware, @ShahakShama, and @sirandreww-starkware).
crates/apollo_propeller/src/types.rs line 70 at r2 (raw file):
} } }
I think the way we usually do it is implement Error and then add annotations above each variant.
Code quote:
impl std::fmt::Display for ShardSignatureVerificationError {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
match self {
ShardSignatureVerificationError::NoPublicKeyAvailable(publisher) => {
write!(f, "No public key available for signer {}", publisher)
}
ShardSignatureVerificationError::EmptySignature => {
write!(f, "Shard has empty signature")
}
ShardSignatureVerificationError::VerificationFailed => {
write!(f, "Shard signature is invalid")
}
}
}
}
guy-starkware
left a comment
There was a problem hiding this comment.
@guy-starkware made 1 comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @noamsp-starkware, @ShahakShama, and @sirandreww-starkware).
crates/apollo_propeller/src/types.rs line 70 at r2 (raw file):
Previously, guy-starkware wrote…
I think the way we usually do it is implement Error and then add annotations above each variant.
I mean thiserror
ShahakShama
left a comment
There was a problem hiding this comment.
@ShahakShama reviewed 3 files and all commit messages, and made 3 comments.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @guy-starkware, @noamsp-starkware, and @sirandreww-starkware).
crates/apollo_propeller/src/types.rs line 70 at r2 (raw file):
Previously, guy-starkware wrote…
I mean thiserror
+1
crates/apollo_propeller/src/types.rs line 46 at r2 (raw file):
} // ****************************************************************************
Remove this
crates/apollo_propeller/src/types.rs line 51 at r2 (raw file):
#[derive(Debug, Clone, PartialEq, Eq)] pub enum ShardSignatureVerificationError { NoPublicKeyAvailable(PeerId),
It's unclear whose peer id is this. Make this a named fields variant
ShahakShama
left a comment
There was a problem hiding this comment.
@ShahakShama made 1 comment.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @guy-starkware, @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
5540d79 to
f8c70d8
Compare
043800c to
2193ed5
Compare
sirandreww-starkware
left a comment
There was a problem hiding this comment.
@sirandreww-starkware made 4 comments.
Reviewable status: all files reviewed, 4 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
I tried building PRs this way and it was really hard to keep them small. Also at this point this requires a huge change in the PR order/structure. If you give me enough time we can do this but we definitely will not hit our February deadline.
crates/apollo_propeller/src/types.rs line 46 at r2 (raw file):
Previously, ShahakShama wrote…
Remove this
please review response in last PR
crates/apollo_propeller/src/types.rs line 51 at r2 (raw file):
Previously, ShahakShama wrote…
It's unclear whose peer id is this. Make this a named fields variant
I think it is very clear that this error is saying "I do not know the public key of this PeerID"
crates/apollo_propeller/src/types.rs line 70 at r2 (raw file):
Previously, ShahakShama wrote…
+1
I'll add a TODO to do this after this file contains all the error types instead of updating all the PRs that add errors
guy-starkware
left a comment
There was a problem hiding this comment.
@guy-starkware reviewed 3 files and all commit messages, made 1 comment, and resolved 1 discussion.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @noamsp-starkware and @ShahakShama).
crates/apollo_propeller/src/types.rs line 51 at r2 (raw file):
Previously, sirandreww-starkware (Andrew Luka) wrote…
I think it is very clear that this error is saying "I do not know the public key of this PeerID"
I think this is the sort of thing you can get "thiserror" to make explicit. When you reach that TODO above and add thiserror, make sure to spell out the errors so the poor debuggers looking at our logs will have an easy time.
ShahakShama
left a comment
There was a problem hiding this comment.
@ShahakShama reviewed all commit messages, made 2 comments, and resolved 2 discussions.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @noamsp-starkware and @sirandreww-starkware).
crates/apollo_propeller/src/types.rs line 46 at r2 (raw file):
Previously, sirandreww-starkware (Andrew Luka) wrote…
please review response in last PR
I still think we should remove this
crates/apollo_propeller/src/types.rs line 51 at r2 (raw file):
Previously, guy-starkware wrote…
I think this is the sort of thing you can get "thiserror" to make explicit. When you reach that TODO above and add thiserror, make sure to spell out the errors so the poor debuggers looking at our logs will have an easy time.
Fair point @guy-starkware
2193ed5 to
7a08a3c
Compare
f8c70d8 to
78e61e7
Compare
sirandreww-starkware
left a comment
There was a problem hiding this comment.
@sirandreww-starkware made 2 comments.
Reviewable status: 0 of 3 files reviewed, 1 unresolved discussion (waiting on @guy-starkware, @noamsp-starkware, and @ShahakShama).
crates/apollo_propeller/src/types.rs line 46 at r2 (raw file):
Previously, ShahakShama wrote…
I still think we should remove this
Done.
crates/apollo_propeller/src/types.rs line 51 at r2 (raw file):
Previously, ShahakShama wrote…
Fair point @guy-starkware
Done
I decided to bite the bullet and do it now that I have your attention
ShahakShama
left a comment
There was a problem hiding this comment.
@ShahakShama reviewed 3 files and all commit messages, made 1 comment, and resolved 1 discussion.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @noamsp-starkware).
ShahakShama
left a comment
There was a problem hiding this comment.
@ShahakShama made 1 comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @noamsp-starkware and @sirandreww-starkware).
crates/apollo_propeller/src/types.rs line 24 at r4 (raw file):
needs to be provided explicitly
Is that an option? If not, erase that part of the sentence
7a08a3c to
8bed384
Compare
78e61e7 to
6a3e6f6
Compare
8bed384 to
208ae26
Compare
6a3e6f6 to
1523328
Compare
Merge activity
|
sirandreww-starkware
left a comment
There was a problem hiding this comment.
@sirandreww-starkware reviewed 3 files and all commit messages, and resolved 1 discussion.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @noamsp-starkware).
d6128a3

No description provided.