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

[Discussion] Do we need data type for TxID? #284

Open
dr-orlovsky opened this issue Jun 23, 2019 · 6 comments

Comments

@dr-orlovsky
Copy link
Contributor

commented Jun 23, 2019

Currently sha256d::Hash type is used for all txids. However, this type is also used by many other projects using rust-bitcoin for other purposes as well, like working with generic hashes, not related to the transactions. Thus as a safety reason I propose to define a custom type alias for txid as TransactionID. If there are no objections, I'll do the corresponding PR

@dpc

This comment has been minimized.

Copy link
Contributor

commented Jun 23, 2019

In the project I'm using rust-bitcoin for, I do use type-alias BlockHash and TxHash for readability. I wanted to try newtypes, but haven't got to it yet.

I think it does makes sense to have these types wrapping the underlying hash, to allow people expressing which hash exactly they expect, and do it in interoperable way between different project using rust-bitcoin.

The type itself can probably Deref to the underlying hash type, and have From implemented too.

@dr-orlovsky

This comment has been minimized.

Copy link
Contributor Author

commented Jun 24, 2019

I will work on the change and PR it then

@apoelstra

This comment has been minimized.

Copy link
Member

commented Jul 27, 2019

Sounds good to me. We've gotten a lot of mileage out of the OutPoint type which is similar.

@stevenroose

This comment has been minimized.

Copy link
Collaborator

commented Jul 29, 2019

It's actually quite annoying to do a newtype on those types because of all the impls we want on them. I recently did this for AssetId: https://github.com/stevenroose/rust-elements/blob/elementshash/src/issuance.rs#L37 and I skipped lots of the index traits.

For rust-slip21 I did a new ([u8; 64]) newtype with all the traits (index borrow asref serde ...) and you can see most of the create is trait impls: https://github.com/stevenroose/rust-slip21/blob/master/src/lib.rs#L17

That being said, I think it's worth it to have the types. Just saying might be worth it to find a more efficient way to do it. Even though most of the time it's just copy pasting. Perhaps we should have a bitcoin_hashes_newtype macro somewhere that can be used to create newtypes around bitcoin_hashes newtypes. That can just upstream most impls.

@apoelstra

This comment has been minimized.

Copy link
Member

commented Aug 3, 2019

Yeah, I think it makes sense to define a hash_newtype macro of some sort in bitcoin_hashes. We have similar requirements for Simplicity where we have different types of Merkle roots that we want to keep separate.

@stevenroose

This comment has been minimized.

Copy link
Collaborator

commented Aug 13, 2019

I created a newtype macro: rust-bitcoin/bitcoin_hashes#59

With that macro, we could easily define types like

hash_newtype!(TxHash, sha256d::Hash, doc="A Bitcoin transaction hash/transaction ID.");
hash_newtype!(BlockHash, sha256d::Hash, doc="A Bitcoin block hash.");
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.