-
Notifications
You must be signed in to change notification settings - Fork 344
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
Improve CanonicalAddr interoperability #1463
Conversation
1fe9a37
to
e6033ae
Compare
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.
The code looks fine. Is it okay for any arbitrary binary string to be infallibly convertible to CanonicalAddress
? The "canonical" sounds like it's supposed to offer some guarantees.
// Array reference | ||
impl<const LENGTH: usize> From<&[u8; LENGTH]> for CanonicalAddr { | ||
fn from(source: &[u8; LENGTH]) -> Self { | ||
Self(source.into()) | ||
} | ||
} |
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.
What about a slice?
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.
You mean impl From<&[u8]> for CanonicalAddr
? This was already implemented before a few lines above this addition.
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.
Ah, my bad!
Excellent question. Historically this has always been an unsafe newtype around Binary (with a public field), so at least this is not introducing any new unsafety. |
That's probably fine. I think Rust's |
Preparation for #1456 and #1451