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

Make hex an optional dep for hashes #2654

Open
TheBlueMatt opened this issue Apr 3, 2024 · 8 comments · May be fixed by #2711
Open

Make hex an optional dep for hashes #2654

TheBlueMatt opened this issue Apr 3, 2024 · 8 comments · May be fixed by #2711

Comments

@TheBlueMatt
Copy link
Member

ISTM hex isn't really all that "required" to do hashing, and personally I'd very much like to use bitcoin_hashes with ~zero dependencies (with potentially eyeing building outside of cargo). It seems making hex an optional dependency is pretty easy, just requires actually adding the right feature tags (and maybe one or two sealed traits). Is this something folks would be open to?

@apoelstra
Copy link
Member

Yep! I believe we had discussed this earlier. I think we want to do the hash trait splitting as part of the same effort (see #2496).

@tcharding tcharding linked a pull request Apr 23, 2024 that will close this issue
@tcharding
Copy link
Member

I tried searching but can't find or remember why one should not feature gate trait implementations. Furthermore I don't understand why this issue mentions sealed traits.

To make hex optional it seems we need to feature gate all the FromStr / Display impls - what am I missing please.

@apoelstra
Copy link
Member

To make hex optional it seems we need to feature gate all the FromStr / Display impls - what am I missing please.

Display in principle we can retain using the core traits. Though we may need to have an alternate/less efficient implementation, and once we implement all the width/precision bullshit we may find that it's not worth the effort. (But maybe not...I think these are really easy to implement if your core algorithm is just a stupid "loop through the bytes and write!(f, "{:02x}", b) them".)

@apoelstra
Copy link
Member

But yes, we would definitely need to feature-gate FromStr behind the hex feature. There is no reason not to do this. I think you're misremembering something.

@tcharding
Copy link
Member

tcharding commented May 3, 2024

Oh that's right we literally had this conversation a week ago you noted that std::error::Error works fine feature gated. Guess something was still floating around my brain unresolved.

@tcharding
Copy link
Member

tcharding commented May 3, 2024

One quick and dirty thing I tried was just printing "<hexidecimal not implemented without hex feature> {:?}" but that is a bit rude.

@apoelstra
Copy link
Member

No, we should definitely either support it "properly" (ok to have crappy support for format specifiers initially but we should aim to have it complete by 1.0) or have it not compile.

@tcharding
Copy link
Member

or have it not compile.

I really do like this option but because of orphan rule it leaves no_std users with the only option of using Debug if they want to display a hash. I think this is too rude, if we say we support no_std we should put the effort in to do it properly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants