Add `miden-package` crate with `Package` type to represent a compiled Miden program/library. #1544

greenhat · 2024-10-23T13:23:46Z

This PR adds miden-package crate with Package type ported from the compiler's ad-hoc implementation to represent a compiled Miden program/library.

Please consider reviewing this PR commit-by-commit. I crafted the commits to be focused on a single change and show the evolution of the implementation.

The reasons for the new crate are explained in 0xPolygonMiden/compiler#376

The compiler's PR that uses miden-package from this PR is 0xPolygonMiden/compiler#349

TODO:

branch out a new branch for the compiler to ship on the pre-EAM(element addressable memory) VM
rebase;
address review notes;

Co-authored-by: Philippe Laferrière <plafer@protonmail.com>

bobbinth

Looks good! Thank you! I left some comments inline. The main ones are about:

Potentially adding some validation to library path deserialization.
Using Serializable/Deserializable instead of serde.
Not including functionality that assumes downstream crates (i.e., miden-base).

package/Cargo.toml

package/README.md

bobbinth · 2025-01-17T00:58:23Z

package/Cargo.toml

@@ -0,0 +1,31 @@
+[package]
+name = "miden-package"


I'm wondering if we should make it a bit more specific. Maybe miden-vm-package or miden-code-package (though, not sure if I like these better than just miden-package).

I got so much used to miden-package so I'm biased here. :) In general, if we plan to have another "packages" (account?) giving it a more specific name makes sense.

What do you think about miden-mast-package here? I would also maybe expand the description a little bit to make it more specific.

package/src/dep/mod.rs

Cargo.toml

greenhat · 2025-01-17T11:17:18Z

@bobbinth @plafer Thank you for the review! I've addressed all notes, implemented easy ones, and I am converting this PR to draft to work on:

Switch PackageExport::name to have QualifiedProcedureName type and propagate Arbitrary implementation for proptest;
Remove bitcode and serde based serialization in favor of Serializable/Deserializable;
New validation for ids (LibraryPath, ProcedureName) on deserialization;

instead of `bitcode` and `serde`

Implement `Serializable/Deserializable` and `Arbitrary`(proptest) for `QualifiedProcedureName`. Add serialization roundtrip tests for `LibraryPath` and `ProcedureName`.

bobbinth

Looks good! Thank you! I left some more comments/questions inline.

Besides these, I think the main thing remaining here is potentially adding some validation to library path deserialization - right?

package/src/lib.rs

package/src/package.rs

package/src/tests.rs

package/src/package.rs

bobbinth · 2025-01-21T08:17:42Z

package/src/package.rs

+pub struct Package {
+    /// Name of the package
+    pub name: String,
+    /// The MAST artifact ([Program] or [Library]) of the package
+    pub mast: MastArtifact,
+    /// The package manifest, containing the set of exported procedures and their signatures,
+    /// if known.
+    pub manifest: PackageManifest,
+}


Probably not for this PR, but where would we put custom metadata? Here, I'm thinking about the storage layout metadata we'd need to instantiate account components.

I'm thinking, either directly in Package or in PackageManifest.

Let's create an issue for this.

constructors in deserialization.

…ode cleanup

`package/mod.rs`

greenhat · 2025-01-22T06:51:27Z

@bobbinth @plafer I implemented everything and addressed all your notes. Please do another round.

plafer

LGTM!

bobbinth

Thank you! Looks good! I left some more comments inline - most are pretty minor.

bobbinth · 2025-01-25T21:45:56Z

package/src/package/serialization/mod.rs

