Upgrade error formatting: missing from module (#19795)

## Description Add formatting to missing from module errors from upgrades. When a struct, enum, or function is missing from a module the relevant module definition is shown along with the specifics of what was missing. ## Test plan Snapshot testing --- ## 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:
MystenLabs · Oct 22, 2024 · 482bc5b · 482bc5b
1 parent 7e01df8
commit 482bc5b
Show file tree

Hide file tree

Showing 15 changed files with 234 additions and 87 deletions.
diff --git a/Cargo.lock b/Cargo.lock
diff --git a/Cargo.toml b/Cargo.toml
@@ -302,6 +302,7 @@ camino = "1.1.1"
 cfg-if = "1.0.0"
 chrono = { version = "0.4.26", features = ["clock", "serde"] }
 clap = { version = "4.4", features = ["derive", "wrap_help"] }
+codespan-reporting = "0.11.1"
 collectable = "0.0.2"
 colored = "2.0.0"
 color-eyre = "0.6.2"

diff --git a/crates/sui/Cargo.toml b/crates/sui/Cargo.toml
@@ -20,6 +20,7 @@ bin-version.workspace = true
 bip32.workspace = true
 camino.workspace = true
 clap.workspace = true
+codespan-reporting.workspace = true
 datatest-stable.workspace = true
 futures.workspace = true
 http.workspace = true
@@ -74,6 +75,7 @@ shared-crypto.workspace = true
 sui-replay.workspace = true
 sui-transaction-builder.workspace = true
 move-binary-format.workspace = true
+move-bytecode-source-map.workspace = true
 test-cluster.workspace = true
 
 fastcrypto.workspace = true

diff --git a/crates/sui/src/client_commands.rs b/crates/sui/src/client_commands.rs
@@ -908,7 +908,7 @@ impl SuiClientCommands {
                         previous_id,
                     )?;
                 }
-                let (package_id, compiled_modules, dependencies, package_digest, upgrade_policy) =
+                let (package_id, compiled_modules, dependencies, package_digest, upgrade_policy, _) =
                     upgrade_result?;
 
                 let tx_kind = client
@@ -1685,7 +1685,17 @@ pub(crate) async fn upgrade_package(
     with_unpublished_dependencies: bool,
     skip_dependency_verification: bool,
     env_alias: Option<String>,
-) -> Result<(ObjectID, Vec<Vec<u8>>, PackageDependencies, [u8; 32], u8), anyhow::Error> {
+) -> Result<
+    (
+        ObjectID,
+        Vec<Vec<u8>>,
+        PackageDependencies,
+        [u8; 32],
+        u8,
+        CompiledPackage,
+    ),
+    anyhow::Error,
+> {
     let (dependencies, compiled_modules, compiled_package, package_id) = compile_package(
         read_api,
         build_config,
@@ -1752,6 +1762,7 @@ pub(crate) async fn upgrade_package(
         dependencies,
         package_digest,
         upgrade_policy,
+        compiled_package,
     ))
 }
 

diff --git a/crates/sui/src/client_ptb/builder.rs b/crates/sui/src/client_ptb/builder.rs
@@ -1029,7 +1029,7 @@ impl<'a> PTBBuilder<'a> {
                     )
                     .map_err(|e| err!(path_loc, "{e}"))?;
                 }
-                let (package_id, compiled_modules, dependencies, package_digest, upgrade_policy) =
+                let (package_id, compiled_modules, dependencies, package_digest, upgrade_policy, _) =
                     upgrade_result.map_err(|e| err!(path_loc, "{e}"))?;
 
                 let upgrade_arg = self

diff --git a/...pgrade_errors/struct_missing_v1/Move.toml → ..._errors/declarations_missing_v1/Move.toml b/...pgrade_errors/struct_missing_v1/Move.toml → ..._errors/declarations_missing_v1/Move.toml
diff --git a/...uct_missing_v1/sources/UpgradeErrors.move → ...ons_missing_v1/sources/UpgradeErrors.move b/...uct_missing_v1/sources/UpgradeErrors.move → ...ons_missing_v1/sources/UpgradeErrors.move
@@ -9,5 +9,14 @@ module upgrades::upgrades {
     public struct StructToBeRemoved {
         b: u64
     }
+
+    public enum EnumToBeRemoved {
+        A,
+        B
+    }
+
+    public fun fun_to_be_removed(): u64 {
+        0
+    }
 }
 
diff --git a/...pgrade_errors/struct_missing_v2/Move.toml → ..._errors/declarations_missing_v2/Move.toml b/...pgrade_errors/struct_missing_v2/Move.toml → ..._errors/declarations_missing_v2/Move.toml
diff --git a/...uct_missing_v2/sources/UpgradeErrors.move → ...ons_missing_v2/sources/UpgradeErrors.move b/...uct_missing_v2/sources/UpgradeErrors.move → ...ons_missing_v2/sources/UpgradeErrors.move
@@ -5,7 +5,9 @@
 
 #[allow(unused_field)]
 module upgrades::upgrades {
-    // struct missing
+    // removed declarations
     // public struct StructToBeRemoved {}
+    // public enum EnumToBeRemoved {}
+    // public fun fun_to_be_removed() {}
 }
 
