Introduce more serde utils #92
Conversation
|
If this lands we can use this to assist the crate smashing effort in |
tcharding
force-pushed
the
06-20-serde-utils
branch
4 times, most recently
from
4dc955c to
9866e27
Compare
|
To clarify -- you copied this from rust-bitcoin? Or from std? |
|
Ah, it's from rust-bitcoin. Cool. |
|
@tcharding can you update Cargo.toml to turn on the I'm not sure why cargo seems to be OK with the current PR but it's technically incorrect and my local non-cargo tooling is breaking on it. |
We derive `serde` traits so we need the `derive` feature to be enabled in all builds not just dev builds. Presumably this bug has not shown up because anytime `serde` is enabled some other crate in the stack enabled `derive`.
Copy the `serde_utils` modules for ser/deser of `BTreeMap`s. Currently includes the `hex_bytes` module even though it would seem we don't need it but I couldn't get it to work with the current functions. Its private as opposed to the original where it was public.
tcharding
force-pushed
the
06-20-serde-utils
branch
from
7d2091d to
36cf5d7
Compare
|
Added an additional patch up front. |
There was a problem hiding this comment.
I believe the first commit is just plain wrong (see below) and second has questionable usefulness.
Side note: I never intended for hex-conservative to be a dumping ground for random hex-related shit. I want it to be a nice idiomatic crate. Adding stuff that's only used in one crate until someone else needs it is not helpful to achieve this goal.
| @@ -27,11 +27,10 @@ alloc = [] | |||
| [dependencies] | |||
| arrayvec = { version = "0.7.2", default-features = false } | |||
| core2 = { version = "0.3.2", default-features = false, optional = true } | |||
| serde = { version = "1.0", default-features = false, optional = true } | |||
| serde = { version = "1.0", default-features = false, features = ["derive"], optional = true } | |||
There was a problem hiding this comment.
No, where do you see any use of derive? I only see it in examples.
| @@ -106,3 +106,304 @@ where | |||
| d.deserialize_map(HexVisitor(PhantomData)) | |||
| } | |||
| } | |||
|
|
|||
| pub(crate) struct SerializeBytesAsHex<'a>(pub(crate) &'a [u8]); | |||
There was a problem hiding this comment.
I believe this no longer needs pub(crate).
| d.deserialize_str(Visitor(core::marker::PhantomData)) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
These btree map things are super-specific and I don't think they'll be shared by many crates. Even in bitcoin we use it for PSBT which has weird serialization. I'd rather remove them entirely rather than put them here.
Oh, this PR causes it, it wasn't a problem before. Having it in two commits is confusing. So if this is accepted I'd prefer to squash the commits since they don't make sense separated. That being said, adding another dependency for this is annoying, it'd be nice trying to impl it manually since it just uses |
Should we start |
|
This particular one is only needed for PSBT. If we need it for multiple things and multiple crates then that would be a strong indication that putting it into |
|
@tcharding is it only PSBT? Actually, if it's really only PSBT then we can maybe get away with dropping the serde serialization entirely and just serializing as strings... I really dislike breaking serde de/serialization but this one is really ad-hoc. |
|
Exactly. Serialize as base64. |
|
Used also in bitcoin/src/blockdata/witness.rs:476: seq.serialize_element(&crate::serde_utils::SerializeBytesAsHex(elem))?; |
|
Damn, I had a huge brain fart yesterday forgetting there's also |
|
For posterity, this was closed in favour of #96 |
Copy the
serde_utilsmodules for ser/deser ofBTreeMaps. Currently includes thehex_bytesmodule even though it would seem we don't need it but I couldn't get it to work with the current functions. Its private as opposed to the original where it was public.