Make it possible to build against on-chain dependencies #14178
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
3 Ignored Deployments
|
There was a problem hiding this comment.
High-level: Thinking about how we can chop this out to make iterative progress, it looks like quite a few commits could be factored out and merged cleanly on their own as preparation for the core changes--especially the ones propagating info for error messages and code shuffling (54a97f3). So, re: discussion points:
(1) & (2) I don't have good recommendations on these yet, will chew on it a bit more.
(3) I think this is a separable problem + solution that could work nicely, ando go in a separate PR for prep
(4) Seems reasonable--the default expectation for devs right now is to compile against source, so that is always an available fallback while we're working towards a v0 for compiling against on-chain deps. I.e., perf considerations can be a fast follow
(5) I think this is OK--we need to take care of removing references from docs / code in the Sui repo and also communicate the change in Changelogs, etc. though. Emitting a warning instead of a hard error in a next release would be a bit more user-friendly, followed by a hard error in a subsequent release.
(6) Sounds good--these renames seem like another set of candidate changes that would be great to separate and merge before the core functional changes.
To summarize, I wonder if we can create separate PR(s) for the mechanical changes required, with the core changes following up. My sense is that might make it easier to think / reason about the core code changes and dealing with (1) and (2), and also to give room for input from @amnn.
| let (src_deps, bytecode_deps): (Vec<_>, Vec<_>) = deps_package_paths | ||
| .clone() | ||
| .into_iter() | ||
| .partition_map(|(p, b)| if b { Either::Left(p) } else { Either::Right(p) }); | ||
| // If bytecode dependency is not empty, do not allow renaming | ||
| if !bytecode_deps.is_empty() { | ||
| if let Some(pkg_name) = resolution_graph.contains_renaming() { | ||
| anyhow::bail!( | ||
| "Found address renaming in package '{}' when \ | ||
| building with bytecode dependencies -- this is currently not supported", | ||
| pkg_name | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
High-level comment: Any thoughts on introducing an enum / variant to distinguish between source and bytecode dependencies? E.g., something like Source/LocalSource and Bytecode/OnChainBytecode dependencies?
Conceptually I'd love if we can discriminate at the type-level, though I'm not sure how this trickles down into the implementation changes--I think you have a better intuition on that at this point.
There was a problem hiding this comment.
This commit in particular I have cherry picked from upstream so I'm also not super familiar with details here. I'd rather leave it as is and perhaps do this change some other time.
| )>, | ||
| ) -> Result<( | ||
| /* sources */ PackagePaths, | ||
| /* deps */ Vec<PackagePaths>, | ||
| /* deps */ Vec<(PackagePaths, bool)>, |
There was a problem hiding this comment.
Ditto for: Thoughts on using an enum type to discriminate paths/dependencies to replace something like src_flag : bool?
| @@ -128,7 +127,7 @@ impl PartialEq for Package { | |||
| // comparing packages during insertion of externally resolved ones - an internally resolved | |||
| // existing package in the graph would not be recognized as a potential different version of | |||
| // the externally resolved one) | |||
| self.kind == other.kind && self.version == other.version | |||
There was a problem hiding this comment.
High-level comment on this commit: I'm a big fan of putting up this PR separately before everything else since it's clean / self-contained. We need to cover our bases with how this affects users / manifests that contain version. I.e., great if it's vestigial and we can conceptually remove it. To go along with that change, for example, we need to remove references to version in the sui repo in examples and elsewhere: https://sourcegraph.com/search?q=context:global+repo:%5Egithub%5C.com/+file:Move%5C.toml+version+-repo:diem+-repo:aptos&patternType=standard&sm=1&groupBy=repo and communicate the change in the changelog
There was a problem hiding this comment.
To go along with that change, for example, we need to remove references to version in the sui repo in examples and elsewhere
As far as I'm concerned we can do that but we need to consider is that old versions of the tool will then error when building packages against master because version field is mandatory.
| pub(crate) fn custom_resolve_pkg_name( | ||
| pkg_path: &PathBuf, | ||
| manifest: &SourceManifest, | ||
| ) -> anyhow::Result<Symbol> { | ||
| if let Some(hooks) = &*HOOKS.lock().unwrap() { | ||
| hooks.custom_resolve_pkg_name(pkg_path, manifest) | ||
| } else { | ||
| Ok(manifest.package.name) | ||
| } | ||
| } |
| /// Original dependency name as defined in parent manifest since it can be different from the | ||
| /// resolved name. Used for printing user-friendly error messages. | ||
| pub dep_orig_name: PM::PackageName, |
There was a problem hiding this comment.
This commit looks like something great we can ship outright as preparation for compiling against on-chain dependencies, i.e., in separate PR. That would help reduce the footprint / size of the core feature and PR at the end!
There was a problem hiding this comment.
This issue only happens when custom_resolve_pkg_name hook is used and returns something that's different than the manifest so it might not make sense to ship it before the hook code (that PR would be confusing).
It should be possible to merge each of these commits on their own. A cool thing is that everything is fully backwards compatible until custom Sui hooks get implemented. But I think everything should probably be commited within the same version cutoff and some consideration be taken about the rollout (because e.g. of (2), (3), (5)). The downside of splitting up the PR is that there is some overhead in getting indiviual PRs to be mergable, but I can do that if you think this will make the overall process easier. Here's how I think commits can be groupped up into separate PRs: 1 2 3 4 5 6 7 8 9 10 These can be amended to some existing commits: |
|
I've added unresolved issue / discussion point (7). |
|
Thanks for your patience!
So I'd love if we could tackle this bit-by-bit and make it a bit more digestible for review; I do think we'll need to do that to make progress here (reviewing it all is a bit intimidating for me to review personally 😅) and the codebase can shift and cause conflicts (which looks like already happened).
That is great. I think it's reasonable to bundle if it doesn't have user-visible impact and turns out to be relatively small (something like less than 1KLOC change. this isn't a hard and fast rule, I'm just trying to give an idea). Considering the user-visible impact of (2), that might not be the case. Side question: do we strictly need (2) to enable on-chain dependencies? If not, we might consider punting on this so it's not blocking the actual functionality.
I agree with the convenience but it might be a bit difficult to pull off in practice, I think we have some options here:
I'm not very confident about a big PR with separate commits, because it makes review very difficult to isolate and incorporate changes that come with feedback. I'm also curious, to what extent you think some of the discussion points could be helped by having a feature flag on the implementation. E.g., to explicitly warn on known limitations. Let me know your thoughts! |
Yes, it is stricly a problem to enable on-chain deps because once you enable the custom hooks, the "package name" gets overwritten with its "original-id" hence the inconsistency. Not a problem though untill the on-chain resolution hooks get implemetned.
I don't think even this will be necessary because everything's backwards compatible until you add the on-chain resolution hooks. Except perhaps the version field removal which will throw "unrecognized field in manifest" warnings, but even then we can get rid of those by registering "version" as a custom field with the hook (similar to "published-at").
I'm not a fan of this approach because you still then need to merge the base PR into main which will require another look / review / rebase. Kinda defeats the purpose of creating multiple PRs.
(1) - if you do an appropriate override it will fix the problem (1) , (3), (4), (6) are only a problem when user has on-chain deps in manifest / graph @rvantonder @amnn I'm happy to start publishing separate PRs for this, but I'd like to first have some general plan or agreement on how we're going to go about these unresolved issues, because I'd like to avoid the situation where we have to change the code landed in these PRs after the fact based on how we're going to solve these. |
|
Thanks for the detailed breakdown, that's really helpful and clear! About resolving outstanding issues, like
Yep, makes sense! |
I agree with this. Something with bad UX is still better than nothing. I'm actually using the toolchain from this PR to build a package with on-chain deps, and although it's a bit inconvenient without Anyway, I have found another issue marked with (8). I will look into it further when I find some time. |
Gotcha, this one seems like a bit of a bummer. In an event, I think we're agreed on "on-chain deps support in an imperfect state is better than nothing" and as long as the code path is relatively clean for supporting that, I think it's reasonable to go ahead with PRs that start chopping up the feature for review. |
…dency
Removed `version` field from `[package]` section and from dependency
specification (`DepName = { .. version = "..." }`). Version is vestigial
and this is preparation for the version resolution hook to be
introduced.
Introduced the `custom_resolve_pkg_name` hook. This hook is called during dependency graph resolution and has the effect of changing the package name to the value returned from the hook (instead of the one defined in its manifest).
When using pkg name resolution, dependency error messages during graph resolution will print resolved names instead of the original ones as defined in manifests which will be confusing to the user. This commit makes it so that original package names are used. This is achieved by storing the original name in the `Dependency` struct which is the edge between two packages in the graph. This way the original name can always be recovered.
…messages Store original name of the root package in the `DependencyGraph` so that it can be used for displaying error messages in the resolution graph.
Moved `collect_overrides` functionality to a more natural place. This is used for resolve_version hook implementation later so that the manifest doesn't have to be parsed again.
Implemented the version resolution hook. This hook is called during dependency graph resolution and returns an arbitrary version (`Symbol`) for a package. This is then used by the resolved to check that two different instances of the same package are of the same version. The version hook effectively makes it possible to reference the same package in different locations (e.g., git, local, or on-chain). As long as their name and version matches, they will be considered the same package (respective of the address renaming and override flag). When the version hook isn't used (returns `None`), packages will be differentiated as they were before the introduction of the hook based on their location (dependency kind in parent manifest).
…_override test This test makes more sense with version resolution.
…files This test highlights the problem of error messages using resolved pkg names instead of original when dependnecy lock files are present. This can be resolved by storing original pkg names in lockfiles.
Hooks should only rely on manifest (or possibly lockfile).
Refactor `Custom` dependency to `OnChain` so that RPC doesn't have to be passed in manually for every dependency in the manifest.
Implements `resolve_on_chain_dependency`, `custom_resolve_pkg_name`, and `resolve_version` hooks for Sui so that on-chain dependencies are supported. This is achieved by resolving dependency names to their `original_id` (the id if package version 1) and their version to their on-chain version when the `published-at` field is present in their manifest. In effect, this consolidates the dependency graph so that on-chain and source transitive dependencies can get deduplicated by changing the names of source dependencies to their `original_id` and assigning them their on-chain version. On-chain dependencies are resolved by fetching their contents from the RPC to their respective location in `~/.move/` directory. RPC from current active environment is used.
This alleviates a bit the problem where the dependency graph picks an on-chain package as dependency for a source package (instead of source) when both are present in the graph which causes undefined address name errors. It does not solve the problem for all packages, but in the worst case, user can use an override until a proper solution is worked out.
…ssages (#15708) ## Description Pass down original name of the root package during dependency resolution so that it's displayed correctly in error messages (instead of resolved name). This commit only affects the printed error messages and not functionality of the toolchain. This is part of the work to enable compiling against on-chain dependencies #14178. @amnn @rvantonder ## Test Plan Unit tests. --- If your changes are not user-facing and do not break anything, you can skip the following section. Otherwise, please briefly describe what has changed under the Release Notes section. ### Type of Change (Check all that apply) - [ ] protocol change - [ ] user-visible impact - [ ] breaking change for a client SDKs - [ ] breaking change for FNs (FN binary must upgrade) - [ ] breaking change for validators or node operators (must upgrade binaries) - [ ] breaking change for on-chain data layout - [ ] necessitate either a data wipe or data migration ### Release notes
…gs (#15750) ## Description Store original name of the root package in the `DependencyGraph` so that it can be used for displaying error messages in the resolution graph. This commit only affects the printed error messages and not functionality of the toolchain. This is part of the work to enable compiling against on-chain dependencies #14178. cc @rvantonder @amnn ## Test Plan Added a unit test --- If your changes are not user-facing and do not break anything, you can skip the following section. Otherwise, please briefly describe what has changed under the Release Notes section. ### Type of Change (Check all that apply) - [ ] protocol change - [ ] user-visible impact - [ ] breaking change for a client SDKs - [ ] breaking change for FNs (FN binary must upgrade) - [ ] breaking change for validators or node operators (must upgrade binaries) - [ ] breaking change for on-chain data layout - [ ] necessitate either a data wipe or data migration ### Release notes
## Description Moved `collect_overrides` functionality to a more natural place. This is used for resolve_version hook implementation later so that the manifest doesn't have to be parsed again. This is part of the work to enable compiling against on-chain dependencies #14178. ## Test Plan It's a pure refactor. Ran unit tests. --- If your changes are not user-facing and do not break anything, you can skip the following section. Otherwise, please briefly describe what has changed under the Release Notes section. ### Type of Change (Check all that apply) - [ ] protocol change - [ ] user-visible impact - [ ] breaking change for a client SDKs - [ ] breaking change for FNs (FN binary must upgrade) - [ ] breaking change for validators or node operators (must upgrade binaries) - [ ] breaking change for on-chain data layout - [ ] necessitate either a data wipe or data migration ### Release notes
## Description Implemented the version resolution hook. This hook is called during dependency graph resolution and returns an arbitrary version (`Symbol`) for a package. This is then used by the resolved to check that two different instances of the same package are of the same version. The version hook effectively makes it possible to reference the same package in different locations (e.g., git, local, or on-chain). As long as their name and version matches, they will be considered the same package (respective of the address renaming and override flag). When the version hook isn't used (returns `None`), packages will be differentiated as they were before the introduction of the hook based on their location (dependency kind in parent manifest). This is part of the work to enable compiling against on-chain dependencies #14178. ## Test Plan Added unit tests --- If your changes are not user-facing and do not break anything, you can skip the following section. Otherwise, please briefly describe what has changed under the Release Notes section. ### Type of Change (Check all that apply) - [ ] protocol change - [ ] user-visible impact - [ ] breaking change for a client SDKs - [ ] breaking change for FNs (FN binary must upgrade) - [ ] breaking change for validators or node operators (must upgrade binaries) - [ ] breaking change for on-chain data layout - [ ] necessitate either a data wipe or data migration ### Release notes
…kg_id` (pure refactor) (#16037) ## Description This is the famed rename from `name` to `identifier`, and boy am I glad we went for this because the code is 10x more readable now. In summary, it changes `*_pkg_name` -> `*_pkg_id` and `*_pkg_original_name` -> `*_pkg_name`. In the second commit, I've found additional places where (what is now) `pkg_id` was being used in error messages instead of name so I switched those around. These don't show up in the unit tests because there aren't any that trigger those edge cases but it should be fine because it's clear from the code that these affect only error messages (in a good way) and not functionality of the resolver. The only thing that remains is that we still use the `name = <...>` field in the lockfile for (what is now) `id`. I didn't want to touch this here because it would be a breaking change for the lockfile. In the next PR I'll be adding the additional fields to the lockfile that we discussed and switch this around. This is part of the work to enable compiling against on-chain dependencies #14178. ## Test Plan Unit tests --- If your changes are not user-facing and do not break anything, you can skip the following section. Otherwise, please briefly describe what has changed under the Release Notes section. ### Type of Change (Check all that apply) - [ ] protocol change - [ ] user-visible impact - [ ] breaking change for a client SDKs - [ ] breaking change for FNs (FN binary must upgrade) - [ ] breaking change for validators or node operators (must upgrade binaries) - [ ] breaking change for on-chain data layout - [ ] necessitate either a data wipe or data migration ### Release notes
|
@rvantonder @amnn so for adding package names and root resolved
In summary we:
[1] Now a question may arise: "why are package names stored in the dependencies section of each parent package instead of the package itself?". The reason for that is that package names are, in the graph, stored in edges betwee the nodes (packages). It was done this way in the "id resolution hook" PR because it's a more natural solution implementation wise (storing them in the package would be possible in principle but a way more intrusive implementation), and also because in principle it is the more correct solution. To illustrate this I will give an example. Suppose we're resolving a graph with both a source and on-chain transitive dependency to Therefore, when constructing the lockfile, it is also more natural store the dependency names in the edges ( |
|
Thanks @kklas, I quickly went over that and sounds reasonable to me, in general! I'll go over it a bit more deeply considering our plans to add more to the lock file here. My overarching comment is that we want to namespace resolution by network (chain identifier), so there will be a multiplicity of move packages we track in the lock file going forward. |
|
Update this side @kklas: we're going over some designs for the |
…fest sections (#15148)
## Description
This PR removes the version field both from package section and
dependency items in manifest (e.g. `SomeDep = { local = "../dep",
version = "0.0.1" }`).
Why is this safe to do?
Well, the version field in the package section isn't used anywhere. It's
not read anywhere in the codebase and thus not propagated through the
toolchain.
As for the version in dependency item, what it does it makes the
dependency graph throw a version conflict when the same package is
referenced with two different versions in the dependnecy graph. There
are 3 types of dependencies (`Local`, `Git`, and `Custom`) out of which
only `Local` and `Git` are used on Sui. Since referencing a different
version of a dependency is already possible with git and local by
specifying a different location, this version field isn't useful.
I've also added `version` as a custom field in Sui hooks in order to
make the `unknown field name found` warning go away when compiling
packages with `sui move`. This should be removed once all `version`
fields are removed from all manifests in this repo. This will be a
breaking change for users running the old version of the toolchain so
it's probably best to do that a bit later.
This is part of the work to enable compiling against on-chain
dependencies MystenLabs/sui#14178.
cc @amnn @rvantonder
## Test Plan
Unit tests.
Some tests (like the `diamond_problem_dep_dev_override_with_reg`) were
using the `version` field to create a version conflict while using a
package in the same location. I've refactored those tests to instead
refer to the same package in different location.
---
If your changes are not user-facing and not a breaking change, you can
skip the following section. Otherwise, please indicate what changed, and
then add to the Release Notes section as highlighted during the release
process.
### Type of Change (Check all that apply)
- [ ] protocol change
- [x] user-visible impact
- [ ] breaking change for a client SDKs
- [ ] breaking change for FNs (FN binary must upgrade)
- [ ] breaking change for validators or node operators (must upgrade
binaries)
- [ ] breaking change for on-chain data layout
- [ ] necessitate either a data wipe or data migration
### Release notes
`Move.toml` files no longer require a `version` field in the `[package]`
section and will warn at the presence of a `version` field in a
`[dependency]` definition. Full PR description:
MystenLabs/sui#15148.
## Description Adds a `custom_resolve_pkg_name` hook in order to allow for the dependency graph to discriminate between packages based on a dynamically resolved symbol (returned by the hook) rather than just package name. When the hook isn't defined it returns the package manifest name when so this doesn't affect Sui package resolution currently. This will make it possible to consolidate source and on-chain packages (which don't have a name) within a single graph. This is part of the work to enable compiling against on-chain dependencies MystenLabs/sui#14178. ## Test Plan Added unit tests. --- If your changes are not user-facing and not a breaking change, you can skip the following section. Otherwise, please indicate what changed, and then add to the Release Notes section as highlighted during the release process. ### Type of Change (Check all that apply) - [ ] protocol change - [ ] user-visible impact - [ ] breaking change for a client SDKs - [ ] breaking change for FNs (FN binary must upgrade) - [ ] breaking change for validators or node operators (must upgrade binaries) - [ ] breaking change for on-chain data layout - [ ] necessitate either a data wipe or data migration ### Release notes
…ssages (#15708) ## Description Pass down original name of the root package during dependency resolution so that it's displayed correctly in error messages (instead of resolved name). This commit only affects the printed error messages and not functionality of the toolchain. This is part of the work to enable compiling against on-chain dependencies MystenLabs/sui#14178. @amnn @rvantonder ## Test Plan Unit tests. --- If your changes are not user-facing and do not break anything, you can skip the following section. Otherwise, please briefly describe what has changed under the Release Notes section. ### Type of Change (Check all that apply) - [ ] protocol change - [ ] user-visible impact - [ ] breaking change for a client SDKs - [ ] breaking change for FNs (FN binary must upgrade) - [ ] breaking change for validators or node operators (must upgrade binaries) - [ ] breaking change for on-chain data layout - [ ] necessitate either a data wipe or data migration ### Release notes
…gs (#15750) ## Description Store original name of the root package in the `DependencyGraph` so that it can be used for displaying error messages in the resolution graph. This commit only affects the printed error messages and not functionality of the toolchain. This is part of the work to enable compiling against on-chain dependencies MystenLabs/sui#14178. cc @rvantonder @amnn ## Test Plan Added a unit test --- If your changes are not user-facing and do not break anything, you can skip the following section. Otherwise, please briefly describe what has changed under the Release Notes section. ### Type of Change (Check all that apply) - [ ] protocol change - [ ] user-visible impact - [ ] breaking change for a client SDKs - [ ] breaking change for FNs (FN binary must upgrade) - [ ] breaking change for validators or node operators (must upgrade binaries) - [ ] breaking change for on-chain data layout - [ ] necessitate either a data wipe or data migration ### Release notes
## Description Moved `collect_overrides` functionality to a more natural place. This is used for resolve_version hook implementation later so that the manifest doesn't have to be parsed again. This is part of the work to enable compiling against on-chain dependencies MystenLabs/sui#14178. ## Test Plan It's a pure refactor. Ran unit tests. --- If your changes are not user-facing and do not break anything, you can skip the following section. Otherwise, please briefly describe what has changed under the Release Notes section. ### Type of Change (Check all that apply) - [ ] protocol change - [ ] user-visible impact - [ ] breaking change for a client SDKs - [ ] breaking change for FNs (FN binary must upgrade) - [ ] breaking change for validators or node operators (must upgrade binaries) - [ ] breaking change for on-chain data layout - [ ] necessitate either a data wipe or data migration ### Release notes
## Description Implemented the version resolution hook. This hook is called during dependency graph resolution and returns an arbitrary version (`Symbol`) for a package. This is then used by the resolved to check that two different instances of the same package are of the same version. The version hook effectively makes it possible to reference the same package in different locations (e.g., git, local, or on-chain). As long as their name and version matches, they will be considered the same package (respective of the address renaming and override flag). When the version hook isn't used (returns `None`), packages will be differentiated as they were before the introduction of the hook based on their location (dependency kind in parent manifest). This is part of the work to enable compiling against on-chain dependencies MystenLabs/sui#14178. ## Test Plan Added unit tests --- If your changes are not user-facing and do not break anything, you can skip the following section. Otherwise, please briefly describe what has changed under the Release Notes section. ### Type of Change (Check all that apply) - [ ] protocol change - [ ] user-visible impact - [ ] breaking change for a client SDKs - [ ] breaking change for FNs (FN binary must upgrade) - [ ] breaking change for validators or node operators (must upgrade binaries) - [ ] breaking change for on-chain data layout - [ ] necessitate either a data wipe or data migration ### Release notes
…kg_id` (pure refactor) (#16037) ## Description This is the famed rename from `name` to `identifier`, and boy am I glad we went for this because the code is 10x more readable now. In summary, it changes `*_pkg_name` -> `*_pkg_id` and `*_pkg_original_name` -> `*_pkg_name`. In the second commit, I've found additional places where (what is now) `pkg_id` was being used in error messages instead of name so I switched those around. These don't show up in the unit tests because there aren't any that trigger those edge cases but it should be fine because it's clear from the code that these affect only error messages (in a good way) and not functionality of the resolver. The only thing that remains is that we still use the `name = <...>` field in the lockfile for (what is now) `id`. I didn't want to touch this here because it would be a breaking change for the lockfile. In the next PR I'll be adding the additional fields to the lockfile that we discussed and switch this around. This is part of the work to enable compiling against on-chain dependencies MystenLabs/sui#14178. ## Test Plan Unit tests --- If your changes are not user-facing and do not break anything, you can skip the following section. Otherwise, please briefly describe what has changed under the Release Notes section. ### Type of Change (Check all that apply) - [ ] protocol change - [ ] user-visible impact - [ ] breaking change for a client SDKs - [ ] breaking change for FNs (FN binary must upgrade) - [ ] breaking change for validators or node operators (must upgrade binaries) - [ ] breaking change for on-chain data layout - [ ] necessitate either a data wipe or data migration ### Release notes
## Description This PR fixes multiple issues caused by bytecode dependencies not being included in `CompiledPackage`: - multiple functionalities failing due to topo sort in `Modules` panicking on missing dependencies - running `move test` on packages with bytecode deps failing due to them not being included in VM storage - source verification failing because bytecode deps aren't handled - commands such as publish and upgrade failing due to bytecode deps not being referenced in the construction of transaction blocks Summary of changes: - removed `move_bytecode_utils::dependency_graph` module and instead added a `compute_topological_order` method to `Modules`. Replaced all calls to `compute_dependency_graph` with a direct call to `compute_topological_order` (2 in total) - added bytecode deps to VM storage for test runs by loading them from `ResolvedGraph` - include bytecode deps in `sui_move_build::CompiledPackage` and fix various module fetching methods to return modules from bytecode deps also - include bytecode deps in `local_modules` function to fix `LocalDependencyNotFound` errors in source verification This is part of the work to enable compiling against on-chain dependencies #14178. cc @rvantonder @amnn ## Test Plan Added unit tests for `move test` and source verification. --- If your changes are not user-facing and do not break anything, you can skip the following section. Otherwise, please briefly describe what has changed under the Release Notes section. ### Type of Change (Check all that apply) - [ ] protocol change - [ ] user-visible impact - [ ] breaking change for a client SDKs - [ ] breaking change for FNs (FN binary must upgrade) - [ ] breaking change for validators or node operators (must upgrade binaries) - [ ] breaking change for on-chain data layout - [ ] necessitate either a data wipe or data migration ### Release notes
## Description This PR fixes multiple issues caused by bytecode dependencies not being included in `CompiledPackage`: - multiple functionalities failing due to topo sort in `Modules` panicking on missing dependencies - running `move test` on packages with bytecode deps failing due to them not being included in VM storage - source verification failing because bytecode deps aren't handled - commands such as publish and upgrade failing due to bytecode deps not being referenced in the construction of transaction blocks Summary of changes: - removed `move_bytecode_utils::dependency_graph` module and instead added a `compute_topological_order` method to `Modules`. Replaced all calls to `compute_dependency_graph` with a direct call to `compute_topological_order` (2 in total) - added bytecode deps to VM storage for test runs by loading them from `ResolvedGraph` - include bytecode deps in `sui_move_build::CompiledPackage` and fix various module fetching methods to return modules from bytecode deps also - include bytecode deps in `local_modules` function to fix `LocalDependencyNotFound` errors in source verification This is part of the work to enable compiling against on-chain dependencies #14178. cc @rvantonder @amnn ## Test Plan Added unit tests for `move test` and source verification. --- If your changes are not user-facing and do not break anything, you can skip the following section. Otherwise, please briefly describe what has changed under the Release Notes section. ### Type of Change (Check all that apply) - [ ] protocol change - [ ] user-visible impact - [ ] breaking change for a client SDKs - [ ] breaking change for FNs (FN binary must upgrade) - [ ] breaking change for validators or node operators (must upgrade binaries) - [ ] breaking change for on-chain data layout - [ ] necessitate either a data wipe or data migration ### Release notes
…rate `name` field to store manifest name (#19387) ## Description As discussed in #14178 (comment) (second and third point), a few changes to the lockfile: - the `name` field in `dependencies` is renamed to `id` to better reflect the meaning in the dependency graph (the packages are discriminated by their identifier, as resolved by the hook, which is not necessarily their manifest name) - added a `name` field which will store the dependency manifest name (this is needed to show user-friendly error messages using the package manifest name instead of identifier which may be confusing) - bumped lockfile version to 3 This is part of the work to enable compiling against on-chain dependencies #14178. cc @rvantonder @amnn ## Test plan All changes are covered by existing unit tests. --- ## Release notes Check each box that your changes affect. If none of the boxes relate to your changes, release notes aren't required. For each box you select, include information after the relevant heading that describes the impact of your changes that a user might notice and any actions they must take to implement updates. - [ ] Protocol: - [ ] Nodes (Validators and Full nodes): - [ ] Indexer: - [ ] JSON-RPC: - [ ] GraphQL: - [x] CLI: Introduce lock file version 3, which renames a dependency's `name` field to `id` and introduces a separate `name` field that stores the package's name from its manifest for improved error reporting. Older lock files will be ignored when resolving dependencies so that this information is always available. - [ ] Rust SDK: - [ ] REST API:
…9540) ## Description Refactor `Custom` dependency to `OnChain` (in the manifest) and streamline it. It now requires only the `id` field which specifies dependency's on-chain id (e.g. `Foo = { id = "0x123" }`. `Custom` dependencies aren't supported on Sui so this should be a fairly safe change. This is part of the work to enable compiling against on-chain dependencies #14178. cc @rvantonder @amnn ## Test plan Updated existing unit tests. --- ## Release notes Check each box that your changes affect. If none of the boxes relate to your changes, release notes aren't required. For each box you select, include information after the relevant heading that describes the impact of your changes that a user might notice and any actions they must take to implement updates. - [ ] Protocol: - [ ] Nodes (Validators and Full nodes): - [ ] Indexer: - [ ] JSON-RPC: - [ ] GraphQL: - [x] CLI: Remove legacy support for custom package hooks, replacing it with initial logic for on-chain dependencies. - [ ] Rust SDK: - [ ] REST API:
… graph (#19595) ## Description Small fix to correctly load dependency version when using external resolver. Changed `echo` in `successful_package_batch_response.sh` to `printf` for more consistent handling of `\0` across systems (it fails on my machine). This is part of the work to enable compiling against on-chain dependencies #14178. cc @rvantonder @amnn ## Test plan Updated the unit test to have a dependency with `version` field. --- ## Release notes Check each box that your changes affect. If none of the boxes relate to your changes, release notes aren't required. For each box you select, include information after the relevant heading that describes the impact of your changes that a user might notice and any actions they must take to implement updates. - [ ] Protocol: - [ ] Nodes (Validators and Full nodes): - [ ] Indexer: - [ ] JSON-RPC: - [ ] GraphQL: - [ ] CLI: - [ ] Rust SDK: - [ ] REST API:
Description
Context https://gist.github.com/kklas/d9edad7875548fae57b0c86bc8871fea
Unresolved issues and discussion points:
(1) Due to how dependencies are resolved, some address mappings don't propagate properly through the graph when the same package exists as on-chain and source (transitive) dependency. This happens becuase the dependency graph treats packages of the same name and version as the same regradless of their location so sometimes the on-chain package ends up in the resulting
package_tableand since those don't have address names defined, it can cause undefined address name errors in the parent source packages. I alleviated this a bit by adding names for system packages ("std", "sui", "sui_system", and "deepbook") to the generated manifest in 2623a91 but this doesn't solve the problem for all packages. It could be perhapse solved by keeping track of all the instances of the same package during dependency resolution and then resolving the addresses by collecting them from all. This is simple in principle but could get tricky to implement because overrides and address renamings also need to be respected. This could also be solved with a source discovery service where we could fetch the names from. In any case, the user can always do an override with a source package to go around this issue.(2) There is now a conflict between old and new lockfiles. The old ones will use non-resolved names, so when building a package with the new binary and dependencies have lockfiles it will cause an incosistency. This is because lockfiles are only regenerated based on manifest and dependency manifest digests, which don't change here. This can be solved by bumping the lockfile version or forcing lockfile regeneration on resulting lockfile contents change https://github.com/MystenLabs/sui/blob/main/external-crates/move/tools/move-package/src/resolution/dependency_graph.rs#L240-L241
(3) Lockfiles don't store package's original name, so when loading dependency graph from lockfiles, error messages will print out resolved names instead of original which will be confusing to the user. I illustrated this issue in b5c3093. This can be solved by adding original package names to lockfiles.
(4) I've made only an in-memory cache for on-chain dependencies and each is fetched once per run for simplicity and seems fast enough. They can easilly be cached to disk in
~/.movethough with a bit of consideration about consistency. System packages can also be embedded in the binary to avoid RPC fetching.(5) The
versionfield can now be removed from all manifests in this repo. Building with the old binary will cause an error in that case. LMK if you want me to do that. Otherwise it will just cause an extra field warning when building with the new binary.(6) To avoid confusion with variable names, the
custom_resolve_pkg_namehook should perhaps be calledresolve_pkg_identifierhook andidentifiershould be used in place ofpkg_nameandresolved_nameetc.(7)
move-analyzerdoesn't work with both custom hooks and bytecode dependencies. I've managed to make it work with custom with a hacky solution where I've copied the whole crate to a new one and made it use the hooks (it required, among other things, to make it use tokio). But it still didn't work because it has some issues probably with interpreting the bytecode and while the server works it throws errors. I didn't dig deeper.(8)
sui move testdoesn't work with on-chain packages. This is because of two separate reasons:#[test_only]modules aren't included, but the test runner requires thestd::unit_testmodule and fails withCompilation in test mode requires passing the UnitTest module in the Move stdlib as a dependencyexternal-crates/move/tools/move-unit-test/src/test_runner.rs:83that relies onmove-bytecode-utils/src/dependency_graph.rsstack trace. Similar why publish was failing (and fixed in 6b28139) it is because the compiler doesn't return the bytecode modules in the result and the graph construction fails due to missing dependency.Both of these can fortunately be solved by doing an appropriate override, but the second issue means that we can't do unit tests when on-chain / bytecodes are present until this is resolved (it's very solveable though). The first issue can be solved by including the source for
0x1instead of bytecode (and also probably for0x2,0x3and other framework packages) but there's some work to figure out how to embed the source code in the binary (and which version of it as there may be discrepencies between networks).Test Plan
Unit and integration tests.
Type of Change (Check all that apply)
Release notes