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

Provide a Rustier wrapper for zcash_script #171

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

sellout
Copy link

@sellout sellout commented Aug 8, 2024

This adds a Script trait that exposes slightly Rustier types in order to have a common interface for the existing C++ implementation as well as the upcoming Rust implementation (and a third instance that runs both and checks that the Rust result matches the C++ one).

@sellout
Copy link
Author

sellout commented Aug 8, 2024

ZcashFoundation/zebra#8751 integrates this with zebrad, and shouldn’t be merged until we know that that relationship is working (which it is currently not).

Have lib.rs re-export them, but make room for the upcoming Rust implementation.
This adds a `Script` trait that exposes slightly Rustier types in order
to have a common interface for the existing C++ implementation as well
as the upcoming Rust implementation (and a third instance that runs both
and checks that the Rust result matches the C++ one).

The module structure (interpreter.rs, zcash_script.rs) and locations of
definitions are intended to mirror the structure of the C++ code, especially as
we get the Rust implementation in place, for easier comparison. That
organization is very likely to change once everything has been checked.
Cargo.toml Outdated Show resolved Hide resolved
src/interpreter.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@nuttycom nuttycom left a comment

Choose a reason for hiding this comment

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

Flushing a few initial comments.

Cargo.toml Outdated Show resolved Hide resolved
src/interpreter.rs Show resolved Hide resolved
src/zcash_script.rs Show resolved Hide resolved
src/zcash_script.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/zcash_script.rs Outdated Show resolved Hide resolved
@sellout sellout marked this pull request as ready for review August 30, 2024 20:09
@sellout
Copy link
Author

sellout commented Aug 30, 2024

The changes in ZcashFoundation/zebra#8751 now work correctly with this, so we’re good for final review.

src/lib.rs Show resolved Hide resolved
Copy link
Contributor

@arya2 arya2 left a comment

Choose a reason for hiding this comment

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

This is looking great, thank you for making these changes.

I still need to review this in-depth, but, at a glance, I don't see any blockers.

src/interpreter.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@conradoplg conradoplg left a comment

Choose a reason for hiding this comment

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

It looks good, thanks! I think we just need to look a bit more into whether passing a Rust function pointer across FFI is safe.

src/zcash_script.rs Outdated Show resolved Hide resolved
src/lib.rs Show resolved Hide resolved
src/cxx.rs Outdated Show resolved Hide resolved
This should fix the Windows build.
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.

4 participants