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

feat: Create keys from owned array values #781

Merged
merged 3 commits into from
Feb 19, 2025

Conversation

uncomputable
Copy link
Contributor

Construct KeyPair directly from [u8; 32]. I can change the parameter to &[u8; 32] depending on the discussion in #780.

@uncomputable
Copy link
Contributor Author

Deprecation of a method is a candidate for the changelog.

Update TupleVisitor{32,33} to pass its owned byte array to the parsing
function instead of a mere byte slice. This gives us more flexibility
inside the parsing function.
Construct KeyPair directly from [u8; 32].
Deprecate KeyPair::from_seckey_slice
and replace all of its calls with the new method.
@uncomputable
Copy link
Contributor Author

Fixed formatting

@apoelstra
Copy link
Member

Do we have serde regression tests here? I'm pretty nervous that the first commit changes the serialization format.

@uncomputable
Copy link
Contributor Author

This PR leaves the serialization format unchanged. The changes in TupleVisitor{32,33} merely affect how the byte array is passed to the parsing function.

Kixunil
Kixunil previously approved these changes Feb 16, 2025
Copy link
Collaborator

@Kixunil Kixunil left a comment

Choose a reason for hiding this comment

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

ACK e7eea32

I'm convinced this doesn't change serialization and whatever problems there are were there already.

src/key.rs Show resolved Hide resolved
src/serde_util.rs Show resolved Hide resolved
src/serde_util.rs Show resolved Hide resolved
@uncomputable uncomputable changed the title feat: KeyPair::from_seckey_byte_array feat: Create keys from owned array values Feb 17, 2025
@uncomputable
Copy link
Contributor Author

I updated the other key constructors to take owned array values. I updated the PR title and added an entry to the changelog for a breaking change.

@Kixunil I didn't see your approval until now; sorry.

@Kixunil
Copy link
Collaborator

Kixunil commented Feb 18, 2025

The lint failure is real, not sure about WASM.

@apoelstra
Copy link
Member

WASM we can ignore. We should probably just drop the CI job entirely until they fix my 18-month-old issue about wasm-pack not working at all in CI setups.

@Kixunil
Copy link
Collaborator

Kixunil commented Feb 18, 2025

Oh, crap. Side note: install should've defaulted to --lock.

I updated the constructors to take owned array values of the form
[u8; LEN] instead of taking array references of the form &[u8; LEN].
This makes the constructors more canonical. Already in this commit,
I could remove a bunch of calls to `&` or `*`.

Because this is a breaking change, I added an entry to the changelog.
@uncomputable
Copy link
Contributor Author

WASM is already broken on master. When CI is never 100% green, it's easy to overlook new CI failures. Let's see if I can fix WASM in a follow-up PR.

In any case, I resolved the clippy lint.

@apoelstra
Copy link
Member

Let's see if I can fix WASM in a follow-up PR.

Thanks! I would appreciate that, though bear in mind that I will never test it locally until they fix wasm-pack so that it's possible to run it without it attempting to download and execute random things.

Copy link
Collaborator

@Kixunil Kixunil left a comment

Choose a reason for hiding this comment

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

ACK 59cf7bd

Just FYI, deprecation is not a breaking change (yet), only removal is. Still, the changelog note is warranted.

Copy link
Member

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

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

ACK 59cf7bd; successfully ran local tests; nice! the old function signatures were super weird. and the docs were wrong lol

Copy link
Member

@tcharding tcharding left a comment

Choose a reason for hiding this comment

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

ACK 59cf7bd

@apoelstra apoelstra merged commit ede62d5 into rust-bitcoin:master Feb 19, 2025
29 of 30 checks passed
@uncomputable uncomputable deleted the 2025-02-array-params branch February 19, 2025 22:31
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