Skip to content

Commit

Permalink
move: validate dependencies' published addresses (MystenLabs#8919)
Browse files Browse the repository at this point in the history
## Description 

Validate `published-at` properties in `Move.toml` for all transitive
package dependencies on `publish`.

## Test Plan 

Covered by existing `simtest` and publish tests. Added additional test
to exercise the case where `published-at` is not set.

---
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)

- [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

Copied from `doc/in-progress/move_published_at_property.md`:

> Package dependencies are now required to specify a `published-at`
field in `Move.toml` that specifies the address that the dependency is
published at. For example, The SUI framework is published at address
`0x2`. So, the `Move.toml` file for the SUI framework has a
corresponding line that says:
>
```toml
published-at = "0x2"
```
>
> If your package depends on another package, like the SUI framework,
your package will be linked against the `published-at` address specified
by the SUI framework on-chain once you publish your package. When
publishing, we resolve all of your package dependencies (i.e.,
transitive dependencies) to link against. This means we recommend
publishing packages where all dependencies have a `published-at` address
in their manifest. The publish command will fail by default if this is
not the case. If needed, you may use the
`--with-unpublished-dependencies` flag with the publish command to
bypass the requirement that all dependencies require a `published-at`
address. When using `--with-unpublished-dependencies`, all unpublished
dependencies are treated as if they are part of your package.
  • Loading branch information
rvantonder authored Mar 9, 2023
1 parent f39cb01 commit feb8654
Show file tree
Hide file tree
Showing 22 changed files with 249 additions and 48 deletions.
5 changes: 5 additions & 0 deletions Cargo.lock

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

1 change: 1 addition & 0 deletions crates/sui-benchmark/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ telemetry-subscribers.workspace = true
roaring = "0.10.1"

move-core-types.workspace = true
move-package.workspace = true
narwhal-node = { path = "../../narwhal/node" }
workspace-hack = { version = "0.1", path = "../workspace-hack" }
test-utils = { path = "../test-utils" }
Expand Down
3 changes: 2 additions & 1 deletion crates/sui-core/src/test_utils.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ use std::sync::Arc;
use std::time::Duration;
use sui_config::genesis::Genesis;
use sui_config::ValidatorInfo;
use sui_framework_build::compiled_package::{BuildConfig, CompiledPackage};
use sui_framework_build::compiled_package::{BuildConfig, CompiledPackage, SuiPackageHooks};
use sui_protocol_config::ProtocolConfig;
use sui_types::base_types::ObjectID;
use sui_types::crypto::{
Expand Down Expand Up @@ -128,6 +128,7 @@ pub fn dummy_transaction_effects(tx: &Transaction) -> TransactionEffects {
}

pub fn compile_basics_package() -> CompiledPackage {
move_package::package_hooks::register_package_hooks(Box::new(SuiPackageHooks {}));
let mut path = PathBuf::from(env!("CARGO_MANIFEST_DIR"));
path.push("../../sui_programmability/examples/basics");

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
[package]
name = "CustomPropertiesInManifestDependencyInvalidPublishedAt"
version = "0.0.0"
# oops we put a non-ID value for published-at...
published-at = "mystery"

[addresses]
main = "0x0"
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
// Copyright (c) Mysten Labs, Inc.
// SPDX-License-Identifier: Apache-2.0

module main::main {
public entry fun main() {}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
[package]
name = "CustomPropertiesInManifestDependencyMissingPublishedAt"
version = "0.0.0"
# oops we there's no published-at field, so a package that depends on us should fail when published...
# published-at = "0x777"

[addresses]
main = "0x0"
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
// Copyright (c) Mysten Labs, Inc.
// SPDX-License-Identifier: Apache-2.0

module main::main {
public entry fun main() {}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
[package]
name = "CustomPropertiesInManifestEnsurePublishedAt"
version = "0.0.0"

[dependencies]
CustomPropertiesInManifestDependencyMissingPublishedAt = { local = "../custom_properties_in_manifest_dependency_missing_published_at" }
CustomPropertiesInManifestDependencyInvalidPublishedAt = { local = "../custom_properties_in_manifest_dependency_invalid_published_at" }

[addresses]
main = "0x0"
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
// Copyright (c) Mysten Labs, Inc.
// SPDX-License-Identifier: Apache-2.0

module main::main {
public entry fun main() {}
}
44 changes: 38 additions & 6 deletions crates/sui-core/src/unit_tests/move_integration_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ use move_core_types::{
value::{MoveStruct, MoveValue},
};
use move_package::source_package::manifest_parser;
use sui_framework_build::compiled_package::{BuildConfig, SuiPackageHooks};
use sui_framework_build::compiled_package::{ensure_published_dependencies, BuildConfig};
use sui_types::{
crypto::{get_key_pair, AccountKeyPair},
error::SuiError,
Expand Down Expand Up @@ -2097,16 +2097,14 @@ async fn test_generate_lock_file() {

#[tokio::test]
#[cfg_attr(msim, ignore)]
async fn test_custom_property_published_at() {
async fn test_custom_property_parse_published_at() {
let build_config = BuildConfig::new_for_testing();
let mut path = PathBuf::from(env!("CARGO_MANIFEST_DIR"));
path.extend(["src", "unit_tests", "data", "custom_properties_in_manifest"]);

move_package::package_hooks::register_package_hooks(Box::new(SuiPackageHooks {}));
sui_framework::build_move_package(&path, build_config).expect("Move package did not build");
let manifest =
manifest_parser::parse_move_manifest_from_file(&path.as_path().join("Move.toml"))
.expect("Could not parse Move.toml");
let manifest = manifest_parser::parse_move_manifest_from_file(path.as_path())
.expect("Could not parse Move.toml");
let properties = manifest
.package
.custom_properties
Expand All @@ -2125,6 +2123,40 @@ async fn test_custom_property_published_at() {
expected.assert_debug_eq(&properties)
}

#[tokio::test]
#[cfg_attr(msim, ignore)]
async fn test_custom_property_ensure_published_at() {
let build_config = BuildConfig::new_for_testing();
let mut path = PathBuf::from(env!("CARGO_MANIFEST_DIR"));
path.extend([
"src",
"unit_tests",
"data",
"custom_properties_in_manifest_ensure_published_at",
]);

let resolution_graph = build_config
.config
.resolution_graph_for_package(&path, &mut Vec::new())
.expect("Could not build resolution graph.");

let error_message = if let SuiError::ModulePublishFailure { error } =
ensure_published_dependencies(&resolution_graph)
.err()
.unwrap()
{
error
} else {
"Expected ModulePublishFailure".into()
};

let expected = expect![[r#"
Package dependency "CustomPropertiesInManifestDependencyInvalidPublishedAt" does not specify a valid published address: could not parse value "mystery" for published-at field.
Package dependency "CustomPropertiesInManifestDependencyMissingPublishedAt" does not specify a published address (the Move.toml manifest for "CustomPropertiesInManifestDependencyMissingPublishedAt" does not contain a published-at field).
If this is intentional, you may use the --with-unpublished-dependencies flag to continue publishing these dependencies as part of your package (they won't be linked against existing packages on-chain)."#]];
expected.assert_eq(&error_message)
}

pub async fn build_and_try_publish_test_package(
authority: &AuthorityState,
sender: &SuiAddress,
Expand Down
151 changes: 120 additions & 31 deletions crates/sui-framework-build/src/compiled_package.rs
Original file line number Diff line number Diff line change
@@ -1,10 +1,12 @@
// Copyright (c) Mysten Labs, Inc.
// SPDX-License-Identifier: Apache-2.0

use core::fmt;
use std::{
collections::{BTreeMap, BTreeSet, HashSet},
io::Write,
path::PathBuf,
str::FromStr,
};

use fastcrypto::encoding::Base64;
Expand Down Expand Up @@ -35,8 +37,10 @@ use move_package::{
resolution::resolution_graph::ResolvedGraph,
BuildConfig as MoveBuildConfig,
};
use move_symbol_pool::Symbol;
use serde_reflection::Registry;
use sui_types::{
base_types::ObjectID,
error::{SuiError, SuiResult},
move_package::{FnInfo, FnInfoKey, FnInfoMap},
MOVE_STDLIB_ADDRESS, SUI_FRAMEWORK_ADDRESS,
Expand Down Expand Up @@ -126,48 +130,62 @@ impl BuildConfig {
/// Given a `path` and a `build_config`, build the package in that path, including its dependencies.
/// If we are building the Sui framework, we skip the check that the addresses should be 0
pub fn build(self, path: PathBuf) -> SuiResult<CompiledPackage> {
let res = if self.print_diags_to_stderr {
let resolution_graph = self
.config
let resolution_graph = if self.print_diags_to_stderr {
self.config
.resolution_graph_for_package(&path, &mut std::io::stderr())
.map_err(|err| SuiError::ModuleBuildFailure {
error: format!("{:?}", err),
})?;
Self::compile_package(resolution_graph, &mut std::io::stderr())
})?
} else {
let resolution_graph = self
.config
self.config
.resolution_graph_for_package(&path, &mut Vec::new())
.map_err(|err| SuiError::ModuleBuildFailure {
error: format!("{:?}", err),
})?;
Self::compile_package(resolution_graph, &mut Vec::new())
})?
};
build_from_resolution_graph(
path,
resolution_graph,
self.run_bytecode_verifier,
self.print_diags_to_stderr,
)
}
}

// write build failure diagnostics to stderr, convert `error` to `String` using `Debug`
// format to include anyhow's error context chain.
let (package, fn_info) = match res {
Err(error) => {
return Err(SuiError::ModuleBuildFailure {
error: format!("{:?}", error),
})
}
Ok((package, fn_info)) => (package, fn_info),
};
let compiled_modules = package.root_modules_map();
if self.run_bytecode_verifier {
for m in compiled_modules.iter_modules() {
move_bytecode_verifier::verify_module(m).map_err(|err| {
SuiError::ModuleVerificationFailure {
error: err.to_string(),
}
})?;
sui_bytecode_verifier::verify_module(m, &fn_info)?;
}
// TODO(https://github.com/MystenLabs/sui/issues/69): Run Move linker
pub fn build_from_resolution_graph(
path: PathBuf,
resolution_graph: ResolvedGraph,
run_bytecode_verifier: bool,
print_diags_to_stderr: bool,
) -> SuiResult<CompiledPackage> {
let result = if print_diags_to_stderr {
BuildConfig::compile_package(resolution_graph, &mut std::io::stderr())
} else {
BuildConfig::compile_package(resolution_graph, &mut Vec::new())
};
// write build failure diagnostics to stderr, convert `error` to `String` using `Debug`
// format to include anyhow's error context chain.
let (package, fn_info) = match result {
Err(error) => {
return Err(SuiError::ModuleBuildFailure {
error: format!("{:?}", error),
})
}
Ok(CompiledPackage { package, path })
Ok((package, fn_info)) => (package, fn_info),
};
let compiled_modules = package.root_modules_map();
if run_bytecode_verifier {
for m in compiled_modules.iter_modules() {
move_bytecode_verifier::verify_module(m).map_err(|err| {
SuiError::ModuleVerificationFailure {
error: err.to_string(),
}
})?;
sui_bytecode_verifier::verify_module(m, &fn_info)?;
}
// TODO(https://github.com/MystenLabs/sui/issues/69): Run Move linker
}
Ok(CompiledPackage { package, path })
}

impl CompiledPackage {
Expand Down Expand Up @@ -474,3 +492,74 @@ impl PackageHooks for SuiPackageHooks {
Ok(())
}
}

enum AddressError {
NoPublishedAddress(Symbol),
InvalidPublishedAddress(Symbol, String),
}

impl fmt::Display for AddressError {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
match self {
AddressError::NoPublishedAddress(name) => write!(
f,
"Package dependency \"{name}\" does not \
specify a published address \
(the Move.toml manifest for \"{name}\" does \
not contain a published-at field)."
),
AddressError::InvalidPublishedAddress(name, value) => write!(
f,
"Package dependency \"{name}\" does not \
specify a valid published address: \
could not parse value \"{value}\" for published-at field.",
),
}
}
}

/// Ensure dependencies are published (contain `published-at` address in manifest).
pub fn ensure_published_dependencies(resolution_graph: &ResolvedGraph) -> Result<(), SuiError> {
let mut address_errors = vec![];
for name in resolution_graph.graph.package_table.keys() {
if let Some(package) = &resolution_graph.package_table.get(name) {
let value = package
.source_package
.package
.custom_properties
.get(&Symbol::from(PUBLISHED_AT_MANIFEST_FIELD));

if value.is_none() {
address_errors.push(AddressError::NoPublishedAddress(*name));
continue;
}

if value
.and_then(|v| ObjectID::from_str(v.as_str()).ok())
.is_none()
{
address_errors.push(AddressError::InvalidPublishedAddress(
*name,
value.unwrap().to_string(),
));
continue;
}
}
}
if !address_errors.is_empty() {
let mut error_messages = address_errors
.iter()
.map(|v| format!("{}", v))
.collect::<Vec<_>>();
error_messages.push(
"If this is intentional, you may use the --with-unpublished-dependencies flag to \
continue publishing these dependencies as part of your package (they won't be \
linked against existing packages on-chain)."
.into(),
);
return Err(SuiError::ModulePublishFailure {
error: error_messages.join("\n"),
});
}
Ok(())
}
1 change: 1 addition & 0 deletions crates/sui-framework/Move.toml
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
[package]
name = "Sui"
version = "0.0.1"
published-at = "0x2"

[dependencies]
# Using a local dep for the Move stdlib instead of a git dep to avoid the overhead of fetching the git dep in
Expand Down
Loading

0 comments on commit feb8654

Please sign in to comment.