-
Notifications
You must be signed in to change notification settings - Fork 79
refactor: swap Goldilocks re-export to the Felt type unified for off-chain and on-chain code
#819
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
base: main
Are you sure you want to change the base?
Changes from all commits
ba5aa2b
1519870
2340896
d852c32
06d007b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -15,12 +15,12 @@ pub mod ies; | |
| pub mod merkle; | ||
| pub mod rand; | ||
| pub mod utils; | ||
| pub mod word; | ||
|
|
||
| use miden_field::word; | ||
| // RE-EXPORTS | ||
| // ================================================================================================ | ||
| pub use p3_goldilocks::Goldilocks as Felt; | ||
| pub use word::{Word, WordError}; | ||
| pub use miden_field::Felt; | ||
| pub use miden_field::{Word, WordError}; | ||
|
Comment on lines
+22
to
+23
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: this could be a single line: pub use miden_field::{Felt, Word, WordError}; |
||
|
|
||
| pub mod field { | ||
| //! Traits and utilities for working with the Goldilocks finite field (i.e., | ||
|
|
@@ -140,7 +140,7 @@ pub type Set<V> = alloc::collections::BTreeSet<V>; | |
| // ================================================================================================ | ||
|
|
||
| /// Number of field elements in a word. | ||
| pub const WORD_SIZE: usize = 4; | ||
| pub const WORD_SIZE: usize = word::WORD_SIZE_FELT; | ||
|
|
||
| /// Field element representing ZERO in the Miden base filed. | ||
| pub const ZERO: Felt = Felt::ZERO; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,43 @@ | ||
| [package] | ||
| authors.workspace = true | ||
| categories.workspace = true | ||
| description = "A unified field element type for on-chain and off-chain Miden Rust code" | ||
| documentation = "https://docs.rs/miden-field" | ||
| edition.workspace = true | ||
| keywords.workspace = true | ||
| license.workspace = true | ||
| name = "miden-field" | ||
| readme = "../README.md" | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we should have a dedicated README file for this crate explaining (briefly) the motivation for it and how it works. |
||
| repository.workspace = true | ||
| rust-version.workspace = true | ||
| version.workspace = true | ||
|
|
||
| [lib] | ||
| crate-type = ["rlib"] | ||
|
|
||
| # dependendies for off-chain target | ||
| [target.'cfg(not(all(target_family = "wasm", miden)))'.dependencies] | ||
| miden-serde-utils = { workspace = true } | ||
| num-bigint = { default-features = false, version = "0.4" } | ||
| p3-challenger = { default-features = false, version = "0.4.2" } | ||
| p3-field = { default-features = false, version = "0.4.2" } | ||
| p3-goldilocks = { default-features = false, version = "0.4.2" } | ||
| paste = { version = "1.0.15" } | ||
| proptest = { default-features = false, features = ["alloc"], optional = true, version = "1.7" } | ||
| rand = { default-features = false, features = ["small_rng"], version = "0.9.0" } | ||
| serde = { default-features = false, features = ["derive"], version = "1.0" } | ||
|
|
||
| # dependendies for on-chain target | ||
| [dependencies] | ||
| thiserror = { default-features = false, version = "2.0" } | ||
|
Comment on lines
+30
to
+32
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure if the comment here is correct - I think these dependencies also apply for off chain targets. I would also move these above line 18. |
||
|
|
||
| [features] | ||
| default = [] | ||
| testing = ["dep:proptest"] | ||
|
|
||
| [dev-dependencies] | ||
| rand = { default-features = false, version = "0.9" } | ||
| rstest = { version = "0.26" } | ||
|
Comment on lines
+38
to
+40
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: formatting. |
||
|
|
||
| [lints] | ||
| workspace = true | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,13 @@ | ||
| use std::env; | ||
|
|
||
| fn main() { | ||
| println!("cargo::rerun-if-env-changed=MIDENC_TARGET_IS_MIDEN_VM"); | ||
| println!("cargo::rustc-check-cfg=cfg(miden)"); | ||
|
|
||
| // `cargo-miden` compiles Rust to Wasm which will then be compiled to Miden VM code by `midenc`. | ||
| // When targeting a "real" Wasm runtime (e.g. `wasm32-unknown-unknown` for a web SDK), we want a | ||
| // regular felt representation instead. | ||
| if env::var_os("MIDENC_TARGET_IS_MIDEN_VM").is_some() { | ||
| println!("cargo::rustc-cfg=miden"); | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,26 @@ | ||
| //! A unified `Felt` for Miden Rust code. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The new
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure adding tests for For native implementation, I think the main thing we'd be testing is that we didn't mix up delegated function calls. Maybe there is a way to add a couple of tests which would run all (most?) operations on some random elements and make sure we get the same result for |
||
| //! | ||
| //! This crate provides a single `Felt` type that can be used in both: | ||
| //! - On-chain (Wasm + `miden`): `Felt` is backed by a Miden VM felt via compiler intrinsics. | ||
| //! - Off-chain (native / non-Miden Wasm): `Felt` is backed by Plonky3's Goldilocks field element. | ||
|
|
||
| #![no_std] | ||
| #![deny(warnings)] | ||
|
|
||
| extern crate alloc; | ||
|
|
||
| #[cfg(all(target_family = "wasm", miden))] | ||
| mod wasm_miden; | ||
| #[cfg(all(target_family = "wasm", miden))] | ||
| pub use wasm_miden::Felt; | ||
|
|
||
| #[cfg(not(all(target_family = "wasm", miden)))] | ||
| mod native; | ||
| #[cfg(not(all(target_family = "wasm", miden)))] | ||
| pub use native::Felt; | ||
|
|
||
| pub mod utils; | ||
|
|
||
| pub mod word; | ||
|
|
||
| pub use word::{Word, WordError}; | ||
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.
Could we not use
miden-field.workspace = truehere?Also, since we now have more than a couple of workspace dependencies, could we move them into a separate section? Similar to how we do it here.