Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates Movy’s Sui integration for the pending v1.65.2 upgrade by increasing default gas amounts, adding a compiled-package disk cache for faster Move builds, and extending the replay environment to support package upgrade replay.
Changes:
- Increased minted gas amounts / gas budgets used by deploy, fuzz, and replay helpers.
- Added on-disk caching for
SuiCompiledPackage::build_checkedand configured Move builds to use a shared install directory. - Implemented upgrade-aware local package loading in
movy-replayby replaying on-chain upgrade steps and applying a final local upgrade step.
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/movy/src/sui/fuzz.rs | Increases the minted gas amount used in fuzz setup. |
| crates/movy/src/sui/deploy.rs | Increases the minted gas amount used in deploy setup. |
| crates/movy-sui/src/compile.rs | Adds a disk cache for compiled packages and uses a shared Move install dir. |
| crates/movy-replay/src/exec.rs | Raises replay gas budgets and minted gas for PTB execution helpers. |
| crates/movy-replay/src/env.rs | Adds upgrade replay flow when local package indicates a published-at upgrade target. |
| .gitignore | Ignores *.bcs artifacts (used by the new compile cache). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let move_toml = folder.join("Move.toml"); | ||
| let content = std::fs::read(&move_toml) | ||
| .map_err(|e| eyre!("failed to read {}: {}", move_toml.display(), e))?; | ||
| let mut hasher = DefaultHash::default(); | ||
| hasher.update(content); |
There was a problem hiding this comment.
The compiled package disk cache key is derived only from Move.toml contents. This will return stale SuiCompiledPackage results when any .move source file (or Move.lock, dependency git revs, etc.) changes without a corresponding Move.toml change. Consider hashing all relevant inputs (e.g., Move.toml + Move.lock + all files under sources/ and tests/, or a build-config/version salt) so the cache invalidates correctly.
| let move_toml = folder.join("Move.toml"); | |
| let content = std::fs::read(&move_toml) | |
| .map_err(|e| eyre!("failed to read {}: {}", move_toml.display(), e))?; | |
| let mut hasher = DefaultHash::default(); | |
| hasher.update(content); | |
| // Build a cache key from all relevant inputs so we don't return stale | |
| // compiled packages when sources or dependencies change. | |
| let mut hasher = DefaultHash::default(); | |
| // Helper to hash a single file's contents into the hasher. | |
| fn hash_file(path: &Path, hasher: &mut DefaultHash) -> Result<(), MovyError> { | |
| let content = std::fs::read(path) | |
| .map_err(|e| eyre!("failed to read {}: {}", path.display(), e))?; | |
| hasher.update(content); | |
| Ok(()) | |
| } | |
| // Helper to recursively collect all `.move` files under a directory. | |
| fn collect_move_files(dir: &Path, acc: &mut Vec<PathBuf>) -> std::io::Result<()> { | |
| if !dir.exists() { | |
| return Ok(()); | |
| } | |
| for entry in std::fs::read_dir(dir)? { | |
| let entry = entry?; | |
| let path = entry.path(); | |
| if path.is_dir() { | |
| collect_move_files(&path, acc)?; | |
| } else if path | |
| .extension() | |
| .and_then(|ext| ext.to_str()) | |
| .map(|ext| ext.eq_ignore_ascii_case("move")) | |
| .unwrap_or(false) | |
| { | |
| acc.push(path); | |
| } | |
| } | |
| Ok(()) | |
| } | |
| // Always include Move.toml. | |
| let move_toml = folder.join("Move.toml"); | |
| hash_file(&move_toml, &mut hasher)?; | |
| // Include Move.lock if it exists. | |
| let move_lock = folder.join("Move.lock"); | |
| if move_lock.exists() { | |
| hash_file(&move_lock, &mut hasher)?; | |
| } | |
| // Include all `.move` source and test files in a deterministic order. | |
| let mut move_files: Vec<PathBuf> = Vec::new(); | |
| collect_move_files(&folder.join("sources"), &mut move_files) | |
| .map_err(|e| eyre!("failed to list sources directory: {}", e))?; | |
| collect_move_files(&folder.join("tests"), &mut move_files) | |
| .map_err(|e| eyre!("failed to list tests directory: {}", e))?; | |
| move_files.sort(); | |
| for path in move_files { | |
| hash_file(&path, &mut hasher)?; | |
| } |
| pub fn build_checked( | ||
| folder: &Path, | ||
| test_mode: bool, | ||
| with_unpublished: bool, | ||
| verify_deps: bool, | ||
| ) -> Result<SuiCompiledPackage, MovyError> { | ||
| if let Some(cached) = | ||
| load_compiled_package_cache(folder, test_mode, with_unpublished, verify_deps)? | ||
| { | ||
| return Ok(cached); | ||
| } | ||
|
|
There was a problem hiding this comment.
build_checked now has observable behavior changes (disk cache hit/miss, cache decode failure eviction), but the test module only exercises compilation. Adding a unit test that (1) compiles, (2) modifies a source file, and (3) asserts the cache is invalidated (or that a new key is used) would help prevent silent stale-cache regressions.
| let original_package_id = local_package_addresses | ||
| .first() | ||
| .copied() | ||
| .ok_or_else(|| eyre!("{} has no root modules after compilation", path.display()))?; |
There was a problem hiding this comment.
original_package_id is derived from the smallest address found in abi_result modules. When unpublished=true, this set commonly includes 0x0, so original_package_id can become ObjectID::ZERO and incorrectly trigger upgrade_mode, leading to filtering the upgrade payload to the wrong modules. Using the compiled root package id directly (e.g., abi_result.package_id) avoids this mis-detection.
| let original_package_id = local_package_addresses | |
| .first() | |
| .copied() | |
| .ok_or_else(|| eyre!("{} has no root modules after compilation", path.display()))?; | |
| let original_package_id = abi_result.package_id; |
| Identifier::from_str("package").unwrap(), | ||
| Identifier::from_str("commit_upgrade").unwrap(), | ||
| vec![], | ||
| vec![Argument::Input(0), upgrade_receipt], |
There was a problem hiding this comment.
The commit_upgrade call hard-codes Argument::Input(0) instead of reusing the cap_input returned by builder.obj(...). This works only as long as the first input remains the cap object; if input ordering changes (e.g., another pure/obj is added earlier), this will silently break. Reuse cap_input to make the call robust.
| vec![Argument::Input(0), upgrade_receipt], | |
| vec![cap_input, upgrade_receipt], |
upgrade
note: should upgrade sui with this pr. wtdcode/sui#7