-
Notifications
You must be signed in to change notification settings - Fork 247
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 Blake3Hash always encode as Hex in JSON #3049
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some suggestions on API improvements. Thanks for talking care of this, I know it was tedious.
crates/subspace-farmer/src/bin/subspace-farmer/commands/benchmark.rs
Outdated
Show resolved
Hide resolved
crates/subspace-networking/src/behavior/persistent_parameters.rs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can do most of the changes you asked for, but some might end up being a bit tricky.
Let's see how much easier it is when I fix the incorrect merkle tree type.
crates/subspace-farmer/src/bin/subspace-farmer/commands/benchmark.rs
Outdated
Show resolved
Hide resolved
b1b069c
to
6dc7222
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think these are pretty much the changes you wanted?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few nits, looks good otherwise
6dc7222
to
d22f20d
Compare
d22f20d
to
7e917a3
Compare
I also removed |
This PR makes sure
Blake3Hash
is always hex-encoded in JSON and debug contexts.It replaces the type alias to
[u8; 32]
with a wrapper type, with appropriate trait impls and methods.This is slightly tricky due to Rust's trait coherence rules: arrays and slices are always foreign types.I'm happy to add or change trait impls to reduce the other code changes, but I've tried a lot of alternatives already. So significant changes might be better as a PR on top of this one.
Code contributor checklist: