Remove code that can panic #5
Conversation
kettlebell
changed the title
unwrap calls in [In|Out]boundUpgrade
kettlebell
force-pushed
the
dev-397-get-rid-of-panicking-code
branch
from
aa061cf to
82a7374
Compare
kettlebell
force-pushed
the
dev-397-get-rid-of-panicking-code
branch
2 times, most recently
from
93bb2ca to
3d89acf
Compare
kettlebell
force-pushed
the
dev-397-get-rid-of-panicking-code
branch
from
3d89acf to
45e1a15
Compare
There was a problem hiding this comment.
The only way to get rid of panicking code is to make illegal states unrepresentable on type-level.
P.s. here is a good article on that topic https://lexi-lambda.github.io/blog/2019/11/05/parse-don-t-validate/
| @@ -33,6 +33,8 @@ pub enum ProtocolHandshakeErr { | |||
| pub enum ProtocolUpgradeErr { | |||
| #[error(transparent)] | |||
| HandshakeErr(#[from] ProtocolHandshakeErr), | |||
| #[error("Unsupported {0:?}")] | |||
| UnsupportedProtocolVer(ProtocolVer), | |||
There was a problem hiding this comment.
Here the case when the negotiated tag is not found in Upgrade is also not possible unless some crazy state inconsistency took place. So no point in adding this exception variant, it can't be handled properly anyway.
There was a problem hiding this comment.
I looked into it in more detail; I see what you mean. Thanks for the Parse, don't validate article, learnt a lot. Do you think it's worth wrapping up these collections in a newtype which prevents any mutation of the underlying collection, and also using a newtype-wrapped protocol id/ver?
e.g. in ProtocolUpgradeIn::supported_versions, we wrap the BTreeMap in a struct SupportedProtocolVers, and instead of handling ProtocolVer directly we have struct SupportedProtocolVer(ProtocolVer) which can only be generated by SupportedProtocolVers?
If that's too much, we can comment the use of the unwrap and explicitly allow it via #[allow(clippy::unwrap_used]. Later when we setup CI/CD, we can add clippy exclusions like e.g. https://github.com/ergoplatform/sigma-rust/blob/develop/ergotree-ir/src/lib.rs#L13-L19
There was a problem hiding this comment.
Yes, wrappers that hold some assumption should be good.
There was a problem hiding this comment.
Implemented the wrappers.
kettlebell
force-pushed
the
dev-397-get-rid-of-panicking-code
branch
from
8c9ee32 to
4643ffb
Compare
| @@ -117,7 +117,13 @@ impl ProtocolTag { | |||
|
|
|||
| impl From<ProtocolTag> for ProtocolVer { | |||
| fn from(p: ProtocolTag) -> Self { | |||
| ProtocolVer::from(p.0[1]) | |||
There was a problem hiding this comment.
Fixed bug with index.
| return Poll::Ready(Some(Err( | ||
| ProtocolHandlerError::Serialization(e), | ||
| ))); | ||
| None |
There was a problem hiding this comment.
Should we do anything different if encoding fails here?
kettlebell
force-pushed
the
dev-397-get-rid-of-panicking-code
branch
from
6ad53a0 to
8f3e035
Compare
No description provided.