diff --git a/...it_tests/snapshots/sui__upgrade_compatibility__upgrade_compatibility_tests__all_fail.snap b/...it_tests/snapshots/sui__upgrade_compatibility__upgrade_compatibility_tests__all_fail.snap
diff --git a/...pshots/sui__upgrade_compatibility__upgrade_compatibility_tests__declarations_missing.snap b/...pshots/sui__upgrade_compatibility__upgrade_compatibility_tests__declarations_missing.snap
@@ -0,0 +1,30 @@
+---
+source: crates/sui/src/unit_tests/upgrade_compatibility_tests.rs
+expression: normalize_path(err.to_string())
+---
+error: struct is missing
+  ┌─ /fixtures/upgrade_errors/declarations_missing_v2/sources/UpgradeErrors.move:7:18
+  │
+7 │ module upgrades::upgrades {
+  │                  ^^^^^^^^ Module 'upgrades' expected struct 'StructToBeRemoved', but found none
+  │
+  = structs are part of a module's public interface and cannot be removed or changed during an upgrade, add back the struct 'StructToBeRemoved'.
+
+error: enum is missing
+  ┌─ /fixtures/upgrade_errors/declarations_missing_v2/sources/UpgradeErrors.move:7:18
+  │
+7 │ module upgrades::upgrades {
+  │                  ^^^^^^^^ Module 'upgrades' expected enum 'EnumToBeRemoved', but found none
+  │
+  = enums are part of a module's public interface and cannot be removed or changed during an upgrade, add back the enum 'EnumToBeRemoved'.
+
+error: public function is missing
+  ┌─ /fixtures/upgrade_errors/declarations_missing_v2/sources/UpgradeErrors.move:7:18
+  │
+7 │ module upgrades::upgrades {
+  │                  ^^^^^^^^ Module 'upgrades' expected public function 'fun_to_be_removed', but found none
+  │
+  = public functions are part of a module's public interface and cannot be removed or changed during an upgrade, add back the public function 'fun_to_be_removed'.
+
+
+Upgrade failed, this package requires changes to be compatible with the existing package. Its upgrade policy is set to 'Compatible'.
diff --git a/...ts/snapshots/sui__upgrade_compatibility__upgrade_compatibility_tests__struct_missing.snap b/...ts/snapshots/sui__upgrade_compatibility__upgrade_compatibility_tests__struct_missing.snap
diff --git a/crates/sui/src/unit_tests/upgrade_compatibility_tests.rs b/crates/sui/src/unit_tests/upgrade_compatibility_tests.rs
@@ -6,26 +6,25 @@ use insta::assert_snapshot;
 use move_binary_format::CompiledModule;
 use std::path::PathBuf;
 use sui_move_build::BuildConfig;
+use sui_move_build::CompiledPackage;
 
 #[test]
+#[should_panic]
 fn test_all_fail() {
-    let (pkg_v1, pkg_v2) = get_packages("all");
+    let (mods_v1, pkg_v2) = get_packages("all");
 
-    let result = compare_packages(pkg_v1, pkg_v2);
-    assert!(result.is_err());
-    let err = result.unwrap_err();
-
-    assert_snapshot!(err.to_string());
+    // panics: Not all errors are implemented yet
+    compare_packages(mods_v1, pkg_v2).unwrap();
 }
 
 #[test]
-fn test_struct_missing() {
-    let (pkg_v1, pkg_v2) = get_packages("struct_missing");
+fn test_declarations_missing() {
+    let (pkg_v1, pkg_v2) = get_packages("declarations_missing");
     let result = compare_packages(pkg_v1, pkg_v2);
 
     assert!(result.is_err());
     let err = result.unwrap_err();
-    assert_snapshot!(err.to_string());
+    assert_snapshot!(normalize_path(err.to_string()));
 }
 
 #[test]
@@ -42,12 +41,12 @@ fn test_entry_linking_ok() {
     assert!(compare_packages(pkg_v1, pkg_v2).is_ok());
 }
 
-fn get_packages(name: &str) -> (Vec<CompiledModule>, Vec<CompiledModule>) {
+fn get_packages(name: &str) -> (Vec<CompiledModule>, CompiledPackage) {
     let mut path: PathBuf = PathBuf::from(env!("CARGO_MANIFEST_DIR"));
     path.push("src/unit_tests/fixtures/upgrade_errors/");
     path.push(format!("{}_v1", name));
 
-    let pkg_v1 = BuildConfig::new_for_testing()
+    let mods_v1 = BuildConfig::new_for_testing()
         .build(&path)
         .unwrap()
         .into_modules();
@@ -56,10 +55,18 @@ fn get_packages(name: &str) -> (Vec<CompiledModule>, Vec<CompiledModule>) {
     path.push("src/unit_tests/fixtures/upgrade_errors/");
     path.push(format!("{}_v2", name));
 
-    let pkg_v2 = BuildConfig::new_for_testing()
-        .build(&path)
-        .unwrap()
-        .into_modules();
+    let pkg_v2 = BuildConfig::new_for_testing().build(&path).unwrap();
+
+    (mods_v1, pkg_v2)
+}
 
-    (pkg_v1, pkg_v2)
+/// Snapshots will differ on each machine, normalize to prevent test failures
+fn normalize_path(err_string: String) -> String {
+    //test
+    let re = regex::Regex::new(r"^  ┌─ .*(\/fixtures\/.*\.move:\d+:\d+)$").unwrap();
+    err_string
+        .lines()
+        .map(|line| re.replace(line, "  ┌─ $1").into_owned())
+        .collect::<Vec<String>>()
+        .join("\n")
 }