Skip to content

feat: into_package() for ProtocolLib and StandardLib + CI Artifact#2859

Open
lima-limon-inc wants to merge 8 commits into
0xMiden:nextfrom
lambdaclass:fabrizioorsi/libs-as-masps
Open

feat: into_package() for ProtocolLib and StandardLib + CI Artifact#2859
lima-limon-inc wants to merge 8 commits into
0xMiden:nextfrom
lambdaclass:fabrizioorsi/libs-as-masps

Conversation

@lima-limon-inc
Copy link
Copy Markdown
Contributor

@lima-limon-inc lima-limon-inc commented Apr 30, 2026

Closes 0xMiden/midenup#200
Blocked-by #2896

This PR is part of an ongoing effort to migrate serialization towards the .masp Package format exclusively (see 0xMiden/midenup#191 (comment)).

A new into_package() method for both ProtocolLib and StandardLib was added which directly returns the corresponding Package.
This new method is then used in generate_package.rs script, which publishs the masp artifact onto the Github release page.

Notes:

@lima-limon-inc lima-limon-inc force-pushed the fabrizioorsi/libs-as-masps branch from a9148d7 to 86f8f70 Compare April 30, 2026 21:10
@bobbinth bobbinth requested review from huitseeker and mmagician May 1, 2026 01:26
Copy link
Copy Markdown
Collaborator

@mmagician mmagician left a comment

Choose a reason for hiding this comment

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

Overall looks good, but there are two items I'd still like double checked:

  • the CI should follow whatever patterns 0xMiden/miden-vm#3029 establishes, it looks like there are a lot of useful comments about the upload-artifcats job there which will be relevant in protocol repo, too
  • verify whether only ProtocolLib or StandardsLib needs to be uploaded as per my comments

Comment thread scripts/generate-package.rs
Comment thread .github/workflows/workspace-publish.yml Outdated
Comment thread crates/miden-standards/src/standards_lib.rs Outdated
Comment thread .github/workflows/workspace-publish.yml Outdated

/// Wraps this library into a [`Package`] named `PROTOCOL_PACKAGE_NAME`,
/// versioned with the `miden-protocol` crate's version.
pub fn into_package(self) -> Box<Package> {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I wonder if there are any meaningful tests we could add for into_package()

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I think before this gets merged, we should switch to building the protocol and standards packages as Miden projects, i.e. via miden-project.toml. You can still obtain a Library from the resulting package for legacy uses, but it produces a proper package rather than trying to create one from a Library, which IMO we should not do.

That way this function goes away, and packaging uses the standard workflow and tooling provided by the assembler, and we're guaranteed that all of the package metadata is correct.

Copy link
Copy Markdown
Collaborator

@mmagician mmagician May 5, 2026

Choose a reason for hiding this comment

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

we should switch to building the protocol and standards packages as Miden projects, i.e. via miden-project.toml

Would this need to happen here in the protocol repo? I haven't followed up on the packages/projects work recently

Copy link
Copy Markdown
Contributor Author

@lima-limon-inc lima-limon-inc May 5, 2026

Choose a reason for hiding this comment

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

Would this need to happen here in the protocol repo? I haven't followed up on the packages/projects work recently

I believe so. I believe that would imply switching to a masm miden-project here on the protocol repo.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@mmagician Yes, the miden-project.toml would be defined for each logical workspace/project in this repo for which we want to produce a Miden package (we modeled the transaction kernel in the examples under miden-project in the VM repo, but the actual definition would look slightly different here). If you have specific questions about that, I'd be glad to hop on a call to discuss how to approach the project structure , or whatever is most convenient. I can take a stab at defining it too, but you guys know best how you intend to structure packages for things like standards.

Copy link
Copy Markdown
Collaborator

@mmagician mmagician May 6, 2026

Choose a reason for hiding this comment

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

@bitwalker regarding your suggestion to switch to using miden-project.toml to build packages, IIUC we have two options:

Option A - each dependency package with its own manifest:

  • crates/miden-protocol/asm/miden-project.toml would be a workspace with four members:
    • shared_utils,
    • kernels/transaction
    • protocol
    • asm/shared_modules/ is either pre-linked as currently, or maybe refactored to promote to a 4th workspace member.
  • crates/miden-standards/asm/miden-project.toml would be a workspace with a single standards member that depends on protocol above
  • build.rs in both crates would be rewritten to call Assembler::link_package
    instead of assembling kernel/library imperatively. I think this is a positive change and will result in less boilerplate code for building.

Option B - only the userspace libraries (protocol & standards) get a manifest:

  • crates/miden-protocol/asm/protocol/miden-project.toml and
    crates/miden-standards/asm/standards/miden-project.toml, both single-
    package manifests
  • Kernel, shared_utils, etc. stay assembled via build.rs as today and only the final protocol/standards libraries are produced via link_package.

LMK if this understanding is correct. AFAIU, for option B - if feasible - is simpler and would be a good start already.
But maybe long-term, we could still implement A.
Am I making sense? (as I said I'm only catching up on the package/project concepts)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The way I was imagining this was roughly like this:

Protocol

We'd have a single crates/miden-protocol/asm/miden-project.toml which would result in the following packages:

  • Transaction kernel library - i.e., result of assembling asm/kernel/transaction/api.masm.
  • Transaction kernel executable - i.e., result of assembling asm/kernel/transaction/main.masm.
  • Transaction script executable - i.e., result of assembling asm/kernel/transaction/tx_script_main.masm.
  • The user-facing protocol library - i.e., the result of assembling asm/protocol directory.

If we can't cover all of these with a single miden-project.toml file, we can split these up into separate projects.

Standards

  • We'd have a project file crates/miden-standards/asm/miden-project.toml to assembler the standards library.
  • We'll also have a project file for each of the components in asm/account_components directory (which we probably should rename to just asm/components.

I'll create an issue for this.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Created #2896.

Comment thread crates/miden-standards/src/standards_lib.rs Outdated
Comment thread .github/workflows/workspace-publish.yml Outdated
make packages
- name: Prepare artifacts
run: |
mv target/packages/miden-protocol.masp miden-protocol.masp
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

It looks like the last commit regressed the naming and we have the miden- prefix again

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

My bad, I've updated the name here: 81e2ac4

# Upload the pre-compiled artifacts (miden-protocol.masp) on the
# GitHub release page.
# Used by midenup to speed up installs.
upload-artifacts:
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This new job is missing runs-on, so GitHub Actions treats the workflow as invalid. actionlint .github/workflows/workspace-publish.yml reports this at the upload-artifacts job. Could this add e.g. runs-on: ubuntu-latest before steps?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good catch!

Added it here: dad9a43

Reported-by: François Garillot <francois@garillot.net>
@bobbinth
Copy link
Copy Markdown
Contributor

@lima-limon-inc - could you resolve the merge conflicts?

@lima-limon-inc
Copy link
Copy Markdown
Contributor Author

@lima-limon-inc - could you resolve the merge conflicts?

However, I'm not sure we want this to be merged as per @bitwalker's comments:

I think before this gets merged, we should switch to building the protocol and standards packages as Miden projects, i.e. via miden-project.toml.

I think tackling this PR as described in #2896 is the way to go.

Regardless, I have resolved the conflicts here, just in case, f727705.

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.

feat: add artifacts for miden-protocol

5 participants