Skip to content

Commit

Permalink
move: support automated address management (#17995)
Browse files Browse the repository at this point in the history
## Description 

This PR adds automated address management support for `sui client
publish` and `sui client upgrade`. The `chain_id` is resolved early in
sui commands and propagated down where published ID resolution is
needed.

High-level:

- If a package contains a `Move.lock` that contains info to support a
package upgrade, then that is used, and a user does not have to (nor
should they) maintain a `published-at` address in their `Move.toml`.

- If a package does _not_ have any `Move.lock` information to support a
package upgrade, we will still fallback to `Move.toml` as we do today.
Nothing changes.

- If a package has both a `Move.lock` and `Move.toml`, then the
`Move.lock` will be used (the user is free to delete `published-at` in
their `Move.toml`). Caveat: If a published address is in both of these
files, they must be the same (we will raise a `Conflict` error message
otherwise).
 
- Note: docs to follow and merge at the same time as this PR,
particularly [this section on
upgrades](https://docs.sui.io/concepts/sui-move-concepts/packages/upgrade#example)

- Note after this PR it is still required to set `[addresses]` to `0x0`
in `Move.toml`. This requirement will be removed subsequently (which
also requires setting to `0x0` still). I didn't want to meddle with that
logic all at once.

## Test plan 

How did you test the new or updated feature?

We cover three cases:

- Upgrade with `Move.toml` only (existing package upgrade test)
- Upgrade with `Move.lock` only (extension of existing package
management test)
- Upgrade with `Move.lock` and `Move.toml` with conflicting addresses
error condition (new test)

- Existing e2e tests pass (and SDK tests now resolve chain ID for
publish commands)

---

## 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: Running `sui client publish` or `sui client upgrade` will now
cause package addresses to be tracked in the `Move.lock` file. This
removes the need to manually record or edit `published-at` addresses in
the `Move.toml` file going forward. See docs PR
#18433 for full details.
- [ ] Rust SDK:
  • Loading branch information
rvantonder authored Jun 27, 2024
1 parent 74616f0 commit 56f9c65
Show file tree
Hide file tree
Showing 15 changed files with 321 additions and 67 deletions.
3 changes: 3 additions & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

10 changes: 5 additions & 5 deletions crates/sui-core/src/unit_tests/move_package_publish_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -301,11 +301,11 @@ async fn test_custom_property_check_unpublished_dependencies() {
.resolution_graph_for_package(&path, &mut std::io::sink())
.expect("Could not build resolution graph.");

let SuiError::ModulePublishFailure { error } =
check_unpublished_dependencies(&gather_published_ids(&resolution_graph).1.unpublished)
.err()
.unwrap()
else {
let SuiError::ModulePublishFailure { error } = check_unpublished_dependencies(
&gather_published_ids(&resolution_graph, None).1.unpublished,
)
.err()
.unwrap() else {
panic!("Expected ModulePublishFailure")
};

Expand Down
8 changes: 3 additions & 5 deletions crates/sui-e2e-tests/tests/snapshot_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -41,11 +41,9 @@ async fn run_one(
}
}
}
SuiCommand::Move {
package_path: _,
build_config: _,
cmd: _,
} => unimplemented!("Supporting Move publish and upgrade commands"),
SuiCommand::Move { .. } => {
unimplemented!("Supporting Move publish and upgrade commands")
}
_ => panic!("Command {:?} not supported by RPC snapshot tests", cli_cmd),
}
}
Expand Down
5 changes: 5 additions & 0 deletions crates/sui-framework/build.rs
Original file line number Diff line number Diff line change
Expand Up @@ -157,34 +157,39 @@ fn build_packages_with_move_config(
config: config.clone(),
run_bytecode_verifier: true,
print_diags_to_stderr: false,
chain_id: None, // Framework pkg addr is agnostic to chain, resolves from Move.toml
}
.build(stdlib_path)
.unwrap();
let framework_pkg = BuildConfig {
config: config.clone(),
run_bytecode_verifier: true,
print_diags_to_stderr: false,
chain_id: None, // Framework pkg addr is agnostic to chain, resolves from Move.toml
}
.build(sui_framework_path)
.unwrap();
let system_pkg = BuildConfig {
config: config.clone(),
run_bytecode_verifier: true,
print_diags_to_stderr: false,
chain_id: None, // Framework pkg addr is agnostic to chain, resolves from Move.toml
}
.build(sui_system_path)
.unwrap();
let deepbook_pkg = BuildConfig {
config: config.clone(),
run_bytecode_verifier: true,
print_diags_to_stderr: false,
chain_id: None, // Framework pkg addr is agnostic to chain, resolves from Move.toml
}
.build(deepbook_path)
.unwrap();
let bridge_pkg = BuildConfig {
config,
run_bytecode_verifier: true,
print_diags_to_stderr: false,
chain_id: None, // Framework pkg addr is agnostic to chain, resolves from Move.toml
}
.build(bridge_path)
.unwrap();
Expand Down
1 change: 1 addition & 0 deletions crates/sui-move-build/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ sui-verifier = { path = "../../sui-execution/latest/sui-verifier", package = "su
serde-reflection.workspace = true
sui-types.workspace = true
sui-protocol-config.workspace = true
sui-package-management.workspace = true

move-binary-format.workspace = true
move-bytecode-utils.workspace = true
Expand Down
30 changes: 22 additions & 8 deletions crates/sui-move-build/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ use move_package::{
};
use move_symbol_pool::Symbol;
use serde_reflection::Registry;
use sui_package_management::{resolve_published_id, PublishedAtError};
use sui_protocol_config::{Chain, ProtocolConfig, ProtocolVersion};
use sui_types::{
base_types::ObjectID,
Expand Down Expand Up @@ -74,6 +75,9 @@ pub struct BuildConfig {
pub run_bytecode_verifier: bool,
/// If true, print build diagnostics to stderr--no printing if false
pub print_diags_to_stderr: bool,
/// The chain ID that compilation is with respect to (e.g., required to resolve
/// published dependency IDs from the `Move.lock`).
pub chain_id: Option<String>,
}

impl BuildConfig {
Expand Down Expand Up @@ -157,11 +161,13 @@ impl BuildConfig {
pub fn build(self, path: &Path) -> SuiResult<CompiledPackage> {
let print_diags_to_stderr = self.print_diags_to_stderr;
let run_bytecode_verifier = self.run_bytecode_verifier;
let chain_id = self.chain_id.clone();
let resolution_graph = self.resolution_graph(path)?;
build_from_resolution_graph(
resolution_graph,
run_bytecode_verifier,
print_diags_to_stderr,
chain_id,
)
}

Expand Down Expand Up @@ -220,8 +226,9 @@ pub fn build_from_resolution_graph(
resolution_graph: ResolvedGraph,
run_bytecode_verifier: bool,
print_diags_to_stderr: bool,
chain_id: Option<String>,
) -> SuiResult<CompiledPackage> {
let (published_at, dependency_ids) = gather_published_ids(&resolution_graph);
let (published_at, dependency_ids) = gather_published_ids(&resolution_graph, chain_id);

let result = if print_diags_to_stderr {
BuildConfig::compile_package(resolution_graph, &mut std::io::stderr())
Expand Down Expand Up @@ -569,6 +576,7 @@ impl Default for BuildConfig {
config,
run_bytecode_verifier: true,
print_diags_to_stderr: false,
chain_id: None,
}
}
}
Expand Down Expand Up @@ -631,12 +639,9 @@ pub struct PackageDependencies {
pub unpublished: BTreeSet<Symbol>,
/// Set of dependencies with invalid `published-at` addresses.
pub invalid: BTreeMap<Symbol, String>,
}

#[derive(Debug, Clone)]
pub enum PublishedAtError {
Invalid(String),
NotPresent,
/// Set of dependencies that have conflicting `published-at` addresses. The key refers to
/// the package, and the tuple refers to the address in the (Move.lock, Move.toml) respectively.
pub conflicting: BTreeMap<Symbol, (String, String)>,
}

/// Partition packages in `resolution_graph` into one of four groups:
Expand All @@ -646,16 +651,18 @@ pub enum PublishedAtError {
/// - The names of packages that have a `published-at` field that isn't filled with a valid address.
pub fn gather_published_ids(
resolution_graph: &ResolvedGraph,
chain_id: Option<String>,
) -> (Result<ObjectID, PublishedAtError>, PackageDependencies) {
let root = resolution_graph.root_package();

let mut published = BTreeMap::new();
let mut unpublished = BTreeSet::new();
let mut invalid = BTreeMap::new();
let mut conflicting = BTreeMap::new();
let mut published_at = Err(PublishedAtError::NotPresent);

for (name, package) in &resolution_graph.package_table {
let property = published_at_property(package);
let property = resolve_published_id(package, chain_id.clone());
if name == &root {
// Separate out the root package as a special case
published_at = property;
Expand All @@ -672,6 +679,12 @@ pub fn gather_published_ids(
Err(PublishedAtError::Invalid(value)) => {
invalid.insert(*name, value);
}
Err(PublishedAtError::Conflict {
id_lock,
id_manifest,
}) => {
conflicting.insert(*name, (id_lock, id_manifest));
}
};
}

Expand All @@ -681,6 +694,7 @@ pub fn gather_published_ids(
published,
unpublished,
invalid,
conflicting,
},
)
}
Expand Down
1 change: 1 addition & 0 deletions crates/sui-move/src/build.rs
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@ impl Build {
config,
run_bytecode_verifier: true,
print_diags_to_stderr: true,
chain_id: None,
}
.build(rerooted_path)?;
if dump_bytecode_as_base64 {
Expand Down
2 changes: 2 additions & 0 deletions crates/sui-package-management/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -15,5 +15,7 @@ tracing.workspace = true

sui-json-rpc-types.workspace = true
sui-sdk.workspace = true
sui-types.workspace = true

move-package.workspace = true
move-symbol-pool.workspace = true
85 changes: 84 additions & 1 deletion crates/sui-package-management/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,17 +2,37 @@
// SPDX-License-Identifier: Apache-2.0

use anyhow::{bail, Context};
use std::fs::File;
use std::path::PathBuf;
use std::str::FromStr;

use move_package::lock_file::{self, LockFile};
use move_package::{
lock_file::{self, schema::ManagedPackage, LockFile},
resolution::resolution_graph::Package,
source_package::layout::SourcePackageLayout,
};
use move_symbol_pool::Symbol;
use sui_json_rpc_types::{get_new_package_obj_from_response, SuiTransactionBlockResponse};
use sui_sdk::wallet_context::WalletContext;
use sui_types::base_types::ObjectID;

const PUBLISHED_AT_MANIFEST_FIELD: &str = "published-at";

pub enum LockCommand {
Publish,
Upgrade,
}

#[derive(Debug, Clone)]
pub enum PublishedAtError {
Invalid(String),
NotPresent,
Conflict {
id_lock: String,
id_manifest: String,
},
}

/// Update the `Move.lock` file with automated address management info.
/// Expects a wallet context, the publish or upgrade command, its response.
/// The `Move.lock` principally file records the published address (i.e., package ID) of
Expand Down Expand Up @@ -73,3 +93,66 @@ pub async fn update_lock_file(
lock.commit(lock_file)?;
Ok(())
}

/// Find the published on-chain ID in the `Move.lock` or `Move.toml` file.
/// A chain ID of `None` means that we will only try to resolve a published ID from the Move.toml.
/// The published ID is resolved from the `Move.toml` if the Move.lock does not exist.
/// Else, we resolve from the `Move.lock`, where addresses are automatically
/// managed. If conflicting IDs are found in the `Move.lock` vs. `Move.toml`, a
/// "Conflict" error message returns.
pub fn resolve_published_id(
package: &Package,
chain_id: Option<String>,
) -> Result<ObjectID, PublishedAtError> {
// Look up a valid `published-at` in the `Move.toml` first, which we'll
// return if the Move.lock does not manage addresses.
let published_id_in_manifest = match published_at_property(package) {
Ok(v) => Some(v),
Err(PublishedAtError::NotPresent) => None,
Err(e) => return Err(e), // An existing but invalid `published-at` in `Move.toml` should fail early.
};

let lock = package.package_path.join(SourcePackageLayout::Lock.path());
let Ok(mut lock_file) = File::open(lock.clone()) else {
return match published_id_in_manifest {
Some(v) => {
ObjectID::from_str(v.as_str()).map_err(|_| PublishedAtError::Invalid(v.to_owned()))
}
None => Err(PublishedAtError::NotPresent),
};
};
let managed_packages = ManagedPackage::read(&mut lock_file).ok();
// Find the environment and ManagedPackage data for this chain_id.
let env_for_chain_id = managed_packages
.and_then(|m| {
let chain_id = chain_id.as_ref()?;
m.into_iter().find(|(_, v)| v.chain_id == *chain_id)
})
.map(|(k, v)| (k, v.latest_published_id));

let package_id = match (env_for_chain_id, published_id_in_manifest) {
(Some((_env, id_lock)), Some(id_manifest)) if id_lock != id_manifest => {
return Err(PublishedAtError::Conflict {
id_lock,
id_manifest,
})
}
(Some((_, id_lock)), _) => id_lock,
(None, Some(id_manifest)) => id_manifest, /* No info in Move.lock: Fall back to manifest */
_ => return Err(PublishedAtError::NotPresent), /* Neither in Move.toml nor Move.lock */
};
ObjectID::from_str(package_id.as_str())
.map_err(|_| PublishedAtError::Invalid(package_id.to_owned()))
}

fn published_at_property(package: &Package) -> Result<String, PublishedAtError> {
let Some(value) = package
.source_package
.package
.custom_properties
.get(&Symbol::from(PUBLISHED_AT_MANIFEST_FIELD))
else {
return Err(PublishedAtError::NotPresent);
};
Ok(value.to_string())
}
17 changes: 10 additions & 7 deletions crates/sui-source-validation-service/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -151,6 +151,15 @@ pub async fn verify_package(
package_path: impl AsRef<Path>,
) -> anyhow::Result<(Network, AddressLookup)> {
move_package::package_hooks::register_package_hooks(Box::new(SuiPackageHooks));
// TODO(rvantonder): use config RPC URL instead of hardcoded URLs
let network_url = match network {
Network::Mainnet => MAINNET_URL,
Network::Testnet => TESTNET_URL,
Network::Devnet => DEVNET_URL,
Network::Localnet => LOCALNET_URL,
};
let client = SuiClientBuilder::default().build(network_url).await?;
let chain_id = client.read_api().get_chain_identifier().await?;
let mut config =
resolve_lock_file_path(MoveBuildConfig::default(), Some(package_path.as_ref()))?;
config.lint_flag = LintFlag::LEVEL_NONE;
Expand All @@ -159,16 +168,10 @@ pub async fn verify_package(
config,
run_bytecode_verifier: false, /* no need to run verifier if code is on-chain */
print_diags_to_stderr: false,
chain_id: Some(chain_id),
};
let compiled_package = build_config.build(package_path.as_ref())?;

let network_url = match network {
Network::Mainnet => MAINNET_URL,
Network::Testnet => TESTNET_URL,
Network::Devnet => DEVNET_URL,
Network::Localnet => LOCALNET_URL,
};
let client = SuiClientBuilder::default().build(network_url).await?;
BytecodeSourceVerifier::new(client.read_api())
.verify_package(
&compiled_package,
Expand Down
Loading

0 comments on commit 56f9c65

Please sign in to comment.