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

encode: add some more generic impls (more tuples, references) #548

Merged
merged 2 commits into from Jan 9, 2021

Conversation

apoelstra
Copy link
Member

No description provided.

@sgeisler
Copy link
Contributor

sgeisler commented Jan 9, 2021

Do you want to fix the breakage caused by byteorder changing its MSRV here or in a separate PR? I already have a commit that does that 9657048 but didn't want to push it here. There is a high chance that it'll need a god-mode merge for being a CI change 😕

@apoelstra
Copy link
Member Author

Thanks! I'll just cherry-pick your commit here.

It shouldn't need a god-mode merge, for what it's worth. Only enabling GA for the first time did.

@sgeisler
Copy link
Contributor

sgeisler commented Jan 9, 2021

It shouldn't need a god-mode merge, for what it's worth

It really should in a secure system: I could change the CI code such that it reveals CI secrets, so it's not safe to run the PRed CI code until it is merged. But I guess it will work due to the indirection with the bash script. CI security is a mess.

@apoelstra
Copy link
Member Author

I mean, there are options I can enable that'd make it impossible for outsiders to change CI. But I haven't enabled them.

They broke their MSRV in a minor release.

Co-authored-by: Sebastian Geisler <sebastian@blockstream.io>
Copy link
Contributor

@sgeisler sgeisler left a comment

Choose a reason for hiding this comment

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

ACK ef116ff

Thinking about it a bit more you could probably also get away with implementing Encodable for T: AsRef<E> where E: Encodable to support all kinds of smart pointers, but the current version is much more transparent.

@apoelstra
Copy link
Member Author

@sgeisler no, we specifically avoid &[u8], even though it would be useful, because @TheBlueMatt complained that arrays sometimes coerced to it, causing them to be length-prefixed when arrays normally are not.

@apoelstra
Copy link
Member Author

Actually, I think T: AsRef<E> wouldn't cover &[u8] ... but I'm not sure what it does cover, so agree that it's best to be more transparent.

@sgeisler
Copy link
Contributor

sgeisler commented Jan 9, 2021

It would, but I had to look it up too 😆

@sanket1729
Copy link
Member

tACK ef116ff

Copy link
Member

@sanket1729 sanket1729 left a comment

Choose a reason for hiding this comment

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

re tACK ef116ff

@apoelstra apoelstra merged commit fd41157 into rust-bitcoin:master Jan 9, 2021
@apoelstra apoelstra deleted the 2021-01--more-encodable branch January 9, 2021 23:55
@sgeisler sgeisler modified the milestone: 0.26.1 Feb 19, 2021
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.

None yet

3 participants