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

[cli] Implement upgrade compatibility checks client side #19562

Conversation

jordanjennings-mysten
Copy link
Contributor

@jordanjennings-mysten jordanjennings-mysten commented Sep 25, 2024

Description

Introduce client side upgrade compatibility checking, allowing users to check before TX submission if an upgrade will be successful. Improves the output of errors by relying on the move-binary-format crates checks to list the issues with the error to the user that are found.

Test plan

manually tested see comment below


Release notes

  • Protocol:
  • Nodes (Validators and Full nodes):
  • Indexer:
  • JSON-RPC:
  • GraphQL:
  • CLI: The CLI displays a different error when an upgrade error is thrown that includes the details of each error.
  • Rust SDK:
  • REST API:

Copy link

vercel bot commented Sep 25, 2024

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 Oct 7, 2024 4:27am
3 Skipped Deployments
Name Status Preview Comments Updated (UTC)
multisig-toolkit ⬜️ Ignored (Inspect) Visit Preview Oct 7, 2024 4:27am
sui-kiosk ⬜️ Ignored (Inspect) Visit Preview Oct 7, 2024 4:27am
sui-typescript-docs ⬜️ Ignored (Inspect) Visit Preview Oct 7, 2024 4:27am

@jordanjennings-mysten
Copy link
Contributor Author

jordanjennings-mysten commented Sep 25, 2024

Tests

Still thinking about test strategy however I have run through most tests manually.

UpgradeErrorsV1.move

/// Module: UpgradeErrors

#[allow(unused_field)]
module upgrades::upgrades {
    // struct missing
    public struct StructToBeRemoved {
        b: u64
    }

    // struct ability mismatch (add)
    public struct StructAbilityMismatchAdd {}

    // struct ability mismatch (remove)
    public struct StructAbilityMismatchRemove has copy {}

    // struct ability mismatch (change)
    public struct StructAbilityMismatchChange has copy {}

    // struct type param mismatch
    public struct StructTypeParamMismatch<S, T> { a: S }

    // struct field mismatch (add)
    public struct StructFieldMismatchAdd {
        a: u64,
        b: u64
    }

    // struct field mismatch (remove)
    public struct StructFieldMismatchRemove {
        a: u64,
        b: u64
    }

    // struct field mismatch (change)
    public struct StructFieldMismatchChange {
        a: u64,
        b: u64
    }

    // enum missing
    public enum EnumToBeRemoved {
        A,
        B
    }

    // enum ability mismatch (add)
    public enum EnumAbilityMismatchAdd  {
        A,
    }

    // enum ability mismatch (remove)
    public enum EnumAbilityMismatchRemove has copy {
        A,
    }

    // enum ability mismatch (change)
    public enum EnumAbilityMismatchChange has copy {
        A,
    }

    // enum new variant
    public enum EnumNewVariant {
        A,
        B,
        C
    }

    // enum variant missing
    public enum EnumVariantMissing {
        A,
        B,
    }

    // function missing public
    public fun function_to_have_public_removed() {}

    // function missing friend
    public(package) fun function_to_have_friend_removed() {}

    // function missing entry


    // function signature mismatch (add)
    public fun function_add_arg() {}

    // function signature mismatch (remove)
    public fun function_remove_arg(a: u64) {}

    // function signature mismatch (change)
    public fun function_change_arg(a: u64) {}
}

Upgrade to -> UpgradeErrorsV2.move

/// Module: UpgradeErrors

#[allow(unused_field)]
module upgrades::upgrades {
    // struct missing
    // public struct StructToBeRemoved {}

    // struct ability mismatch (add)
    public struct StructAbilityMismatchAdd has copy {} // added the copy ability where none existed

    // struct field mismatch (remove)
    public struct StructAbilityMismatchRemove {} // removed the copy ability

    // struct field mismatch (change)
    public struct StructAbilityMismatchChange has drop {} // changed from drop to copy

    // struct type param mismatch
    public struct StructTypeParamMismatch<T> { a: T } // changed S to T

    // struct field mismatch (add)
    public struct StructFieldMismatchAdd {
        a: u64,
        b: u64,
        c: u64, // added
    }

    // struct field mismatch (remove)
    public struct StructFieldMismatchRemove {
        a: u64,
        // removed b: u64
    }

    // struct field mismatch (change)
    public struct StructFieldMismatchChange {
        a: u64,
        b: u8 // changed b from u64 to u8
    }

    // enum missing
    // public enum EnumToBeRemoved {}

    // enum ability mismatch (add)
    public enum EnumAbilityMismatchAdd has copy {
        A,
    }

    // enum ability mismatch (remove)
    public enum EnumAbilityMismatchRemove {
        A,
    }

    // enum ability mismatch (change)
    public enum EnumAbilityMismatchChange has drop {
        A,
    }

    // enum new variant
    public enum EnumNewVariant {
        A,
        B,
        C,
        D // new variant
    }

    // enum variant missing
    public enum EnumVariantMissing {
        A,
        // remove B,
    }

    // function missing public
    fun function_to_have_public_removed() {}

    // function missing friend
    fun function_to_have_friend_removed() {}

    // function missing entry

    // function signature mismatch (add)
    public fun function_add_arg(a: u64) {}

    // function signature mismatch (remove)
    public fun function_remove_arg() {}

    // function signature mismatch (change)
    public fun function_change_arg(a: u8) {} // now has u8 instead of u64
}

Output

