-
Notifications
You must be signed in to change notification settings - Fork 305
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
Extract keccak-hasher crate #3408
Conversation
ac92137
to
b40649b
Compare
b16f644
to
8c2912d
Compare
8c2912d
to
0688483
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.
Looks good overall, just a few small things
sdk/keccak-hasher/Cargo.toml
Outdated
|
||
[dependencies] | ||
borsh = { workspace = true, optional = true } | ||
bs58 = { workspace = true } |
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.
Since this is using solana-hash, I don't think this dependency is needed
sdk/keccak-hasher/src/lib.rs
Outdated
solana_sanitize::Sanitize, | ||
std::{convert::TryFrom, fmt, str::FromStr}, | ||
thiserror::Error, | ||
std::{fmt, str::FromStr}, |
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.
how about making this crate no_std
like the hash crate? this can use core::{fmt, str::FromStr}
. It can have an std
feature which pretty much just enables the std
feature on solana-hash
// TODO: replace this with `solana_hash::Hash` in the | ||
// next breaking change. | ||
// It's a breaking change because the field is public | ||
// here and private in `solana_hash`, and making | ||
// it public in `solana_hash` would break wasm-bindgen |
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.
Someone tried to combine all the hashes awhile ago, and we couldn't for this exact reason 🙃
sdk/keccak-hasher/Cargo.toml
Outdated
bs58 = { workspace = true } | ||
serde = { workspace = true, optional = true } | ||
serde_derive = { workspace = true, optional = true } | ||
solana-atomic-u64 = { workspace = true } |
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.
This dependency can also be removed
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.
Looks great, thanks! @yihau can you accept solana-keccak-hasher
?
Problem
solana_program::keccak
imposes asolana_program
dep on some SPL cratesSummary of Changes
solana_program::keccak
to a new cratesolana-keccak-hasher
, modelled onsolana-sha256-hasher
(see sdk: Extract hash and hasher crates #2015)Hash
struct withsolana_hash::Hash
but it won't be able to do so without a breaking change. In the meantime I've just made it rely onsolana_hash
as much as possible while avoiding breakageThis branches off #3405 so that needs to be merged first (update: done)