[CLI] Only use referenced dependencies in the linkage table #20509

stefan-mysten · 2024-12-04T22:56:12Z

Description

When a package is added as a dependency in the Move.toml manifest file, it will become part of the linkage table regardless if any code in that package is actually referenced in the code or not.

This PR fixes that by making sure that a package's linkage table contains only the dependencies that are actually referenced in the code by computing the list of dependencies from the modules' dependencies after all modules and their transitive dependencies are compiled rather than the manifest files.

Test plan

Note that now source validation will not allow to publish such a package (see below) because it cannot find the dependency on-chain.

Structure:
A_v1 -> published
A_v2 -> upgrade A_v1
B -> dependency on A_v2, but no code is called from A_v2.

Locally: After the change

{
  "data": {
    "package": {
      "modules": {
        "nodes": [
          {
            "name": "b",
            "package": {
              "address": "0xb185657f36ad8af5c84e95884f674747bcfffe5bc51b98053bfbf2c349ef0633",
              "linkage": [
                {
                  "originalId": "0x0000000000000000000000000000000000000000000000000000000000000001",
                  "upgradedId": "0x0000000000000000000000000000000000000000000000000000000000000001",
                  "version": 1
                },
                {
                  "originalId": "0x0000000000000000000000000000000000000000000000000000000000000002",
                  "upgradedId": "0x0000000000000000000000000000000000000000000000000000000000000002",
                  "version": 1
                }
              ]
            }
          }
        ]
      }
    }
  }
}

Before the change

{
  "data": {
    "package": {
      "modules": {
        "nodes": [
          {
            "name": "b",
            "package": {
              "address": "0x1f7f5733d11a5556cddcf73d4642351159b77aafe1b7c5d3d0b2e310f315d4e6",
              "linkage": [
                {
                  "originalId": "0x0000000000000000000000000000000000000000000000000000000000000001",
                  "upgradedId": "0x0000000000000000000000000000000000000000000000000000000000000001",
                  "version": 1
                },
                {
                  "originalId": "0x0000000000000000000000000000000000000000000000000000000000000002",
                  "upgradedId": "0x0000000000000000000000000000000000000000000000000000000000000002",
                  "version": 1
                },
                {
                  "originalId": "0x40adad4ef6d528484468adabe558d051a1d98cd9202de1f4ed3d71724f3f43c3",
                  "upgradedId": "0x922ac7f39faf25f030db32835219e7deb7b87b08f1a7703e1b6e5beb6b5a96f2",
                  "version": 2
                }
              ]
            }
          }
        ]
      }
    }
  }
}

B's source code is:

module b::b {
    use std::string::{Self, String};
    use a_v1::a_v1;

    public fun hello_default(ctx: &mut TxContext) {
        let name = string::utf8(b"hello");
    }

    //public fun test(ctx: &mut TxContext): a_v1::Object {
    //    a_v1::default(ctx)
    //}
}

B's Move.toml dependency list is:

[dependencies]
sui = { git = "https://github.com/mystenlabs/sui.git", subdir = "crates/sui-framework/packages/sui-framework", rev = "framework/testnet" }
a_v1 = { local = "../a_v2" }

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.

vercel · 2024-12-04T22:56:19Z

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	Jan 18, 2025 0:24am

3 Skipped Deployments

Name	Status	Preview	Updated (UTC)
multisig-toolkit	⬜️ Ignored (Inspect)	Visit Preview	Jan 18, 2025 0:24am
sui-kiosk	⬜️ Ignored (Inspect)	Visit Preview	Jan 18, 2025 0:24am
sui-typescript-docs	⬜️ Ignored (Inspect)	Visit Preview	Jan 18, 2025 0:24am

stefan-mysten · 2024-12-05T03:11:47Z

This PR breaks the following tests. Would have to look into them:

 TRY 4 FAIL [   0.897s] sui-core move_package_tests::package_digest_changes_with_dep_upgrades_and_in_sync_with_move_package_digest
 TRY 4 FAIL [   2.742s] sui-package-resolver tests::test_function_parameters
 TRY 4 FAIL [   2.010s] sui-package-resolver tests::test_relinking_layout
 TRY 4 FAIL [  23.935s] sui-source-validation tests::linkage_differs
 TRY 4 FAIL [  18.823s] sui-source-validation tests::successful_verification_upgrades
 TRY 4 FAIL [  16.837s] sui-source-validation tests::successful_verification_with_bytecode_dep

amnn

The implementation looks good, thanks @stefan-mysten. When it comes to updating the tests, I suspect many of them are now failing because before they were adding a dependency that was not referenced as a convenient short hand as part of the test set-up.

In most if not all cases the right thing to do is probably to introduce a reference to the non-existent dependency in the Move code, not expect that it will be stripped. I made a comment to that effect on the test that was already updated, but watch out for that when cleaning up the other tests too.