Upgrade compatibility check failed with the following errors:
- Struct ability mismatch: StructAbilityMismatchAdd
  Old: []
  New: [Copy, ]
- Struct ability mismatch: StructAbilityMismatchChange
  Old: [Copy, ]
  New: [Drop, ]
- Struct ability mismatch: StructAbilityMismatchRemove
  Old: [Copy, ]
  New: []
- Struct field mismatch: StructFieldMismatchAdd
  Old: [Field { name: Identifier("a"), type_: U64 }, Field { name: Identifier("b"), type_: U64 }]
  New: [Field { name: Identifier("a"), type_: U64 }, Field { name: Identifier("b"), type_: U64 }, Field { name: Identifier("c"), type_: U64 }]
- Struct field mismatch: StructFieldMismatchChange
  Old: [Field { name: Identifier("a"), type_: U64 }, Field { name: Identifier("b"), type_: U64 }]
  New: [Field { name: Identifier("a"), type_: U64 }, Field { name: Identifier("b"), type_: U8 }]
- Struct field mismatch: StructFieldMismatchRemove
  Old: [Field { name: Identifier("a"), type_: U64 }, Field { name: Identifier("b"), type_: U64 }]
  New: [Field { name: Identifier("a"), type_: U64 }]
- Struct missing: StructToBeRemoved
- Struct type param mismatch: StructTypeParamMismatch
  Old: [DatatypeTyParameter { constraints: [], is_phantom: false }, DatatypeTyParameter { constraints: [], is_phantom: false }]
  New: [DatatypeTyParameter { constraints: [], is_phantom: false }]
- Enum ability mismatch: EnumAbilityMismatchAdd
  Old: []
  New: [Copy, ]
- Enum ability mismatch: EnumAbilityMismatchChange
  Old: [Copy, ]
  New: [Drop, ]
- Enum ability mismatch: EnumAbilityMismatchRemove
  Old: [Copy, ]
  New: []
- Enum new variant: EnumNewVariant
  Old: [Variant { name: Identifier("A"), fields: [] }, Variant { name: Identifier("B"), fields: [] }, Variant { name: Identifier("C"), fields: [] }]
  New: [Variant { name: Identifier("A"), fields: [] }, Variant { name: Identifier("B"), fields: [] }, Variant { name: Identifier("C"), fields: [] }, Variant { name: Identifier("D"), fields: [] }]
- Enum missing: EnumToBeRemoved
  Enum { abilities: [], type_parameters: [], variants: [Variant { name: Identifier("A"), fields: [] }, Variant { name: Identifier("B"), fields: [] }] }
- Enum variant missing: EnumVariantMissing
  Variant { name: Identifier("B"), fields: [] }
- Function signature mismatch: function_add_arg
  Old:
    Params: []
    Return: []
  New:
    Params: [U64]
    Return: []
- Function signature mismatch: function_change_arg
  Old:
    Params: [U64]
    Return: []
  New:
    Params: [U8]
    Return: []
- Function signature mismatch: function_remove_arg
  Old:
    Params: [U64]
    Return: []
  New:
    Params: []
    Return: []
- Function lost friend visibility: function_to_have_friend_removed
- Function lost public visibility: function_to_have_public_removed

@jordanjennings-mysten jordanjennings-mysten changed the title Implement upgrade compatibility checks client side (cli) Implement upgrade compatibility checks client side Sep 25, 2024
@jordanjennings-mysten jordanjennings-mysten changed the title (cli) Implement upgrade compatibility checks client side [cli] Implement upgrade compatibility checks client side Sep 25, 2024
Copy link
Member

@amnn amnn left a comment

Choose a reason for hiding this comment

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

The core of this change looks great -- one thing to note is that we do still need to pay attention to which checks are enabled in the Compatibility struct when we finish, but otherwise this logic looks good.

Main thing to work on before landing is to polish the content of error messages (and figure out the testing strategy).

crates/sui/src/client_commands.rs Outdated Show resolved Hide resolved
crates/sui/src/upgrade_compatibility.rs Outdated Show resolved Hide resolved
crates/sui/src/upgrade_compatibility.rs Outdated Show resolved Hide resolved
crates/sui/src/upgrade_compatibility.rs Outdated Show resolved Hide resolved
crates/sui/src/upgrade_compatibility.rs Outdated Show resolved Hide resolved
crates/sui/src/upgrade_compatibility.rs Show resolved Hide resolved
crates/sui/src/upgrade_compatibility.rs Outdated Show resolved Hide resolved
Compatibility::upgrade_check().check_with_mode::<CliCompatibilityMode>(
&Module::new(&existing_module),
&Module::new(new_module),
)?;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

pass in OLD then NEW. fixes the issue we saw last week, falls under the tests.

Copy link
Member

@amnn amnn left a comment

Choose a reason for hiding this comment

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

Thanks @jordanjennings-mysten -- comments are mostly clean-ups, but the most important one to address is the one about dependencies to Sui in test packages -- we should use a "local" dep for that, rather than a "git" dep to avoid making CI times longer.

Otherwise this is good to go, accepting to unblock!

crates/sui/Cargo.toml Outdated Show resolved Hide resolved
crates/sui/src/unit_tests/upgrade_compatibility_tests.rs Outdated Show resolved Hide resolved
@jordanjennings-mysten jordanjennings-mysten enabled auto-merge (squash) October 7, 2024 04:51
@jordanjennings-mysten jordanjennings-mysten merged commit f23da66 into MystenLabs:main Oct 7, 2024
45 checks passed
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