Skip to content

Manually implement serde traits for OutPoint - #4933

Merged
apoelstra merged 2 commits into
rust-bitcoin:masterfrom
tcharding:push-yolptywqkzpy
Sep 10, 2025
Merged

Manually implement serde traits for OutPoint#4933
apoelstra merged 2 commits into
rust-bitcoin:masterfrom
tcharding:push-yolptywqkzpy

Conversation

@tcharding

Copy link
Copy Markdown
Member

Death to all macros.

This was pulled out of #4902, review suggestions over there are relevant. Also I started #4932 after working on this.

@tcharding

tcharding commented Sep 3, 2025

Copy link
Copy Markdown
Member Author

Hand written, no vibe coded junk. lol, 'hand written' - I cannot believe we have to say that now.

@tcharding
tcharding force-pushed the push-yolptywqkzpy branch 2 times, most recently from b9cfdd8 to a460b6a Compare September 3, 2025 00:59
Comment thread primitives/src/transaction.rs Outdated
// serializing as an array was found in the past to break for some serializers so we use
// a slice instead. This causes 8 bytes to be prepended for the length (even though this
// is a bit silly because know the length).
state.serialize_field("txid", &self.txid.as_byte_array()[..])?;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Maybe .as_slice(), instead of &x[..]? It's few characters longer, but expresses the intention well matching the comment? I donno, just a thought while scanning through it.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Agreed (though if this is the only thing in this PR I won't hold it up for it).

For the most part throughout this crate we us &x[..] because for the most part it doesn't matter .. but in this case we're being deliberate about the slice vs array-ref distunction.

@tcharding
tcharding marked this pull request as draft September 3, 2025 22:15
@tcharding

Copy link
Copy Markdown
Member Author

TODO: Woops, note to self, remove first patch. I gotta dash rn

@tcharding
tcharding marked this pull request as ready for review September 3, 2025 23:05
@tcharding

Copy link
Copy Markdown
Member Author

Force push, after fixing, is just usage of as_slice.

@apoelstra

Copy link
Copy Markdown
Member

Needs rebase.

@tcharding

Copy link
Copy Markdown
Member Author

Rebase only, fixed conflicts in imports.

@apoelstra

Copy link
Copy Markdown
Member

4d549cc looks good, but can you add a fixed test vector for serde_json::from_str and bincode::deserialize?

(Sorry, I meant to post this a week ago, but got halfway through typing it and tabbed away..)

@tcharding

Copy link
Copy Markdown
Member Author

No stress, done.

@apoelstra

Copy link
Copy Markdown
Member

The new tests do not compile :).

This is likely (hopefully) tested elsewhere but in preparation for
manually implementing serde traits for `OutPoint` add
deserialization and roundtrip tests for both human and non-human
readable.
Reading Rust macros makes my eyes bleed - the
`internals::serde_struct_human_string_impl` is a great example.

The macro is used in exactly one place, for `OutPoint`.

Implement `serde` stuff for `OutPoint` and remove
`serde_struct_human_string_impl` - death to all macros.
@tcharding

tcharding commented Sep 9, 2025

Copy link
Copy Markdown
Member Author

Goodness me that was a mess, add IOU one whiskey to your tally.

@apoelstra apoelstra left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

ACK 42d3039; successfully ran local tests

@apoelstra
apoelstra merged commit 795ac3e into rust-bitcoin:master Sep 10, 2025
25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants