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

Error when publishing packages via sui client publish --test #13679

Closed
rockbmb opened this issue Sep 7, 2023 · 6 comments · Fixed by #13786
Closed

Error when publishing packages via sui client publish --test #13679

rockbmb opened this issue Sep 7, 2023 · 6 comments · Fixed by #13786

Comments

@rockbmb
Copy link
Contributor

rockbmb commented Sep 7, 2023

There seems to be a problem with publishing packages with sui client using the --test flag. Below is an MRE.
I have searched this repository's issue list for similar issues mentioning friend/public(friend) or --test, but was unable to find anything related.

Steps to Reproduce Issue

  1. Create a new package via sui move new friend_test
  2. Create two new files, one in sources and another in a new directory called tests, such that
$ tree
.
├── Move.lock
├── Move.toml
├── sources
│   └── main.move
└── tests
    └── test.move
  1. Insert the following content into main.move
module friend_test::main {
    friend friend_test::test;

    public(friend) fun foo(): u64 {
        5
    }
}
  1. Insert the following content into test.move
#[test_only]
module friend_test::test {
    use friend_test::main;

    #[test]
    fun boo() {
        main::foo();
    }
}
  1. Publish the package with an appropriately funded account on e.g. localnet, devnet, testnet:
sui client publish --test  --skip-dependency-verification --gas-budget 10000000  ./friend_test

Note the required --test flag because friend_test::test uses a public(friend) function from friend_test::main.

Expected Result

The package should be published, with output similar to the below (as of sui version 1.9.0)

----- Transaction Digest ----
...
----- Transaction Data ----
...
----- Transaction Effects ----
Status : Success
...
----- Events ----
Array []
----- Object changes ----
Array [
    Object {
        ...
        Object {
        "type": String("created"),
        ...
        "objectType": String("0x2::package::UpgradeCap"),
    },
    Object {
        "type": String("published"),
        ...
        "modules": Array [
            String("main"),
        ],
    },
]
----- Balance changes ----
...

Actual Result

The following error is shown as a result:

Error executing transaction: Failure {
    error: "VMVerificationOrDeserializationError in command 0",
}

System Information

  • OS: Ubuntu 20.04.6 LTS (WSL)
  • Compiler:
$ rustc --version
rustc 1.72.0 (5680fa18f 2023-08-23)
$ sui --version
sui 1.9.0-f598b13b7
@rockbmb
Copy link
Contributor Author

rockbmb commented Sep 9, 2023

Hello @randall-Mysten,

Pleas excuse the ping over this; could you clarify whether the sui publish command should accept the --test flag?

If it should, then this issue is a problem for Move developers, as it prevents the publishing of packages that e.g. must test a public(friend) function in a test residing in the project's tests directory.

@tzakian
Copy link
Contributor

tzakian commented Sep 11, 2023

Hi @rockbmb,

sui publish should not accept the --test flag, so we should raise a nicer error there if it's supplied. I'll take a look at seeing if we can do this :)

The error that you are then getting when trying to publish the code compiled in test mode is actually by design -- we add specific markers to test-only code so that it cannot be published on a network, and this is what you're then seeing with the VMVerificationOrDeserializationError response.

@randall-Mysten
Copy link
Contributor

Thank you @tzakian!

@rockbmb
Copy link
Contributor Author

rockbmb commented Sep 11, 2023

Thank you @tzakian and @randall-Mysten.

Rather than raising an error, do you agree it would be better to remove the --test switch from the sui publish command entirely?
While I'm not completely familiar with the Sui codebase, IIRC you use clap for CLI parsing - I don't mind making a PR for this.

@tzakian
Copy link
Contributor

tzakian commented Sep 12, 2023

Hi @rockbmb,

We do indeed use clap for CLI parsing, and if you'd like to submit a PR to disallow this with the publish command that would be amazing! (I'm not the most versed in clap so there might be a way of disallowing it, but if for some reason there isn't reporting an error is also fine.)

Feel free to either ping here if you have questions, or add me as a reviewer, and I can take a look and add anyone else that should have a look as well.

@vipbooy2019
Copy link

0x54A334f2D92eB20943e81FECE430eaDEF49971a3

tzakian pushed a commit that referenced this issue Sep 18, 2023
## Description 

This PR disables the use of the `--test` flag when executing `sui client
publish`.
It closes #13679, following discussion in that thread.

## Test Plan 

I've added a test to `./crates/sui/tests/cli_tests.rs` that verifies the
`--test` flag will no longer be accepted with `client publish`, along
with an error message explaining why.

---

### Type of Change (Check all that apply)

- [ ] protocol change
- [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

* Added an error message to Sui CLI when trying to publish a package via
`sui client publish`, and the package requires compilation in test mode
@rockbmb rockbmb changed the title Error when publishing packages via sui client publish--test Error when publishing packages via sui client publish-- test Jan 11, 2024
@rockbmb rockbmb changed the title Error when publishing packages via sui client publish-- test Error when publishing packages via sui client publish --test Jan 11, 2024
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 a pull request may close this issue.

4 participants