refactor: Clean up generate_default_deployment_with_cache() - #2396
Draft
jbencin-stacks wants to merge 19 commits into
Draft
refactor: Clean up generate_default_deployment_with_cache()#2396jbencin-stacks wants to merge 19 commits into
generate_default_deployment_with_cache()#2396jbencin-stacks wants to merge 19 commits into
Conversation
Replace the four-line helper with `entry().or_default().push()` at the three call sites. Single hashmap lookup instead of two, no `unwrap`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The Vec<String> at the tail of generate_default_deployment_with_cache was built but never returned, logged, or otherwise consumed. Observable behavior is unchanged — failure already propagates through artifacts.success. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Call sites now name the option (`BatchingMode::Single` / `BatchingMode::Chunked`) instead of passing a bare bool. The chain limit (25 chunked, 100_000 single) lives on the enum so the magic numbers are colocated with their meaning. The CLI flag itself stays `pub no_batch: bool` since that's idiomatic for clap; conversion happens at the boundary. The clarinet-cli wrapper drops its unused `_no_batch` parameter — callers already pass a real value at the wrapper layer. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Pull the per-network (stacks-node, bitcoin-node) URL resolution into its own function. Devnet/Testnet/Mainnet each had a ~10-line block inside the main function; isolating them shrinks the caller by ~40 lines and removes a borrow-by-move pattern (the testnet/mainnet branches were calling `.unwrap_or(...)` on owned fields of network_manifest, which left it partially-moved). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The per-epoch chunking loop becomes a one-liner at the call site, and the `id` field is now derived from `batches.len()` rather than a parallel counter. The `if !txs.is_empty()` guard is dropped — chunks() never yields empty slices. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Collapse the 25-line accessor-vs-disk dispatch into a single helper. The disk branch becomes an iterator chain that collects into the target HashMap directly instead of building it via a mutable loop. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Pulls the ~60-line custom boot-contract validation block out of the main function. The helper returns failure diagnostics by id so the caller can extend its running map directly. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The ~200-line requirements queue+resolution loop is now its own
function. The helper takes the four downstream state collections by
`&mut` (so the pre-loaded boot ASTs flow in naturally as the seed for
dependency detection) and uses `&dyn FileAccessor` for the I/O.
Side cleanups inside the moved code:
- Replace the three `if matches!(network, X) { ... } else if matches!(network, Y) { ... }` chains with a single `match network`.
- Hoist the sBTC-mainnet-to-testnet remap into a `let (..., ...) = if {} else {}` to drop the duplicated remap_principals build.
- Use `BTreeMap::from([(k, v)])` for the one-entry maps.
- Pre-condition `requirements.is_some()` check moved inside via let-else so the caller no longer needs to gate the call.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The per-iteration cache-or-rebuild dispatch moves out of the parent function. The loop body collapses from ~70 lines to 14, and the two paths (cache hit, cache miss, no cache) sit side-by-side in the helper. Behavior is preserved: cache hits replay the original diagnostics and ast_success flag. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The 85-line loop that turns each manifest entry into a (ClarityContract, location) pair plus a publish TransactionSpecification becomes its own function. The caller's remaining lines just feed those into the AST-cache loop and the deployment plan. The "did we strip an env(simnet) marker" flag now flows out via the return tuple instead of mutating an enclosing local — easier to reason about and isolates the helper from the parent's state. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The accessor-or-disk match for NetworkManifest mirrors the pattern already extracted in load_project_contract_sources. Same shape, same treatment. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Pull the post-AST topological-ordering + per-contract transaction push into its own function. The (source, location) pair shared by both publish variants is now read once into a tuple instead of duplicating the insert in each match arm. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Two small but well-isolated chunks pulled out: - `build_session_settings` covers SessionSettings construction, including the simnet-only-filter for override_boot_contracts_source. - `load_boot_contracts` seeds `requirements_data` with boot-contract ASTs and returns the id set, short-circuiting cleanly when simnet uses remote data. Minor inline cleanup: load_boot_contracts inserts directly into the caller's `requirements_data` instead of building a temporary BTreeMap and `.append`ing it. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The accounts → wallets conversion becomes a tidy collect over an iterator with `?` for the address parse, instead of a mut-vec push loop with a manual `map_err`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- `Result::unwrap_or_else` replaces a `match { Ok(x) => x, Err((x, _)) => x }` block.
- `dependencies.extend(...)` replaces a loop that inserts one entry at a time, dropping a per-iteration `.clone()` of the contract id.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
jbencin-stacks
commented
May 18, 2026
jbencin-stacks
commented
May 18, 2026
jbencin-stacks
commented
May 18, 2026
jbencin-stacks
commented
May 18, 2026
jbencin-stacks
commented
May 18, 2026
jbencin-stacks
commented
May 18, 2026
jbencin-stacks
commented
May 18, 2026
jbencin-stacks
commented
May 18, 2026
jbencin-stacks
commented
May 18, 2026
Contributor
There was a problem hiding this comment.
Pull request overview
Refactors clarinet-deployments’s default deployment generation logic by decomposing generate_default_deployment_with_cache() into smaller helpers and replacing the no_batch boolean with an explicit BatchingMode API, then propagates that API change through the CLI, LSP, WASM SDK, and benches.
Changes:
- Introduce
BatchingMode(ChunkedvsSingle) and update all call sites to use it instead ofno_batch: bool. - Split the previous large deployment-generation function into focused helpers (requirements resolution, source loading, batching, wallet building, etc.).
- Preserve AST cache reuse behavior while simplifying the AST build/reuse flow.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| components/clarity-lsp/src/common/state.rs | Update deployment generation call to pass BatchingMode::Chunked. |
| components/clarity-lsp/benches/ast_cache.rs | Update bench call sites/imports for BatchingMode. |
| components/clarinet-sdk-wasm/src/core.rs | Update WASM SDK deployment generation call to use BatchingMode. |
| components/clarinet-deployments/src/lib.rs | Core refactor: introduce BatchingMode, extract helpers, and rework deployment generation flow. |
| components/clarinet-cli/src/frontend/dap.rs | Update DAP flow to pass BatchingMode::Chunked. |
| components/clarinet-cli/src/frontend/cli.rs | Map --no-batch to BatchingMode::Single and update other call sites to BatchingMode::Chunked. |
| components/clarinet-cli/src/deployments/mod.rs | Re-export BatchingMode and update wrapper APIs accordingly. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This PR takes the
generate_default_deployment_with_cache()from an ~800 line monstrosity into a more manageable ~200 line function. Each change was made as a separate commit, so for further details, see the commit messagesNote
To reviewers: This PR contains a lot of moved code, and the diff is hard to read. It will be much easier to see what was done by checking over each commit