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

feat: add native-script verify function #332

Merged
merged 5 commits into from
Jun 11, 2024

Conversation

hadelive
Copy link
Contributor

No description provided.

@hadelive hadelive closed this Jun 10, 2024
@hadelive hadelive deleted the add-verify-native-script branch June 10, 2024 15:53
@hadelive hadelive restored the add-verify-native-script branch June 10, 2024 15:53
@hadelive hadelive deleted the add-verify-native-script branch June 10, 2024 15:55
@hadelive hadelive restored the add-verify-native-script branch June 10, 2024 16:00
@hadelive hadelive reopened this Jun 10, 2024
Copy link
Contributor

@SebastienGllmt SebastienGllmt left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@rooooooooob rooooooooob left a comment

Choose a reason for hiding this comment

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

Two things:

  1. We put these kinds of things in utils.rs files instead of mod.rs. The mod.rs we try and keep as auto-generated as possible (outside of module declarations and some imports). That way it doesn't get overwritten when we need to regenerate something when they update the cddl definitions.

  2. This should be in the chain/rust crate and then a simple binding over it for the chian/wasm code so that way it's exposed for rust projects too.

Copy link
Contributor

@rooooooooob rooooooooob left a comment

Choose a reason for hiding this comment

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

The rust part looks good. Do we want to put the wasm bindings for that now in chain/wasm/utils.rs?

@hadelive
Copy link
Contributor Author

The rust part looks good. Do we want to put the wasm bindings for that now in chain/wasm/utils.rs?

Yes

@@ -68,6 +69,61 @@ impl NativeScript {
pub fn hash(&self) -> ScriptHash {
self.0.hash().into()
}

pub fn verify(
Copy link
Contributor

Choose a reason for hiding this comment

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

I meant just a wrapper around the rust code to avoid duplicating logic e.g.:

self.0.verify(lower_bound, upper_bound, key_hashes.as_ref())

^ untested but it's something like that

We have AsRef and Into/From to convert to/from the rust and WASM structs.

@rooooooooob
Copy link
Contributor

The checks fail with a clippy warning. Once that is fixed and the check passes I'll LGTM and merge it. Thanks.

Copy link
Contributor

@rooooooooob rooooooooob left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for the contribution!

@rooooooooob rooooooooob merged commit 4299c23 into dcSpark:develop Jun 11, 2024
1 check passed
@hadelive hadelive deleted the add-verify-native-script branch June 11, 2024 19:52
@hadelive
Copy link
Contributor Author

How long does it take for the new release?

rooooooooob pushed a commit that referenced this pull request Jul 23, 2024
* feat: add native-script verify function
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.

3 participants