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

clean up encoding/decoding traits #265

Open
wants to merge 5 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@apoelstra
Copy link
Member

commented May 23, 2019

Builds on #264

@apoelstra apoelstra force-pushed the apoelstra:2019-05-enc-dec-cleanup branch from 2c532d4 to 2a8ad22 May 23, 2019

@apoelstra apoelstra force-pushed the apoelstra:2019-05-enc-dec-cleanup branch 2 times, most recently from b991f8a to bcb25b0 Jun 5, 2019

@TheBlueMatt

This comment has been minimized.

Copy link
Member

commented Jun 5, 2019

ACK if you make it fucking compile.

@stevenroose
Copy link
Collaborator

left a comment

What's the concrete motivation for this change?

#[inline]
fn consensus_decode(d: &mut D) -> Result<$thing, ::consensus::encode::Error> {
use consensus::encode::Decodable;
use consensus::Decodable;

This comment has been minimized.

Copy link
@stevenroose

stevenroose Jun 6, 2019

Collaborator

Given that all type references in the macros have the :: prefix, this one should have it too I guess.


#[test]
fn serialize_test() {
assert_eq!(serialize(&Network::Bitcoin), vec![0xf9, 0xbe, 0xb4, 0xd9]);

This comment has been minimized.

Copy link
@stevenroose

stevenroose Jun 6, 2019

Collaborator

Couldn't you have kept the tests, but just added .magic()?

@apoelstra apoelstra force-pushed the apoelstra:2019-05-enc-dec-cleanup branch 2 times, most recently from 8352c62 to d701884 Jun 6, 2019

@apoelstra

This comment has been minimized.

Copy link
Member Author

commented Jun 6, 2019

Addressed Steven's nits.

@stevenroose the reason for renaming Encoder and Decoder is that Matt says he was misled by the old names; for returning the length from encodable is so that you can determine the length of hashed data without re-encoding it into some special-purpose counting Write; for returning usize from size-returning functions is so that you don't need a gazillion casts everywhere to use them.

@stevenroose

This comment has been minimized.

Copy link
Collaborator

commented Jun 6, 2019

@apoelstra

This comment has been minimized.

Copy link
Member Author

commented Jun 6, 2019

Yes, we are saying that. The argument (from @TheBlueMatt) is that we're not intending to create or support a general serialization library, and "Bitcoin extension stuff" is not Bitcoin.

@stevenroose

This comment has been minimized.

Copy link
Collaborator

commented Jun 6, 2019

@TheBlueMatt

This comment has been minimized.

Copy link
Member

commented Jun 6, 2019

You still shouldn't be using the Rust encoding traits for that. If you build some demo app, you can trivially build a X : std::io::Write and then call the rust-bitcoin traits to write them when you have a rust-bitcoin object and write the other data manually (which you'd have to do anyway).

@TheBlueMatt

This comment has been minimized.

Copy link
Member

commented Jun 6, 2019

More importantly, the stuff in rust-bitcoin is really easy to misuse, and unless we want to commit to cleaning it up a ton and making it robust, its pretty clear we shouldn't be exposing it.

@apoelstra

This comment has been minimized.

Copy link
Member Author

commented Jun 7, 2019

I don't really see how you can "misuse" serialization traits, but I agree they're fairly special-purpose and I'd like to retain the ability to change them (in particular they probably support some unnecessary things, like serializing i16) without causing tons of downstream pain.

@apoelstra

This comment has been minimized.

Copy link
Member Author

commented Jun 7, 2019

In person Matt points out that having an impl for byte-slices (or any slices, really) interacts badly with Rust's implicit coercions, which can convert fixed-width types to variable-width types resulting in surprising serializations.

Given that serde has the same issue (see rust-bitcoin/rust-secp256k1#73 for example) I'm not very convinced by this. Removing the impl for &[T], which happened in #264, solved this.

@TheBlueMatt

This comment has been minimized.

Copy link
Member

commented Jun 7, 2019

Well its also an example of why this framework isnt really designed to be a generic, easy-to-use API (ie the only type of thing we should export) - its easy to be confused as to what will happen. If we want to support it as such we can put in some work to document it properly, make sure its consistent for such issues, then fine, but until then I think its pretty clearly not something that meets our export criteria.

@TheBlueMatt

This comment has been minimized.

Copy link
Member

commented Jun 7, 2019

And if we do want a supported, public serialization framework, it should certainly be its own crate.

@apoelstra apoelstra force-pushed the apoelstra:2019-05-enc-dec-cleanup branch from d701884 to ac4ff61 Jun 7, 2019

@apoelstra

This comment has been minimized.

Copy link
Member Author

commented Jun 7, 2019

rebased

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.