Skip to content

Allow key deserialization from owned strings #217

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

Closed
wants to merge 1 commit into from

Conversation

sgeisler
Copy link
Contributor

This patch allows the underlying parser impl to either return a String or a &'de str. Deserializing json encoded keys from Readers fails otherwise.

@sgeisler
Copy link
Contributor Author

Adding no-std support made the PR quite ugly. Does anyone have an idea how to implement it more elegantly?

@elichai
Copy link
Member

elichai commented May 15, 2020

Damn It.
So I spent a few hours re-implementing this such that it will work without the no-std trick (and will also work for Owned bytes(ie Vec))
and after I finished apparently PublicKey already have a working impl of its own outside of the macro which is almost exactly the same as the one I wrote: https://github.com/rust-bitcoin/rust-secp256k1/blob/master/src/key.rs#L392

I think I'll remove the macro all together because only SecretKey uses it and the other types have their own shticks.
And I'll add more tests that actually test this bug

@sgeisler
Copy link
Contributor Author

Oh, damn it, I didn't notice the macro was only used there, that's ugly indeed. Do you want to take this over? If not I can do it too.

@elichai
Copy link
Member

elichai commented May 15, 2020

Oh, damn it, I didn't notice the macro was only used there, that's ugly indeed. Do you want to take this over? If not I can do it too.

Please see #218 :)

@sgeisler
Copy link
Contributor Author

I think #218 is the better version now.

@sgeisler sgeisler closed this May 22, 2020
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.

2 participants