[move lockfile] rename dependency name field to id and add a separate name field to store manifest name

[kklas]

Description

As discussed in #14178 (comment) (second and third point), a few changes to the lockfile:

• the name field in dependencies is renamed to id to better reflect the meaning in the dependency graph (the packages are discriminated by their identifier, as resolved by the hook, which is not necessarily their manifest name)
• added a name field which will store the dependency manifest name (this is needed to show user-friendly error messages using the package manifest name instead of identifier which may be confusing)
• bumped lockfile version to 3

This is part of the work to enable compiling against on-chain dependencies #14178.

cc @rvantonder @amnn

Test plan

All changes are covered by existing unit tests.

---

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: Introduce lock file version 3, which renames a dependency's name field to id and introduces a separate name field that stores the package's name from its manifest for improved error reporting. Older lock files will be ignored when resolving dependencies so that this information is always available.
• Rust SDK:
• REST API:

[vercel]

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	Sep 20, 2024 4:03pm

3 Skipped Deployments

Name	Status	Preview	Comments	Updated (UTC)
multisig-toolkit	⬜️ Ignored (Inspect)	Visit Preview		Sep 20, 2024 4:03pm
sui-kiosk	⬜️ Ignored (Inspect)	Visit Preview		Sep 20, 2024 4:03pm
sui-typescript-docs	⬜️ Ignored (Inspect)	Visit Preview		Sep 20, 2024 4:03pm

[amnn]

Mostly looks good, thanks @kklas ! Just a couple of smaller (but important) questions. Also a thought: Would it make sense to make name optional if it already matches id? I can think of arguments on both sides.

[amnn]

Interesting -- first example of the id and name being different -- this wasn't the case I expected! Is it possible that this shouldn't be how things work in the presence of external resolution?

When we externally resolve a package, the name given to the dependency that invokes the external resolver is not meaningful to the final dependency graph and I wonder whether that's changed here.

[kklas]

There isn't external resolution or loading from lockfile happening here. name and id are different here because there is a special id resolution hook registered for the tests that when it encounters *-Rename for package name it will resolve its id to *-Resolved.

[kklas]

@amnn

Also a thought: Would it make sense to make name optional if it already matches id? I can think of arguments on both sides.

Hm I suppose we could just load name from id when it's None (and same for writing). It adds a few more lines of code, but what benefits do you see from this?

[amnn]

It adds a few more lines of code, but what benefits do you see from this?

I just noticed that in most cases the name and id will match, so having the name as an optional field will help call attention when it's there, but the downside is indeed the extra complexity to read the name, and the fact that the lock file is first and foremost a machine format, so these kind of legibility tricks don't strictly have a place there.

I'm not strongly attached to the idea, I was just curious to get your thoughts.

[kklas]

but the downside is indeed the extra complexity to read the name, and the fact that the lock file is first and foremost a machine format, so these kind of legibility tricks don't strictly have a place there.

Yes, I think there's no technical reason to have this so it's difficult to justify special casing the code like this which will make it more difficult to understand.

[kklas]

Is there anything else we need to do aside from bumping the version to 3? I'm getting missing field id for key move.dependencies when I run the new binary against a package that has a lockfile. Shouldn't it ignore / rewrite it since it's an old version?

[amnn]

Is there anything else we need to do aside from bumping the version to 3? I'm getting missing field id for key move.dependencies when I run the new binary against a package that has a lockfile. Shouldn't it ignore / rewrite it since it's an old version?

Ah good question -- the intention is typically that newer versions of the binary can read and understand older lock files. In this case, would it work to treat name like id if the lock file is a VERSION = 2 file?

[kklas]

In this case, would it work to treat name like id if the lock file is a VERSION = 2 file?

I think that makes sense. It would mean making id an optional field in schema.rs and adding back optional name in the [move.package] section and making id optional. A bit hacky but seems OK if there's a follow up to add versioned schemas (or perhaps I can do that here? it doesn't seem to difficult).

Another thing to consider is that we will have to ignore old lockfiles of dependencies once on-chain hooks land because it will lead to inconsistencies (e.g. duplicate modules of SuiFrameowork because it's being resolved as 0x2 in root but as SuiFramework in a dependency from a lockfile). So perhaps it would be a good idea to add a resolver_version field or something to the lockfile here for that?

[amnn]

It sounds to me like the easiest of those options is the one you originally suggested, given the issues when the on-chain hook gets enabled (namely that if we see a lock file of an older version, we ignore it, and recompute it) -- if we made things backwards compatible today, we are only kicking the can down the road, same for adding a versioned schema, which is not difficult, but wouldn't be useful once the package hook lands. I don't think we want to add a different resolver_version though -- the existing version field should suffice.

[kklas]

Yep, makes sense! I made the resolver ignore the lockfile when it's lower version and bump the version when the dependency graph is updated. Changed the VERSION const to u16 because the toml library doesn't support u64 (only i64).

[amnn]

This looks good to me, thanks @kklas !

[amnn]

Landed -- thanks @kklas !