Skip to content

Optimize wasm by default on contract build#2581

Merged
fnando merged 5 commits into
mainfrom
stellar-optimize-default
May 16, 2026
Merged

Optimize wasm by default on contract build#2581
fnando merged 5 commits into
mainfrom
stellar-optimize-default

Conversation

@fnando
Copy link
Copy Markdown
Member

@fnando fnando commented May 16, 2026

What

stellar contract build now runs wasm-opt on the generated wasm by default. The previous behaviour required passing --optimize. To skip optimization, pass --optimize=false.

When stellar-cli is built without the additional-libs feature (which gates wasm-opt, OS keyring, and Ledger hardware-wallet support), the build still succeeds with an un-optimized wasm and prints a warning suggesting reinstalling with --features additional-libs.

Why

Optimized wasm is what users almost always want to ship, and forgetting --optimize produced larger artifacts than necessary. Making it the default removes a footgun and matches what stellar contract optimize (already deprecated in favour of build --optimize) was encouraging.

Close #2565
Close #2284

Known limitations

N/A

Copilot AI review requested due to automatic review settings May 16, 2026 01:04
@fnando fnando requested a review from a team as a code owner May 16, 2026 01:04
@github-project-automation github-project-automation Bot moved this to Backlog (Not Ready) in DevX May 16, 2026
@claude
Copy link
Copy Markdown

claude Bot commented May 16, 2026

Claude finished @fnando's task in 2m 53s —— View job


Claude Code Review

  • Read changed files and understand the diff
  • Check build.rs changes (default optimize behavior, flag parsing)
  • Check Cargo.toml and feature gating changes
  • Check optimize.rs and doctor.rs changes
  • Verify FULL_HELP_DOCS.md matches the new behavior
  • Post review feedback

Overall this is a solid, well-scoped change. The intent is clear, the help text is updated, and the feature-gating cleanup is consistent across build.rs, optimize.rs, doctor.rs, and Cargo.toml. A few things worth considering before merging:

Suggestions / potential issues

1. --optimize parsing is mildly footgunny without require_equals = truebuild.rs:108-116

#[arg(
    long,
    default_value_t = true,
    default_missing_value = "true",
    num_args = 0..=1,
    action = clap::ArgAction::Set,
)]
pub optimize: bool,

With num_args = 0..=1 and no require_equals, clap will accept either --optimize=false or --optimize false (space-separated). The space-separated form has a subtle failure mode: if a user types --optimize immediately before another bareword (e.g. stellar contract build --optimize mypackage), clap will try to parse mypackage as a bool, fail, and emit a confusing "invalid value 'mypackage' for '--optimize '" error rather than the clearer "no value" semantics. Adding require_equals = true enforces the --optimize=false syntax that the help text already advertises and eliminates that ambiguity. Fix this →

2. BuildArgs::default() now silently changes semantics for programmatic callers

The new manual Default impl returns optimize: true, which is the right user-facing default but is also a behavior change for any in-tree callsite that builds BuildArgs via Default. I found two:

  • cmd/soroban-cli/src/commands/contract/upload.rs:182 — uses self.build_args.clone() (carries whatever was parsed from clap), not affected.
  • cmd/soroban-cli/src/commands/contract/deploy/wasm.rs:320build_args: build::BuildArgs::default() in the upload::Cmd constructed when the user passes --wasm <path> directly. In that code path resolve_wasm_paths short-circuits on self.wasm.is_some() so the optimize flag is never read, so this is harmless today — but it's a latent foot-gun if that branch ever changes. Worth a comment, or constructing BuildArgs { optimize: false, ..Default::default() } explicitly there since no build will occur.
  • cmd/soroban-cli/src/commands/contract/build.rs:220Cmd::default() now means "optimize = true" for direct programmatic use. Documenting this in the Default impl would help.

3. wasm-opt is now an unconditional dependency

Moving wasm-opt out of additional-libs adds non-trivial compile time and binary size to every build of soroban-cli, including stripped-down builds via --no-default-features. That's the intended trade-off per the PR description, but I'd recommend mentioning in the PR description (or a release note) that users who previously did cargo install --no-default-features to keep the binary small will now still pull in binaryen/wasm-opt source. No way around this if "optimize by default" must work without features — just calling out the implication.

4. doctor.rs "Wasm optimization" check is now a tautologydoctor.rs:209

fn check_optional_features(print: &Print) {
    print.checkln("Wasm optimization");
    ...
}

This line now unconditionally prints a green check regardless of build configuration, because wasm-opt is always compiled in. It's not wrong, but it's not really a "check" either — it conveys no information. Two options:

  • Remove the line entirely (wasm optimization is no longer an "optional feature" — it's a core feature).
  • Keep it under the Wasm section heading separate from check_optional_features, since it's no longer optional.

5. Removed error variant Error::OptimizeFeatureNotEnabledbuild.rs

This is a public enum variant of build::Error. Removing it is technically a breaking change for any external crate that pattern-matches on build::Error. soroban-cli doesn't publish a stable lib API by convention, so this is almost certainly fine, but worth noting if downstream crates exist.

6. Minor: missing regression test

A small unit test asserting BuildArgs::default().optimize == true (and ideally one that runs a parse of --optimize=false and verifies the flag flips to false) would catch accidental regressions of the new default. Optional but cheap.

Things I like

  • Cleanly removes all the #[cfg(feature = "additional-libs")] branches around optimization — much easier to reason about.
  • Help text is unambiguous about how to opt out (--optimize=false).
  • FULL_HELP_DOCS.md is regenerated consistently across build, deploy contract, and contract install/upload.
  • Manual Default impl is correctly hand-written since #[derive(Default)] would give optimize: false.

Nothing blocking

The changes are coherent and the user-facing behavior is a strict improvement. The clap require_equals and BuildArgs::default() semantics are the only items I'd flag as "consider before merge" — the rest is polish.
· Branch: stellar-optimize-default

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Makes wasm-opt always compiled in and turns --optimize on by default for stellar contract build (and deploy/upload which share BuildArgs). The additional-libs cargo feature continues to gate the OS keyring and Ledger hardware-wallet integrations only.

Changes:

  • BuildArgs::optimize becomes a bool flag defaulting to true (overridable via --optimize=false); related cfg gating, error variants, and the Default impl are updated accordingly.
  • wasm-opt is moved out of the additional-libs feature in Cargo.toml, and optimize.rs drops all cfg(feature = "additional-libs") gating and the Install error.
  • Doctor output unconditionally reports "Wasm optimization" as available; FULL_HELP_DOCS.md is regenerated to document the new --optimize <bool> flag.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
FULL_HELP_DOCS.md Regenerated help to reflect --optimize <OPTIMIZE> with true default.
cmd/soroban-cli/Cargo.toml Removes wasm-opt from additional-libs and makes it a non-optional dep.
cmd/soroban-cli/src/commands/contract/build.rs Unconditionally imports optimize, makes --optimize default to true, drops cfg-gated error paths, adds explicit Default impl.
cmd/soroban-cli/src/commands/contract/optimize.rs Removes all additional-libs cfg gating and the Install error since wasm-opt is always present.
cmd/soroban-cli/src/commands/doctor.rs Always reports Wasm optimization as available; removes it from the disabled-features list.

Comment thread cmd/soroban-cli/src/commands/contract/optimize.rs
@claude
Copy link
Copy Markdown

claude Bot commented May 16, 2026

Claude finished @fnando's task in 2m 8s —— View job


Claude Code Review (update)

  • Pull latest changes and diff against origin/main
  • Inspect what changed since the previous review (commits 2480771, a9187e7)
  • Verify which of the prior review items were addressed
  • Re-check build.rs, optimize.rs, doctor.rs, Cargo.toml, FULL_HELP_DOCS.md
  • Post updated review

What changed since the previous review

Commit 2480771 ("Drop tautological doctor check and document Default override.") addresses two items I flagged earlier:

  • Doctor tautology (prior Update README.md #4) — fixed. print.checkln("Wasm optimization") is gone from check_optional_features, so doctor no longer prints a useless always-green line. cmd/soroban-cli/src/commands/doctor.rs:208-223 now only deals with additional-libs-gated features, which reads cleanly.
  • BuildArgs::default() documentation (prior CLI: Invoke a contract in a local mock (e.g. MemHost) sandbox #2) — partially addressed. The new // Manual impl so optimize defaults to true… comment on cmd/soroban-cli/src/commands/contract/build.rs:119-120 is exactly the heads-up a future reader needs to keep #[derive(Default)] from being applied accidentally. Good fix.

Still open from prior review

  1. --optimize argument still allows space-separated valuecmd/soroban-cli/src/commands/contract/build.rs:109-115. Without require_equals = true, stellar contract build --optimize mypackage will try to parse mypackage as a bool and produce a confusing error rather than the cleaner "no value supplied" semantics. Help text advertises --optimize=false; enforcing it in the parser would match. Same suggestion as before. Fix this →

  2. build::BuildArgs::default() at cmd/soroban-cli/src/commands/contract/deploy/wasm.rs:320 is still harmless-but-latent. This callsite runs only when the user passes --wasm <path> directly; resolve_wasm_paths short-circuits before optimize is consulted, so the new optimize: true default never takes effect on that code path. It's safe today, but the Default comment you just added in build.rs is upstream — if you wanted belt-and-suspenders, BuildArgs { optimize: false, ..Default::default() } here would make the intent ("no build will happen, so optimize is irrelevant") obvious. Optional.

  3. No regression test for the new default. A two-line #[test] asserting BuildArgs::default().optimize == true (and another exercising --optimize=false via clap::Parser::try_parse_from) would pin the new behavior. Cheap and the only thing standing between you and an accidental revert. The existing test module is right there at cmd/soroban-cli/src/commands/contract/build.rs:823-852. Add tests →

  4. Removed Error::OptimizeFeatureNotEnabled variant — still a theoretical breaking change for any external consumer pattern-matching on build::Error. soroban-cli doesn't advertise a stable lib API, so almost certainly fine; flagging only because it's a public enum.

Re: Copilot's deprecation comment on optimize.rs

Copilot suggested adding a runtime deprecation warning to optimize::Cmd::run. Worth noting that the deprecation is wired into the help text already — cmd/soroban-cli/src/commands/contract/mod.rs:85 carries /// ⚠️ Deprecated, use + "build --optimize" + . Optimize a WASM file, which flows through to FULL_HELP_DOCS.md:97, 992. So users running stellar contract optimize --help see the deprecation. The question of whether to also print a runtime warning on the stdout/stderr of stellar contract optimize (or set hide = true on the variant) is a judgment call — I'd argue the help-text notice is sufficient for now and a hard-deprecate can come in a follow-up. Not a blocker either way.

Summary

Solid follow-up commit. The two items it addressed are clean fixes. The only remaining items I'd genuinely recommend before merge are #1 (require_equals) and #3 (a regression test for the default) — both are tiny and meaningfully protect the new behavior. Everything else is polish.
· Branch: stellar-optimize-default

@fnando
Copy link
Copy Markdown
Member Author

fnando commented May 16, 2026

Thanks for the review. Replies to each item:

  1. require_equals = true — Skipping. clap already validates bool values, so --optimize garbage fails with a clear "invalid value" error today; --optimize mypackage is not a real footgun because build takes no positionals. Adding require_equals would only restrict the space-separated form, which is a needless syntax restriction for a marginal error-message UX gain.

  2. BuildArgs::default() semantics — Addressed. Added a comment on the manual Default impl noting that optimize: true matches the CLI default and #[derive(Default)] would give false. The deploy/wasm.rs:320 callsite never reads optimize (the wasm is already built), so no behaviour change there.

  3. wasm-opt is now unconditional — Updated the PR description to call out that --no-default-features builds now also pull in wasm-opt/binaryen. This is the intended trade-off.

  4. doctor.rs tautology — Fixed. Removed the unconditional print.checkln("Wasm optimization") from check_optional_features. Wasm optimization is no longer an optional feature, so it does not belong in that listing.

  5. Removed Error::OptimizeFeatureNotEnabled variant — Not concerned. soroban-cli is a binary crate, not a stable library API, so internal error variants are not part of a contract.

  6. Regression test for BuildArgs::default().optimize == true — Skipping. Asserting the default in a unit test essentially restates the source line. A parse test for --optimize=false is meaningful but low-leverage for a single clap attribute. Manual verification with both forms passed.

@fnando fnando moved this from Backlog (Not Ready) to Needs Review in DevX May 16, 2026
@fnando fnando self-assigned this May 16, 2026
@leighmcculloch
Copy link
Copy Markdown
Member

wasm-opt out of additional-libs

I expect this breaks something. I think we should keep the additional lib inside additional libs. For the binary releases leaving it there will have no impact so no impact for most people.

@leighmcculloch
Copy link
Copy Markdown
Member

Copy link
Copy Markdown
Member

@leighmcculloch leighmcculloch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One comment otherwise lgtm 🎉

Comment thread cmd/soroban-cli/Cargo.toml Outdated
@claude
Copy link
Copy Markdown

claude Bot commented May 16, 2026

Claude finished @fnando's task in 3m 8s —— View job


Claude Code Review (update)

  • Pull latest changes and diff against origin/main
  • Inspect the two new commits (235c6ac, ea51f3f) that respond to leigh's feedback
  • Re-check build.rs, optimize.rs, Cargo.toml, doctor.rs, FULL_HELP_DOCS.md
  • Verify the warning path: behavior when additional-libs is disabled and --optimize is on
  • Post updated review

What changed since the previous review

235c6ac + ea51f3f reverse course on the most contentious part of the PR: wasm-opt no longer needs to leave additional-libs. The diff vs. origin/main is now just two files (build.rs + FULL_HELP_DOCS.md) — Cargo.toml, optimize.rs, and doctor.rs all match main exactly. Concretely:

  • additional-libs gating is restored: Cargo.toml no longer makes wasm-opt unconditional, optimize::Error::Install and the #[cfg(feature = "additional-libs")] branches in optimize.rs/doctor.rs are back as on main.
  • Hard fail → soft fail in build.rs: cmd/soroban-cli/src/commands/contract/build.rs:344-350 replaces the previously-removed Error::OptimizeFeatureNotEnabled with a print.warnln and continues the build. So --no-default-features builds with the new default-on optimize no longer error — they print a warning and produce an unoptimized wasm.
  • Warning wording was tightened in ea51f3f: "Optimization skipped: stellar-cli was installed without the additional-libs feature. Reinstall with --features additional-libs to enable." — concise and tells the user how to fix it. Good.

This is the right shape: binary releases (which bundle additional-libs) get optimize-by-default; toolchain-only builds, which historically couldn't optimize at all, now get a default-on flag that no-ops with a clear warning rather than a confusing error. It directly addresses the concern in #2284 (comment) — users on a default Rust toolchain still get a buildable CLI.

Items worth a look before merge

1. Warning is printed once per package, not once per runbuild.rs:344-350. The warning lives inside the for p in packages loop, so a workspace with N cdylib crates and --no-default-features will get N duplicate warning lines. Cheap fix: hoist a single check before the loop, e.g.

#[cfg(not(feature = "additional-libs"))]
if self.build_args.optimize {
    print.warnln("Optimization skipped: ...");
}
// then drop the per-iteration print

This also gives the user the warning before any compile work starts, so they can Ctrl-C and reinstall if they want optimization. Fix this →

2. Asymmetry between build --optimize and contract optimizeoptimize.rs:38-41 still returns Error::Install on --no-default-features builds, while the new code in build.rs only warns. Worth noting because the --optimize flag on build/deploy/upload and the standalone contract optimize subcommand now behave differently when the feature is missing. Defensible (the standalone command exists only to optimize, so failing is correct), but worth being intentional about. No change recommended — just calling out the divergence.

3. print.warnln is suppressed by --quietprint.rs:99-103. A CI script running stellar contract build --quiet on a --no-default-features install will silently produce unoptimized wasm with no indication. That's probably fine (the user asked for quiet, and they chose --no-default-features), but it's the kind of thing that bites someone six months from now wondering why their contracts grew. Not blocking.

4. Help text doesn't acknowledge feature-gated availability — The --optimize help (and FULL_HELP_DOCS.md) says "Enabled by default" without noting that on --no-default-features builds, "enabled" really means "warned and skipped." Edge case, and the runtime warning is clear. Optional polish.

Items I previously raised and now consider resolved

  • wasm-opt leaving additional-libs (leigh's concern + my prior Initial commit #3) — fully resolved.
  • doctor.rs tautology (prior Update README.md #4) — already resolved in 2480771, still good.
  • BuildArgs::default() semantics (prior CLI: Invoke a contract in a local mock (e.g. MemHost) sandbox #2) — documented; upload.rs:183 and deploy/wasm.rs:284,320 callsites verified safe (the wasm.rs:320 one constructs upload::Cmd only when --wasm <path> is supplied directly, so the build path with optimize: true is never entered; resolve_wasm_paths short-circuits before calling build_cmd.run).
  • require_equals on --optimize (prior CLI: Boilerplate #1) — author's call, and I agree on reflection: build/deploy/upload have no bareword positionals that could be greedily consumed (deploy's slop: Vec<OsString> uses last = true, so its positionals are after --). Skipping require_equals keeps --optimize false working for users who type it that way.
  • Regression test for the default (prior Contract Spec: Add CLI command that extracts XDR from .wasm. #6/Initial commit #3) — author's call.
  • Removed Error::OptimizeFeatureNotEnabled (prior Make clippy compliant #5) — now moot because the variant is gone and the failure mode is gone; replaced with a warning, no breaking enum change.

Summary

The pivot from "make wasm-opt unconditional" to "keep wasm-opt gated, warn instead of fail" is a clean fix that lands the user-visible win (optimize-by-default) without forcing binaryen into builds that can't tolerate it. The diff is now tightly scoped to build.rs and the regenerated help docs. Only thing I'd genuinely tweak is hoisting the per-package warning to a single warning (#1). Everything else is taste.

· Branch: stellar-optimize-default ·

Copy link
Copy Markdown
Member

@leighmcculloch leighmcculloch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎉

Comment thread cmd/soroban-cli/src/commands/contract/build.rs
@leighmcculloch
Copy link
Copy Markdown
Member

@claude
Copy link
Copy Markdown

claude Bot commented May 16, 2026

Claude finished @fnando's task in 1m 2s —— View job


Claude Code Review (update)

  • Diff current branch against origin/main to see what changed since the last review
  • Inspect the new commit d0a0e94 ("Note additional-libs requirement in --optimize help.")
  • Re-check build.rs and FULL_HELP_DOCS.md
  • Confirm prior open items still open vs. resolved
  • Post updated review

What changed since my last review

Only one new commit: d0a0e94 updates the --optimize help text (and the four corresponding entries in FULL_HELP_DOCS.md) to add ", Requires the additional-libs feature." That directly addresses the prior #4 ("Help text doesn't acknowledge feature-gated availability"). Concise wording, consistent across build, deploy contract, contract install, and contract upload. Nice fix.

Items I previously raised and now consider resolved

  • Update README.md #4 Help text didn't mention additional-libs requirement — fixed in d0a0e94. The --optimize help now reads: "Optimize the generated wasm. Enabled by default; pass --optimize=false to disable. Requires the additional-libs feature." (cmd/soroban-cli/src/commands/contract/build.rs:110, mirrored in the four FULL_HELP_DOCS.md locations).

Still open from the prior review

1. Per-package warning duplicationcmd/soroban-cli/src/commands/contract/build.rs:344-350. The print.warnln("Optimization skipped: ...") still sits inside the for p in packages loop (loop opens at build.rs:256), so a workspace with N cdylib crates will emit N identical warnings on a --no-default-features install. Hoisting this above the loop fires the warning once and surfaces it before any compile work starts, so a user who wanted optimization can Ctrl-C and reinstall instead of finishing N builds. Small change; I'd still recommend it. Fix this →

2. print.warnln is suppressed by --quietprint.rs:99-103. Same observation as before: a CI invocation of stellar contract build --quiet on a --no-default-features install will silently produce unoptimized wasm. Defensible (the user opted into both flags), just calling it out — no change recommended.

Items the author has explicitly declined and I won't keep raising

  • require_equals = true on --optimize (per fnando: build/deploy/upload take no positionals that could be greedily consumed — agree, leaving it)
  • Unit test for BuildArgs::default().optimize == true (per fnando: low leverage; manually verified)
  • BuildArgs::default() callsite belt-and-suspenders at deploy/wasm.rs:320 (harmless today; the manual Default impl is now documented in build.rs:119-120)
  • Runtime deprecation warning in optimize.rs (per fnando: already exists at cmd/soroban-cli/src/commands/contract/mod.rs:197; help-text deprecation already at mod.rs:85)

Summary

d0a0e94 is a clean, scoped help-text correction that resolves the last documentation gap I flagged. Approved-shape from my side, with the one remaining note (per-package warning hoist) being a minor polish item that I think is worth doing but isn't blocking. The PR description still claims Close #2565 and Close #2284, matching leigh's last comment — those linkages look correct given the current diff (gated wasm-opt preserved, optimize-by-default landed via warn-not-fail path).

· Branch: stellar-optimize-default ·

@fnando fnando enabled auto-merge (squash) May 16, 2026 06:38
@fnando fnando merged commit b17d3f0 into main May 16, 2026
211 of 212 checks passed
@fnando fnando deleted the stellar-optimize-default branch May 16, 2026 06:51
@github-project-automation github-project-automation Bot moved this from Needs Review to Done in DevX May 16, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

3 participants