+            Library::read_from(source).map(Arc::new).map(MastArtifact::Library)
+        } else {
+            Err(DeserializationError::InvalidValue(format!(
+                "Invalid MAST artifact tag: {:?}",


nit: we usually don't capitalize error the first letter of error messages (i.e., "Invalid" -> "invalid").

bobbinth · 2025-01-25T21:50:43Z

package/src/package/mod.rs

+/// A procedure exported by a package, along with its digest and
+/// signature(will be added after MASM type attributes are implemented).


nit: there should be a space after "signature". Also, we usually do newlines after about 100 chars. So, this comment should look like:

/// A procedure exported by a package, along with its digest and signature (will be added after /// MASM type attributes are implemented).

bobbinth · 2025-01-25T21:57:31Z

package/src/lib.rs

+#[cfg(test)]
+extern crate std;
+
+pub use vm_core::{chiplets::hasher::Digest, mast::MastForest, Program};


I would also re-export Library, QualifiedProcedureName, LibraryPath, and maybe ProcedureName as they are also used in the public interface (these would come from the assembly crate though).

bobbinth · 2025-01-25T22:08:01Z

assembly/src/ast/procedure/name.rs

+            Some((_, c))
+                if c.is_ascii_lowercase() || c == '_' || c == '-' || c == '$' || c == '.' =>
+            {


Not for this PR, but the code in this function is a bit difficult to follow. Let's create an issue (could be a "good first issue") to make the code more readable.

bobbinth · 2025-01-25T22:26:52Z

assembly/src/library/path.rs

@@ -505,7 +505,23 @@ impl Deserializable for LibraryPath {
        let path = source.read_slice(len)?;
        let path =
            str::from_utf8(path).map_err(|e| DeserializationError::InvalidValue(e.to_string()))?;
-        Self::new(path).map_err(|e| DeserializationError::InvalidValue(e.to_string()))
+        LibraryPath::new(path).or_else(|_| {
+            // Try to parse at least the namespace


Could we expand the comment a bit here to explain what the code below is intending to cover? My understanding is:

If path is not a valid LibraryPath, we try to take the first "leg" of the path and use it as a namespace and the rest of the string as the module name. And if there is no first "leg", then we use Anon namespace and try to use the whole path as the module name.

If the above is correct, it is not clear to me what exactly we are trying to enable here. It seems like maybe we want to make sure that a path with no namespaces get Anon namespace? If so, could we simplify the code as:

LibraryPath::new(path).or_else(|_| { let module_id = Ident::new(path).map_err(|e| { DeserializationError::InvalidValue(format!("Invalid module id: {}", e)) })?; Ok(LibraryPath::new_from_components(LibraryNamespace::Anon, [module_id])) }

bobbinth · 2025-01-25T22:28:25Z

package/Cargo.toml

+version = "0.11.0"
+description = "Miden VM package"
+documentation = "https://docs.rs/miden-package/0.11.0"


Let's increment the versions to v0.12.0.

bobbinth · 2025-01-25T22:30:07Z

package/Cargo.toml

@@ -0,0 +1,31 @@
+[package]
+name = "miden-package"


What do you think about miden-mast-package here? I would also maybe expand the description a little bit to make it more specific.

bobbinth · 2025-01-25T22:31:44Z

Let's also resolve the merge conflicts.

greenhat changed the title ~~Ass miden-package crate with Package type to represent a compiled Miden program/library.~~ Add miden-package crate with Package type to represent a compiled Miden program/library. Oct 23, 2024

greenhat force-pushed the greenhat/miden-package branch 4 times, most recently from 76a4b7e to 9ff342e Compare October 30, 2024 09:44

greenhat mentioned this pull request Oct 30, 2024

fix: use unchecked constructor for ProcedureName on Library deserialization #1554

Merged

greenhat force-pushed the greenhat/miden-package branch from 9ff342e to 7c9b95b Compare October 30, 2024 10:14

greenhat changed the base branch from next to greenhat/library-deser-uncheck-exports October 30, 2024 10:15

greenhat force-pushed the greenhat/library-deser-uncheck-exports branch from 1ca8581 to 3581727 Compare November 4, 2024 17:37

Base automatically changed from greenhat/library-deser-uncheck-exports to next November 4, 2024 21:37

greenhat force-pushed the greenhat/miden-package branch 7 times, most recently from 41b5f5d to c5ff6f0 Compare November 12, 2024 14:07

greenhat mentioned this pull request Nov 12, 2024

Support ability to specify advice data via MASM #1547

Open

greenhat force-pushed the greenhat/miden-package branch 6 times, most recently from 211f34c to 84ea102 Compare November 19, 2024 08:49

greenhat changed the base branch from next to greenhat/i1547-mastforest-add-advicemap November 19, 2024 08:49

greenhat changed the title ~~Add miden-package crate with Package type to represent a compiled Miden program/library.~~ [2/2] Add miden-package crate with Package type to represent a compiled Miden program/library. Nov 19, 2024

greenhat force-pushed the greenhat/i1547-mastforest-add-advicemap branch from eda4a01 to 006e03a Compare November 19, 2024 14:40

greenhat force-pushed the greenhat/miden-package branch 2 times, most recently from 5ec902e to d6419ca Compare November 19, 2024 15:00

greenhat force-pushed the greenhat/i1547-mastforest-add-advicemap branch from 006e03a to 92f3309 Compare November 20, 2024 07:05

greenhat and others added 8 commits January 16, 2025 12:53

docs: wording

4d031dd

Co-authored-by: Philippe Laferrière <plafer@protonmail.com>

docs: formatting

15dfc12

Co-authored-by: Philippe Laferrière <plafer@protonmail.com>

docs: formatting

ae31949

Co-authored-by: Philippe Laferrière <plafer@protonmail.com>

docs: formatting

2d65242

Co-authored-by: Philippe Laferrière <plafer@protonmail.com>

docs: wording

cc88636

Co-authored-by: Philippe Laferrière <plafer@protonmail.com>

docs: wording

86d8974

Co-authored-by: Philippe Laferrière <plafer@protonmail.com>

chore: fix clippy, formatting, cleanup comments

7e37745

refactor: rename types for resolved dependency for the Package

43e0a83

bobbinth reviewed Jan 17, 2025

View reviewed changes

greenhat added 2 commits January 17, 2025 10:44

refactor: remove SystemLibraryId and use DependencyName(String)

ab26b55

chore: add license in package/README.md and fix docs.rs version in

ad3e722

Cargo.toml

greenhat marked this pull request as draft January 17, 2025 11:17

greenhat added 3 commits January 20, 2025 18:07

refactor: serialize Package with Serializable/Deserializable impls

3cd2d1b

instead of `bitcode` and `serde`

refactor: make PackageExport::name to be QualifiedProcedureName,

72b700e

Implement `Serializable/Deserializable` and `Arbitrary`(proptest) for `QualifiedProcedureName`. Add serialization roundtrip tests for `LibraryPath` and `ProcedureName`.

chore: formatting

a93f3a2

greenhat force-pushed the greenhat/miden-package branch from 915c093 to a93f3a2 Compare January 20, 2025 16:07

chore: fix no_std build

8b60a31

bobbinth reviewed Jan 21, 2025

View reviewed changes

fix: relax Ident and ProcedureName validation, use checked

37fe048

constructors in deserialization.

greenhat force-pushed the greenhat/miden-package branch from 223c95f to 37fe048 Compare January 22, 2025 05:17

greenhat added 2 commits January 22, 2025 08:02

chore: export Digest, MastForest, Program in miden-package, c…

e3fa250

…ode cleanup

refactor: move tests to serialization module, rename package.rs to

a1b44a7

`package/mod.rs`

greenhat mentioned this pull request Jan 22, 2025

chore: update the Miden VM (with updated miden-package crate) 0xPolygonMiden/compiler#376

Draft

greenhat marked this pull request as ready for review January 22, 2025 06:51

plafer approved these changes Jan 22, 2025

View reviewed changes

bobbinth reviewed Jan 25, 2025

View reviewed changes

greenhat mentioned this pull request Feb 5, 2025

[3/x] frontend: translate BrTable Wasm op 0xPolygonMiden/compiler#388

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `miden-package` crate with `Package` type to represent a compiled Miden program/library. #1544

Add `miden-package` crate with `Package` type to represent a compiled Miden program/library. #1544

greenhat commented Oct 23, 2024 •

edited

Loading

bobbinth left a comment

bobbinth Jan 17, 2025

greenhat Jan 17, 2025

bobbinth Jan 25, 2025

greenhat commented Jan 17, 2025

bobbinth left a comment

bobbinth Jan 21, 2025

greenhat Jan 21, 2025

bobbinth Jan 25, 2025

greenhat commented Jan 22, 2025

plafer left a comment

bobbinth left a comment

bobbinth Jan 25, 2025

bobbinth Jan 25, 2025

bobbinth Jan 25, 2025

bobbinth Jan 25, 2025

bobbinth Jan 25, 2025

bobbinth Jan 25, 2025

bobbinth Jan 25, 2025

bobbinth commented Jan 25, 2025

		/// A procedure exported by a package, along with its digest and
		/// signature(will be added after MASM type attributes are implemented).

Add miden-package crate with Package type to represent a compiled Miden program/library. #1544

Are you sure you want to change the base?

Add miden-package crate with Package type to represent a compiled Miden program/library. #1544

Conversation

greenhat commented Oct 23, 2024 • edited Loading

bobbinth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

greenhat commented Jan 17, 2025

bobbinth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

greenhat commented Jan 22, 2025

plafer left a comment

Choose a reason for hiding this comment

bobbinth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bobbinth commented Jan 25, 2025

Add `miden-package` crate with `Package` type to represent a compiled Miden program/library. #1544

Add `miden-package` crate with `Package` type to represent a compiled Miden program/library. #1544

greenhat commented Oct 23, 2024 •

edited

Loading