Conversation
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
There was a problem hiding this comment.
🟡 Changes recommended
Cleanliness checks can admit uncommitted archived files or prevent reruns, while permission hardening regresses config-file repair.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds reproducible source-archive generation for future verifiable contract builds.
Changes:
- Adds
stellar contract archivewith dry-run and hardened output. - Implements deterministic tar/gzip creation, ignore handling, and cleanliness checks.
- Adds unit, integration, and help documentation coverage.
File summaries
| File | Description |
|---|---|
FULL_HELP_DOCS.md |
Documents the archive command. |
cmd/soroban-cli/src/config/locator.rs |
Adds recursive permission hardening. |
cmd/soroban-cli/src/commands/contract/mod.rs |
Registers the command. |
cmd/soroban-cli/src/commands/contract/build/source_archive.rs |
Implements archive generation. |
cmd/soroban-cli/src/commands/contract/build.rs |
Exposes the archive module. |
cmd/soroban-cli/src/commands/contract/archive.rs |
Implements the CLI interface. |
cmd/soroban-cli/Cargo.toml |
Adds archive dependencies. |
cmd/crates/soroban-test/tests/it/build.rs |
Adds integration coverage. |
Cargo.lock |
Locks new dependencies. |
Review details
- Files reviewed: 8/9 changed files
- Comments generated: 4
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
🟡 Changes recommended
Cross-platform executable modes, submodule handling, and whole-archive memory buffering undermine reproducibility and reliability.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
cmd/soroban-cli/src/commands/contract/build/source_archive.rs:238
- This first materializes the entire uncompressed tar in memory, then
gzipallocates a secondVecfor the compressed archive. Since the walk intentionally permits unignoredtarget,node_modules, and large assets, valid inputs can require more than twice the archive size in RAM and terminate the CLI with OOM. Stream the tar throughGzEncoderinto a hashing/output writer (recording names during the walk for dry-run) instead of buffering both representations.
- Files reviewed: 8/9 changed files
- Comments generated: 3
- Review effort level: Balanced
| } | ||
|
|
||
| let mut builder = tar::Builder::new(Vec::new()); | ||
| builder.mode(tar::HeaderMode::Deterministic); |
|
|
||
| /// The set of tracked files under `source_root`, as paths relative to it. | ||
| fn tracked_files(source_root: &Path) -> Result<std::collections::HashSet<PathBuf>, Error> { | ||
| let out = run_git(source_root, &["ls-files", "-z"])?.unwrap_or_default(); |
| if !dirs.is_empty() { | ||
| print.warnln("Removed group/other access from config directories."); | ||
| } | ||
| if !files.is_empty() { | ||
| print.warnln("Removed group/other access from config files."); | ||
| } |
leighmcculloch
left a comment
leighmcculloch
left a comment
There was a problem hiding this comment.
Functionality looks sound, but I think we should put it in a different place. Deferring to you on that.
Otherwise just a couple minor help output and warning tweaks.
| /// sources) — the user owns the bytes they produce there. | ||
| pub(crate) fn ensure_clean_tree(source_root: &Path, print: &Print) -> Result<(), Error> { | ||
| if tree_is_dirty(source_root)? { | ||
| print.warnln(format!( |
There was a problem hiding this comment.
Because the error is returned I think we end up with a duplicate error printing do we?
|
|
||
| Build(build::Cmd), | ||
|
|
||
| /// Generate the reproducible source archive used by verifiable builds |
There was a problem hiding this comment.
This short line clobbers the larger doc comment that is on the Cmd type. We should remove this one, so that the larger one on the Cmd is visible in the help.
|
|
||
| - `--print-commands-only` — Print commands to build without executing them | ||
|
|
||
| ## `stellar contract archive` |
There was a problem hiding this comment.
I wonder if we should put this under the build command. The concept of archiving a contract is an overloaded term, since contract themselves can be archived by the network. I had to dig into the code to really understand what this new command was doing too.
So something like:
stellar contract build archive
Or:
stellar contract build --archive-only
There was a problem hiding this comment.
Actually, I think something even more verbose is probably even better:
stellar contract build create-source-archive
What
Adds a standalone
stellar contract archivecommand that generates — or inspects — a byte-reproducible source archive of the working tree. Split out of #2709 so it can be reviewed on its own; it's the shared foundation the--verifiablebuild is stacked on.The archive walks the working directory honoring the project's own
.gitignore/.ignorefiles (.gitis always skipped), so the same tree hashes to the samesource_sha256across machines. It refuses a dirty git tree, is written0600, and the--dry-runlisting is sanitized against hostile filenames.Why
SEP-58 verifies that a deployed WASM came from a specific source; this archive is the exact artifact its
source_sha256refers to.Known limitations
N/A