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

refactor(identity): call Into::into instead of copying the slice #5788

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

drHuangMHT
Copy link
Contributor

@drHuangMHT drHuangMHT commented Jan 3, 2025

Description

Fixes:

// FIXME: Once `generic-array` hits 1.0, we should be able to just use `Into` here.
let mut array = [0u8; 32];
array.copy_from_slice(generic_array.as_slice());

Related: #3850

Notes & open questions

Internal change.

Change checklist

  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • A changelog entry has been made in the appropriate crates

@drHuangMHT drHuangMHT marked this pull request as ready for review January 3, 2025 06:09
Copy link
Member

@dariusc93 dariusc93 left a comment

Choose a reason for hiding this comment

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

LGTM. Left a small minor comment.

Can you update the changelog, and, if applicable, Cargo.toml?

array.copy_from_slice(generic_array.as_slice());

let message = Message::parse(&array);
let message = Message::parse(&Sha256::digest(msg).into());
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we should have it in separate variables before passing it to Message::parse

eg

Suggested change
let message = Message::parse(&Sha256::digest(msg).into());
let digest = Sha256::digest(msg);
let message = Message::parse(&digest.into());

Thoughts? Not that it should impact anything from a glance over, but just thinking about readability.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe the information is sufficient since the method is called digest. Calling into introduces some ambiguity but I don't think it matters.

@elenaf9
Copy link
Contributor

elenaf9 commented Jan 4, 2025

I am not sure, but I think the FIXME note was added because we don't want to use Into until generic-array is at a stable version >= 1.0.0?

If so, then I think that reasoning still applies, because we still indirectly depend on generic-array v.0.14.0 and not on a stable version >= v1.0.0. Am I missing something?

@drHuangMHT
Copy link
Contributor Author

I am not sure, but I think the FIXME note was added because we don't want to use Into until generic-array is at a stable version >= 1.0.0?

If so, then I think that reasoning still applies, because we still indirectly depend on generic-array v.0.14.0 and not on a stable version >= v1.0.0. Am I missing something?

Oops I forgot to check the actual dependency tree. But it compiles for some reason.

@elenaf9
Copy link
Contributor

elenaf9 commented Jan 4, 2025

Oops I forgot to check the actual dependency tree. But it compiles for some reason.

I believe we were already able to use Into at the point where the above FIXME was added, but it was decided against it because of the mentioned stabilization. But not sure why that decision was made; as far as I can tell we're just converting between GenericArray and a slice?

@drHuangMHT
Copy link
Contributor Author

I believe we were already able to use Into at the point where the above FIXME was added, but it was decided against it because of the mentioned stabilization.

as far as I can tell we're just converting between GenericArray and a slice?

I think so.

But not sure why that decision was made;

I have no idea because that's not even our direct dependency.

@drHuangMHT drHuangMHT marked this pull request as draft January 5, 2025 14:32
Copy link
Member

@jxs jxs left a comment

Choose a reason for hiding this comment

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

LGTM! Yeah I also wondered that @elenaf9, the FIXME was introduced with #3850
but no further explanation exists, I'd say we merge this, thoughts?

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