Skip to content

Add TxIdentifier trait#2987

Merged
apoelstra merged 1 commit intorust-bitcoin:masterfrom
tcharding:07-09-as-tx-identifier
Jul 10, 2024
Merged

Add TxIdentifier trait#2987
apoelstra merged 1 commit intorust-bitcoin:masterfrom
tcharding:07-09-as-tx-identifier

Conversation

@tcharding
Copy link
Copy Markdown
Member

@tcharding tcharding commented Jul 9, 2024

Add a new trait TxIdentifier that abstracts over the Txid and Wtxid types. We make AsRef a super trait so that the new trait needs no methods.

Seal the trait so consumers of the library cannot implement it.

Use the new trait in:

  • the bip152 module to tighten up the with_siphash_keys function
  • as a trait bound on the Leaf associated type in the MerkleNode trait

@github-actions github-actions bot added the C-bitcoin PRs modifying the bitcoin crate label Jul 9, 2024
Add a new trait `TxIdentifier` that abstracts over the `Txid` and
`Wtxid` types. We make `AsRef` a super trait so that the new trait needs
no methods.

Seal the trait so consumers of the library cannot implement it.

Use the new trait in:

- the `bip152` module to tighten up the `with_siphash_keys` function
- as a trait bound on the `Leaf` associated type in the `MerkleNode` trait
@tcharding tcharding force-pushed the 07-09-as-tx-identifier branch from 3bd8032 to a738754 Compare July 9, 2024 02:28
Copy link
Copy Markdown
Member

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

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

ACK a738754

match version {
1 => short_ids.push(ShortId::with_siphash_keys(&tx.compute_txid(), siphash_keys)),
2 => short_ids.push(ShortId::with_siphash_keys(&tx.compute_wtxid(), siphash_keys)),
_ => unreachable!(),
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

OT but this should be an enum. Honestly last time I looked at this module I didn't like it at all. IIUC it's a literal transaction from C++ Core code, so that's why it's not idiomatic.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yeah IMO all the bip modules need a bunch of love, I wasn't going to give it to them until after all the crate smashing is done.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I made an issue

Copy link
Copy Markdown
Collaborator

@Kixunil Kixunil left a comment

Choose a reason for hiding this comment

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

ACK a738754

@apoelstra apoelstra merged commit 5ad78cc into rust-bitcoin:master Jul 10, 2024
@tcharding tcharding deleted the 07-09-as-tx-identifier branch July 11, 2024 03:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

C-bitcoin PRs modifying the bitcoin crate

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants