move: automated address mgmt for pkg upgrades #17931
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
3 Ignored Deployments
|
| let property = published_at_property(package); | ||
| let property = resolve_published_id(package, chain_id.clone()); |
There was a problem hiding this comment.
Main top-level change: previously we resolved the published_at property only from the manifest. Now we do so from either the Move.lock or manifest in the resolve_published_id function.
| let mut buf = String::new(); | ||
| move_toml.read_to_string(&mut buf).unwrap(); | ||
|
|
||
| // Add a `published-at = "0x<package_object_id>"` to the Move manifest. | ||
| let mut lines: Vec<String> = buf.split('\n').map(|x| x.to_string()).collect(); | ||
| let idx = lines.iter().position(|s| s == "[package]").unwrap(); | ||
| lines.insert( | ||
| idx + 1, | ||
| format!( | ||
| "published-at = \"{}\"", | ||
| package.reference.object_id.to_hex_uncompressed() | ||
| ), | ||
| ); | ||
| let new = lines.join("\n"); | ||
| move_toml.write_at(new.as_bytes(), 0).unwrap(); | ||
|
|
There was a problem hiding this comment.
This test test_package_management_on_upgrade_command previously performed an upgrade to check that we are writing the correct package management info to Move.lock. It previously used the same logic, relying on Move.toml to have the info.
Now, we remove the reliance on Move.toml to perform the upgrade in this test (and it ought to still succeed).
| // Purposely add a conflicting `published-at` address to the Move manifest. | ||
| lines.insert(idx + 1, "published-at = \"0xbad\"".to_string()); |
There was a problem hiding this comment.
Main logic for this test: put a bogus address in Move.toml and expect the error message.
| "Conflicting published package address: `Move.toml` contains published-at address \ | ||
| {id_manifest} but `Move.lock` file contains published-at address {id_lock}. \ | ||
| You may want to: | ||
|
|
||
| - delete the published-at address in the `Move.toml` if the `Move.lock` address is correct; OR | ||
| - update the `Move.lock` address using the `sui manage-package` command to be the same as the `Move.toml`; OR | ||
| - check that your `sui active-env` {env_alias} corresponds to the chain on which the package is published (i.e., devnet, testnet, mainnet); OR | ||
| - contact the maintainer if this package is a dependency and request resolving the conflict." |
There was a problem hiding this comment.
Open to ideas or comments on this error message. One part of me wants to cover the most obvious things that could go wrong, and another part of me wants to direct the user to the docs and just say something like "conflicting addresses error, check the docs on how to resolve this".
There was a problem hiding this comment.
Not needing to read the docs is a plus in some way. Doc links also get stale, so my opinion is to go with having here an explicit explanation on how to address this issue error.
Regarding the message itself, after I read it I am slightly conflicted which one to try first. Perhaps the sui manage-package could be the first line of defense. If a command can do it for me, that's what I would try.
There was a problem hiding this comment.
Right, fully agree, doc links are not the way to go. The idea was more of a soft "refer to the docs online for more information" rather than a specific link.
|
thanks for your enthusiasm @stefan-mysten! There is a snag and I need to update some things in this PR, but we are indeed close!! |
Cool, let me know when that's fixed and I will get a review in! |
Description
This PR adds automated address management support for
sui client upgrade. I'm building this out iteratively so that it's easier to reason about and ensure backwards-compatibility during the transition when a package does not yet have info tracking inMove.lock(i.e., supporting packagepublishwill come next).High-level:
If a package contains a
Move.lockthat contains info to support a package upgrade, then that is used, and a user does not have to (nor should they) maintain apublished-ataddress in theirMove.toml.If a package does not have any
Move.lockinformation to support a package upgrade, we will still fallback toMove.tomlas we do today. Nothing changes.If a package has both a
Move.lockandMove.toml, then theMove.lockwill be used (the user is free to deletepublished-atin theirMove.toml). Caveat: If a published address is in both of these files, they must be the same (we will raise aConflicterror message otherwise).Note after this PR it is still required to set
[addresses]to0x0inMove.toml. This requirement will be removed afterpublishsupport is added (which also requires setting to0x0still). I didn't want to meddle with that logic all at once.Test plan
How did you test the new or updated feature?
We cover three cases:
Move.tomlonly (existing package upgrade test)Move.lockonly (extension of existing package management test)Move.lockandMove.tomlwith conflicting addresses error condition (new test)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.