crates/sui-move-build/src/lib.rs

amnn · 2024-12-05T12:57:43Z

crates/sui/tests/cli_tests.rs

-        "{}",
-        err
-    );
+    // publishing with a dependency that is not referenced in code is OK, as the dependency list


It seems like in this case you need to update the test such that the dependency is actually referenced but does not exist, because that is what we're trying to test here.

Yes, but now the issue will be that source verification will trigger an error that it cannot find the on-chain version of this dependency which is a slightly different error than the one we had here with Dependency object does not exist or was deleted. FWIW, that Move package has set a random published-at field in the manifest.

It's okay that the error wording is different, the point is that we're testing the error case here.

…e mutable

… the local network that is running

stefan-mysten requested review from mdgeorge4153 and tzakian December 4, 2024 22:56

stefan-mysten temporarily deployed to sui-typescript-aws-kms-test-env December 4, 2024 22:56 — with GitHub Actions Inactive

stefan-mysten requested a review from amnn December 4, 2024 22:56

vercel bot deployed to Preview – sui-docs December 4, 2024 22:57 View deployment

stefan-mysten temporarily deployed to sui-typescript-aws-kms-test-env December 5, 2024 03:18 — with GitHub Actions Inactive

vercel bot deployed to Preview – sui-docs December 5, 2024 03:19 View deployment

amnn approved these changes Dec 5, 2024

View reviewed changes

stefan-mysten temporarily deployed to sui-typescript-aws-kms-test-env December 6, 2024 19:28 — with GitHub Actions Inactive

vercel bot deployed to Preview – sui-docs December 6, 2024 19:30 View deployment

stefan-mysten temporarily deployed to sui-typescript-aws-kms-test-env December 13, 2024 21:47 — with GitHub Actions Inactive

vercel bot deployed to Preview – sui-docs December 13, 2024 21:49 View deployment

stefan-mysten force-pushed the cli_publishing_deps branch from 68faf41 to 883ed0a Compare December 13, 2024 21:51

stefan-mysten temporarily deployed to sui-typescript-aws-kms-test-env December 13, 2024 21:51 — with GitHub Actions Inactive

vercel bot deployed to Preview – sui-docs December 13, 2024 21:52 View deployment

stefan-mysten temporarily deployed to sui-typescript-aws-kms-test-env December 13, 2024 22:30 — with GitHub Actions Inactive

vercel bot deployed to Preview – sui-docs December 13, 2024 22:33 View deployment

stefan-mysten added 11 commits January 17, 2025 14:56

Only use referenced dependencies in the linkage table

cb9c4db

Fixes

faa38c1

Improve code by removing clone on list and instead making the variabl…

89355a2

…e mutable

Try to fix tests?

21aefe4

Test

d39ff41

WIP

4ca0536

Fixes

c1aac20

Fix test

f6f53d1

Fixes

4de4e55

Fix test

1de9d7a

Fix

b69890c

Try to fix test by fixing the chain-id in the Move.lock to the one of…

7122a7f

… the local network that is running

stefan-mysten force-pushed the cli_publishing_deps branch from e3ef4ae to 7122a7f Compare January 18, 2025 00:13

stefan-mysten temporarily deployed to sui-typescript-aws-kms-test-env January 18, 2025 00:13 — with GitHub Actions Inactive

vercel bot deployed to Preview – sui-docs January 18, 2025 00:24 View deployment

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CLI] Only use referenced dependencies in the linkage table #20509

[CLI] Only use referenced dependencies in the linkage table #20509

stefan-mysten commented Dec 4, 2024 •

edited

Loading

vercel bot commented Dec 4, 2024 •

edited

Loading

stefan-mysten commented Dec 5, 2024 •

edited

Loading

amnn left a comment

amnn Dec 5, 2024

stefan-mysten Dec 6, 2024 •

edited

Loading

amnn Dec 9, 2024

[CLI] Only use referenced dependencies in the linkage table #20509

Are you sure you want to change the base?

[CLI] Only use referenced dependencies in the linkage table #20509

Conversation

stefan-mysten commented Dec 4, 2024 • edited Loading

Description

Test plan

Release notes

vercel bot commented Dec 4, 2024 • edited Loading

stefan-mysten commented Dec 5, 2024 • edited Loading

amnn left a comment

Choose a reason for hiding this comment

amnn Dec 5, 2024

Choose a reason for hiding this comment

stefan-mysten Dec 6, 2024 • edited Loading

Choose a reason for hiding this comment

amnn Dec 9, 2024

Choose a reason for hiding this comment

stefan-mysten commented Dec 4, 2024 •

edited

Loading

vercel bot commented Dec 4, 2024 •

edited

Loading

stefan-mysten commented Dec 5, 2024 •

edited

Loading

stefan-mysten Dec 6, 2024 •

edited

Loading