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 Blake3Hash always encode as Hex in JSON #3049

Merged
merged 6 commits into from
Sep 26, 2024
Merged

Make Blake3Hash always encode as Hex in JSON #3049

merged 6 commits into from
Sep 26, 2024

Conversation

teor2345
Copy link
Contributor

@teor2345 teor2345 commented Sep 23, 2024

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:

@teor2345 teor2345 added refactor improvement it is already working, but can be better labels Sep 23, 2024
@teor2345 teor2345 self-assigned this Sep 23, 2024
Copy link
Member

@nazar-pc nazar-pc left a 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/sc-proof-of-time/src/verifier/tests.rs Outdated Show resolved Hide resolved
crates/subspace-core-primitives/src/lib.rs Outdated Show resolved Hide resolved
crates/subspace-core-primitives/src/lib.rs Outdated Show resolved Hide resolved
crates/subspace-core-primitives/src/lib.rs Outdated Show resolved Hide resolved
crates/subspace-core-primitives/src/lib.rs Outdated Show resolved Hide resolved
crates/subspace-core-primitives/src/lib.rs Outdated Show resolved Hide resolved
crates/subspace-core-primitives/src/lib.rs Outdated Show resolved Hide resolved
Copy link
Contributor Author

@teor2345 teor2345 left a 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-core-primitives/src/lib.rs Outdated Show resolved Hide resolved
crates/subspace-core-primitives/src/lib.rs Outdated Show resolved Hide resolved
crates/subspace-core-primitives/src/lib.rs Outdated Show resolved Hide resolved
crates/subspace-core-primitives/src/lib.rs Outdated Show resolved Hide resolved
crates/sp-domains/src/merkle_tree.rs Outdated Show resolved Hide resolved
crates/subspace-core-primitives/src/lib.rs Outdated Show resolved Hide resolved
Copy link
Contributor Author

@teor2345 teor2345 left a 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?

crates/subspace-core-primitives/src/lib.rs Outdated Show resolved Hide resolved
Copy link
Member

@nazar-pc nazar-pc left a 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

crates/sc-proof-of-time/src/verifier/tests.rs Outdated Show resolved Hide resolved
crates/subspace-core-primitives/src/crypto.rs Outdated Show resolved Hide resolved
crates/subspace-core-primitives/src/lib.rs Outdated Show resolved Hide resolved
@teor2345
Copy link
Contributor Author

I also removed impl From<Blake3Hash> for PosSeed, there didn't seem much point for one usage.

@teor2345 teor2345 added this pull request to the merge queue Sep 26, 2024
Merged via the queue into main with commit fe71583 Sep 26, 2024
9 checks passed
@teor2345 teor2345 deleted the blake3-hash-hex branch September 26, 2024 03:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement it is already working, but can be better refactor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants