Skip to content
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

Remove some Identity conversions that could panic #1966

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

jsdt
Copy link
Contributor

@jsdt jsdt commented Nov 8, 2024

Description of Changes

This removes Identity::{from_slice,from_be_slice}, which previously had the potential to panic. Since callers can just use try_into, I don't think we need this in the API.

API and ABI breaking changes

This removes two functions from the Identity lib. If we find that these are widely used, we could add back a safe version. Making this safe will always be a breaking change though, since we need to change the return type to a Result.

Expected complexity level and risk

1

Testing

Not much added testing, since this is a pretty mechanical change.

@jsdt jsdt added the api-break label Nov 8, 2024
@jsdt jsdt requested a review from Centril November 8, 2024 16:46
@jsdt jsdt marked this pull request as ready for review November 8, 2024 16:50
Copy link
Contributor

@kazimuth kazimuth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a good idea. I was just retouching these and didn't like that unwrap. However, it is an API break. My concern was that from_slice might already be being used by clients so I wasn't sure about changing it. from_be_slice is definitely fine to remove since it was just added.

We could also change them to return Option<Self> instead.

@bfops bfops removed the api-break label Nov 11, 2024
Copy link
Contributor

@gefjon gefjon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for cleaning this up. One minor terminology request: in the future, please don't use "unsafe" to describe Rust code unless it is unsafe, i.e. liable to invoke UB. Your PR title made me somewhat worried about this, when the actual changes are trivial and easy to approve.

crates/lib/src/identity.rs Outdated Show resolved Hide resolved
@gefjon
Copy link
Contributor

gefjon commented Nov 12, 2024

Spurious test failure should be fixed by #1979 . Please rebase onto master or merge master into this branch, at your preference, to confirm.

@jsdt jsdt changed the title Remove some unsafe Identity conversions that could panic Remove some Identity conversions that could panic Nov 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants