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

NativeScript::hash() return changed to ScriptHash #234

Merged
merged 3 commits into from
Nov 4, 2021

Conversation

rooooooooob
Copy link
Contributor

both Ed25519KeyHash and ScriptHash are 28-byte wrapper types with the
same API (both generated by impl_hash_type!()) around a blake2b224
hash and are CBOR-equivalent, but are different semantically.

both Ed25519KeyHash and ScriptHash are 28-byte wrapper types with the
same API (both generated by `impl_hash_type!()`) around a blake2b224
hash and are CBOR-equivalent, but are different semantically.
@@ -1615,11 +1615,11 @@ pub enum ScriptHashNamespace {

#[wasm_bindgen]
impl NativeScript {
pub fn hash(&self, namespace: ScriptHashNamespace) -> Ed25519KeyHash {
pub fn hash(&self, namespace: ScriptHashNamespace) -> ScriptHash {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think in #127 when Ashish copied the hashing code from the key hashing method the wrapper type just wasn't changed to KeyHash. They're binary equivalent anyway both defined like:

impl_hash_type!(Ed25519KeyHash, 28);
impl_hash_type!(ScriptHash, 28);


let script_hash = ScriptHash::from_bytes(
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although here we're doing the binary conversion explicitly in a test so it's weird that the type wouldn't have been changed to ScriptHash when writing that test.

@vsubhuman vsubhuman self-requested a review November 4, 2021 22:17
@vsubhuman vsubhuman added this to the 10.0.0 milestone Nov 4, 2021
@vsubhuman vsubhuman merged commit 78b6421 into Emurgo:master Nov 4, 2021
@vsubhuman vsubhuman mentioned this pull request Feb 6, 2022
40 tasks
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 this pull request may close these issues.

2 participants