hash!: Unify all hash types, introduce "offchain" feature#236
hash!: Unify all hash types, introduce "offchain" feature#236joncinque merged 10 commits intoanza-xyz:masterfrom
Conversation
#### Problem There are many repeated `Hash` types in all of the different hasher crates, which brings in unnecessary dependencies and copied code. #### Summary of changes Unify all hashes under `solana_hash::Hash`, by re-exporting that type from keccak and blake3. While doing this work, I also cleaned up the dependencies, which were unnecessarily bringing in the crate for the offchain implementation. When integrating the SDK with Pinocchio, we'll have many situations in which Pinocchio doesn't want to have an offchain fallback implementation available, but that fallback is still used by other downstream users. To satisfy both uses, this PR also introduces a general `"offchain"` crate feature. For example, `Pubkey::find_program_address` could use the offchain feature for deriving the address without using the syscall, which is useful for most offchain applications. I could also split off the offchain feature in a different PR if that's clearer.
febo
left a comment
febo
left a comment
There was a problem hiding this comment.
The changes look great! I am not entirely sure on the name of the feature "offchain". I wonder if we will have crates that offer multiple features for "offchain" things, which means that a single feature won't fit all cases. At the same time, I can't come up with an alternative name, apart from using a name related to the functionality/crate being enabled by the feature.
We can go with that too, as the crates have been since the breakup. I was thinking a common name would be better for downstream users, so they don't need to read through our source code. On the flip-side, maybe that's not a big deal, since the compiler will tell you if you're trying to use something and it's not available due to a feature-gate, or our panic messages will tell that too. So if you don't like it, I'll revert. I don't feel that strongly about it. |
| } | ||
|
|
||
| #[cfg(test)] | ||
| #[cfg(feature = "blake3")] |
There was a problem hiding this comment.
It seems that the feature is enabled on the dev-dependencies, so we probably don't need this. I might have missed that on my first review. Not sure if the idea is to have this pattern, but it can get a bit verbose for crates that have many features.
febo
left a comment
febo
left a comment
There was a problem hiding this comment.
Looks great! I think having a "named" feature works well, so it is indicative of the functionality being enabled.
Yeah, I had the new |
* hash!: Unify all hash types, introduce "offchain" feature #### Problem There are many repeated `Hash` types in all of the different hasher crates, which brings in unnecessary dependencies and copied code. #### Summary of changes Unify all hashes under `solana_hash::Hash`, by re-exporting that type from keccak and blake3. While doing this work, I also cleaned up the dependencies, which were unnecessarily bringing in the crate for the offchain implementation. When integrating the SDK with Pinocchio, we'll have many situations in which Pinocchio doesn't want to have an offchain fallback implementation available, but that fallback is still used by other downstream users. To satisfy both uses, this PR also introduces a general `"offchain"` crate feature. For example, `Pubkey::find_program_address` could use the offchain feature for deriving the address without using the syscall, which is useful for most offchain applications. I could also split off the offchain feature in a different PR if that's clearer. * Guard Hasher re-export with not(target_os = "solana") * Fixup CI * Properly use offchain feature * Oops, missing semi-colon * Enable offchain feature in offchain-message * Fix doctests * Improve panic message * Require offchain feature for tests * Revert feature names
2 participants
Problem
There are many repeated
Hashtypes in all of the different hasher crates, which brings in unnecessary dependencies and copied code.Summary of changes
Unify all hashes under
solana_hash::Hash, by re-exporting that type from keccak and blake3.While doing this work, I also cleaned up the dependencies, which were unnecessarily bringing in the crate for the offchain implementation.
When integrating the SDK with Pinocchio, we'll have many situations in which Pinocchio doesn't want to have an offchain fallback implementation available, but that fallback is still used by other downstream users.
To satisfy both uses, this PR also introduces a general
"offchain"crate feature. For example,Pubkey::find_program_addresscould use the offchain feature for deriving the address without using the syscall, which is useful for most offchain applications.I could also split off the offchain feature in a different PR if that's clearer.
cc @kevinheavey in case you want to weigh in on this too