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

[7/n][vm-rewrite][sui-execution] Update adapter #21071

Open
wants to merge 1 commit into
base: tzakian/vm-rewrite-adapter-6
Choose a base branch
from

Conversation

tzakian
Copy link
Contributor

@tzakian tzakian commented Feb 3, 2025

This is the big one...

There is still some refactoring and changes need, but broad-strokes this plumbs in the new VM to the adapter.

The main conceptual change is the idea of a LinkedContext which is an execution context with a specific linkage and VM instance associated with it. All commands currently require a LinkedContext.

On the TODO list (but for other PRs) is:

  1. To relax the need for a LinkedContext for Upgrade and Publish commands to require a context a priori (however there are some mechanics that will need to be sorted out to make this work).
  2. Properly handle type versioning/defining IDs for typetags/type resolution.
  3. General cleanup.

@tzakian tzakian requested a review from cgswords February 3, 2025 21:11
Copy link

vercel bot commented Feb 3, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
sui-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Feb 7, 2025 8:19pm
2 Skipped Deployments
Name Status Preview Comments Updated (UTC)
multisig-toolkit ⬜️ Ignored (Inspect) Visit Preview Feb 7, 2025 8:19pm
sui-kiosk ⬜️ Ignored (Inspect) Visit Preview Feb 7, 2025 8:19pm

@tzakian tzakian temporarily deployed to sui-typescript-aws-kms-test-env February 3, 2025 21:11 — with GitHub Actions Inactive
@tzakian tzakian force-pushed the tzakian/vm-rewrite-adapter-6 branch from 489397b to 53ce0a8 Compare February 3, 2025 21:22
@tzakian tzakian force-pushed the tzakian/vm-rewrite-adapter-7 branch from 4dde691 to 3c7de3c Compare February 3, 2025 21:22
@tzakian tzakian temporarily deployed to sui-typescript-aws-kms-test-env February 3, 2025 21:22 — with GitHub Actions Inactive
@tzakian tzakian force-pushed the tzakian/vm-rewrite-adapter-6 branch from 53ce0a8 to 04ed3db Compare February 4, 2025 17:44
@tzakian tzakian requested a review from ronny-mysten as a code owner February 4, 2025 17:44
@tzakian tzakian force-pushed the tzakian/vm-rewrite-adapter-7 branch from 3c7de3c to eee4b44 Compare February 4, 2025 17:44
@tzakian tzakian temporarily deployed to sui-typescript-aws-kms-test-env February 4, 2025 17:45 — with GitHub Actions Inactive
@vercel vercel bot temporarily deployed to Preview – sui-docs February 4, 2025 17:55 Inactive
@tzakian tzakian force-pushed the tzakian/vm-rewrite-adapter-6 branch from 04ed3db to 89b34cf Compare February 4, 2025 20:30
@tzakian tzakian force-pushed the tzakian/vm-rewrite-adapter-7 branch from eee4b44 to 6942570 Compare February 4, 2025 20:30
@tzakian tzakian temporarily deployed to sui-typescript-aws-kms-test-env February 4, 2025 20:32 — with GitHub Actions Inactive
@tzakian tzakian force-pushed the tzakian/vm-rewrite-adapter-6 branch from 89b34cf to 208bdfc Compare February 5, 2025 20:10
@tzakian tzakian force-pushed the tzakian/vm-rewrite-adapter-7 branch from 6942570 to 4bd62dc Compare February 5, 2025 20:11
@tzakian tzakian temporarily deployed to sui-typescript-aws-kms-test-env February 5, 2025 20:12 — with GitHub Actions Inactive
@tzakian tzakian requested a review from tnowacki February 5, 2025 20:45
This is the big one...

There is still some refactoring and changes need, but broad-strokes this
plumbs in the new VM to the adapter.

The main conceptual change is the idea of a `LinkedContext` which is an
execution context with a specific linkage and VM instance associated
with it. All commands currently require a `LinkedContext`.

On the TODO list is:
1. To relax the need for a `LinkedContext` for `Upgrade` and `Publish`
   commands to require a context a priori (however there are some
   mechanics that will need to be sorted out to make this work).
2. Properly handle type versioning/defining IDs for typetags/type
   resolution.
3. General cleanup.

NB: The code in the PR may not be working as future PRs will build on top of
this.
/// Newly published packages
new_packages: Vec<MovePackage>,
pub new_packages: Vec<MovePackage>,
Copy link
Contributor

Choose a reason for hiding this comment

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

Please no pub here. Bad things can happen if this is pub--very delicate code

Comment on lines +128 to +129
/// A "reverse" linkage of storage_id -> runtime_id
pub reverse_linkage: BTreeMap<ObjectID, ObjectID>,
Copy link
Contributor

Choose a reason for hiding this comment

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

Why isn't this a part of ResolvedLinkage?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No particular reason, I think adding in to that would make sense to me.

)?;
let reverse_linkage = linkage
.iter()
.map(|(k, v)| (*v, *k))
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this guaranteed to be bidirectional?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, we are guaranteed that every version of a package corresponds to exactly one original package id (i.e., package UUID).

Basically, if there were a conflict here it would mean that package A@v1 is somehow both a A@v1 and a B@v_something package which is impossible.

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.

2 participants