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

Implement de/serialization for SharedSecret #418

Merged
merged 1 commit into from Mar 11, 2022

Conversation

tcharding
Copy link
Member

@tcharding tcharding commented Mar 10, 2022

As we do for other keys implement serde de/serialization for the SharedSecret.

Please note, this adds from_slice and from_bytes (borrowed and owner respectively) because I needed to use them. Doing so treads on @dspicher's toes because he is in the process of implementing an owned conversion method for SharedSecret. The fair thing to do would be let #417 resolve and merge before merging this one (I can rebase).

Side note, its kind of rubbish that BytesVisitor deserializes into a buffer (presumably) then we reallocate and copy the buffer to use the borrowed conversion method due to the serde function signature fn visit_bytes<E: de::Error>(self, v: &[u8]) -> Result<Self::Value, E> (I was bumbling nonsense.)

Closes: #416

@apoelstra
Copy link
Member

Will wait for #417 (just needs CI to finish and then I'll merge it) -- but will hold off on publishing the new version until this is in.

As we do for other keys implement serde de/serialization for the
`SharedSecret`. Includes implementation of `from_slice` method that is
the borrowed version of `from_bytes` as well as a `FromStr`
implementation that parses a hex string.
@tcharding
Copy link
Member Author

What's the go with the four CI jobs that don't run immediately (seemed this changed a week or two ago)? Was that on purpose or did we botch something?


- Stable - Docs / WebAssembly Build (stable) Expected — Waiting for status to be reported Required
- Tests (1.29.0) Expected — Waiting for status to be reported Required
- Tests (beta) Expected — Waiting for status to be reported Required
- Tests (stable) Expected — Waiting for status to be reported 

@apoelstra
Copy link
Member

Oh, I think we removed those but github requires that I go manually uncheck them. I'll do that now.

apoelstra
apoelstra previously approved these changes Mar 11, 2022
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 d68531d815425bd091855248afcc9c60d490584d

@apoelstra apoelstra dismissed their stale review March 11, 2022 21:46

acked in wrong tab

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 de65fb2

@apoelstra apoelstra merged commit 49905b0 into rust-bitcoin:master Mar 11, 2022
@tcharding tcharding deleted the shared-secret-serde branch March 21, 2022 23:09
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.

SharedSecret should have a "deserialize from bytes" method and serde impls
2 participants