sui
sui copied to clipboard
[move] Option to verify on-chain dependencies on publish
Adds the option to verify a Move package's dependencies against their on-chain counterparts on publish.
Test Plan
Newly added tests:
sui-source-validation$ cargo nextest run
Or by running sui client publish --verify-dependencies ...
.
Supersedes #4975
This PR adds the following changes (see last commit) relative to #4975:
All outstanding comments addressed and some additional things noticed after adding tests for the happy path and every error case:
Fixing Correctness Issues:
- Make compatible with
simtest
. - Don't take
CompiledPackage
by value to verify it, take it by reference. - Change logic around addresses -- the check for visiting addresses was handled per-module which was not correct -- an address corresponds to a package, and can have multiple modules, and we should visit all of them. The check can be simplified to skipping the verification for the zero address (the current package).
- Don't key modules solely by their package name and module name when identifying missing modules -- need to include their address (or at least replace package name with address) (this is fixed by changing the structure of the main loop body, see last bullet in "Coding Style/Idioms").
Behavioural changes:
- Format errors using
thiserror
. - Add flag to
PublishCommand
to opt-in to verification (temporary while we stabilise it, to allow people including/especially Movey team to try it out). - Report
LocalDependencyNotFound
instead ofModuleCountMismatch
, to matchOnChainDependencyNotFound
.
Improving Performance:
- Avoid the linear scan through verified dependencies.
- Avoid converting
Symbol
toString
, useas_ref
when a&str
is required. - Avoid unnecessary clones and
mut
s. - Use Map
Entry
APIs to avoid an extra lookup when testing for containment and adding an empty value.
Coding Style/Idioms:
- Use
let/else
ormap_err
where possible. - Favour explicit field names for errors, rather than documenting field names in comments.
- Get rid of unnecessary dependencies.
- Implement the main body of verification as two loops, consuming a dictionary of local modules and a dictionary of on-chain modules, which produces an error if there is an unmatched entry in either by the end of the loop.
Stacked on #6246 (first two commits)
Thoughts on making dep verification the default? Seems like a good security measure
Yes, we definitely should enable it as the default. My plan was to have a couple of devnet versions where it was available but not always on, and advertise it so people try it.
If it generally works well, (low false positive rate is the main thing I'm looking for, and a zero false negative rate, but that should be a given), we can invert the behaviour of the flag to --skip-dependency-verification
.
This should unblock us to let the Movey team (and anyone else who is keen) try the feature out while we build out the docs and supporting tools, which @oxade is going to make a start on, on top of this PR).