protocol: VM v0.22.1 compatibility fixes#2742
Conversation
djolertrk
force-pushed
the
main
branch
2 times, most recently
from
097e895 to
f6fb407
Compare
|
cc @bitwalker @greenhat -- but It looks like we have some problems with new licences from dependencies. |
|
We can definitely allow MPL-2.0 as a license, it just wasn't explicitly added as a supported license before. |
What are the implications of this? Can a project that has a dependency with an MLP license be licensed under MIT? Or do we need to change licenses here somehow? |
|
Also, I somehow missed that |
I would still want this 0xMiden/miden-client#1959 to be integrated into the |
I'm going to try to make a PR in |
bobbinth
requested review from
bobbinth,
huitseeker,
igamigo and
mmagician
and removed request for
bobbinth
bobbinth
left a comment
bobbinth
left a comment
There was a problem hiding this comment.
Looks good! Thank you! I wasn't able to find a way to patch v0.22.1 such that it contains non-breaking changes. So, we'll need to proceed with this PR and yank all v0.22.0 VM and protocol/node crates that rely on it.
I also left a comment for the future - but that's more relevant for the miden-vm repo.
| let package_with_metadata = Package { | ||
| name: "test_package".to_string(), | ||
| mast: MastArtifact::Library(Arc::new(library.clone())), | ||
| manifest: PackageManifest::new(None), | ||
| kind: PackageKind::AccountComponent, | ||
| name: "test_package".into(), | ||
| mast: library.clone(), | ||
| manifest: PackageManifest::new(core::iter::empty()).unwrap(), | ||
| kind: TargetType::AccountComponent, | ||
| sections: vec![Section::new( | ||
| SectionId::ACCOUNT_COMPONENT_METADATA, | ||
| metadata_bytes.clone(), | ||
| )], | ||
| version: Default::default(), | ||
| version: Version::new(0, 0, 0), | ||
| description: None, | ||
| }; |
There was a problem hiding this comment.
Not for this PR, but looking at this now, I think we may want to make all fields in the Package private and expose them via accessors - otherwise, it may be too easy to construct invalid packages.
Also, renaming from PackageKind to TargetType feels a bit off. So, we should probably consider renaming it back. cc @bitwalker.
There was a problem hiding this comment.
LGTM overall (low context on protocol — so low confidence on semantics, but the rust changes do seem minimally invasive).
Checked MPL change, which LGTM (minimal, file-level copyleft). Checked node does not use MastArtifact/PackageKind and does not build TargetType::Executable packages (or packages of any kind).
Left inline note on versioning.
| ProcedureExport, | ||
| Section, | ||
| SectionId, | ||
| TargetType, |
There was a problem hiding this comment.
This looks like a source-breaking API change for the current 0.14.x line. Technically, downstream code importing MastArtifact or PackageKind from miden_protocol::vm will stop compiling here unless we release at a major version.
4 participants
Update miden-protocol and miden-standards for VM v0.22.1 compatibility:
enum