feat: auto-split deployment batches when gas limit exceeded#77
Conversation
- Add Deployment struct to consolidate queue entries (name, salt, gasEstimate, predictedAddress) - Automatically split deployBatch into multiple batches when PER_TRANSACTION_GAS_LIMIT would be exceeded - Use DynamicArrayLib for efficient batch boundary calculation - Compute and store predicted addresses when adding to queue - Simplify gas estimation to per-contract only (BASE_TX_COST added per batch) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
WalkthroughConsolidates deployment queue in Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as Deploy Script
participant Facet as DeployFacet (queue)
participant Multicall as Multicall3
participant Target as Target Contracts
rect rgb(230, 248, 230)
CLI->>Facet: add(name, salt) (push Deployment)
CLI->>Facet: getQueue()
Facet-->>CLI: Deployment[] entries + totalGasEstimate
end
rect rgb(230, 230, 248)
CLI->>Facet: deployBatch()
Facet->>Facet: _calculateBatchBoundaries(entries, gas limits)
Facet->>Multicall: _deploySingleBatch(multicallPayload)
Multicall->>Target: execute multiple deploy txs (internal calls)
Multicall-->>Facet: batch results (addresses, receipts)
Facet->>CLI: emit per-Deployment logs (addr, gasEstimate)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks✅ Passed checks (3 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
scripts/common/DeployFacet.s.sol (1)
314-318: Consider caching bytecode in Deployment struct to avoid redundant I/O.The function calls
vm.getCode(getArtifactPath(entry.name))for each contract during deployment, but bytecode was already loaded in theadd()function (lines 96, 107). Consider storing bytecode in theDeploymentstruct to avoid redundant disk I/O, especially for large deployment batches.Trade-off: Increased memory usage vs. reduced I/O operations.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
scripts/common/DeployFacet.s.sol
🔇 Additional comments (5)
scripts/common/DeployFacet.s.sol (5)
27-36: LGTM! Good consolidation of deployment data.The Deployment struct effectively consolidates the previously separate arrays and adds the predicted address field, improving code organization and reducing the chance of index mismatches.
111-119: LGTM! Efficient pre-computation of deployment metadata.The refactored add() function now computes and stores both gas estimates and predicted addresses when queueing contracts, which improves efficiency by avoiding redundant calculations during batch deployment.
146-148: LGTM! Updated to use Deployment struct.The logging correctly accesses the new Deployment struct fields.
194-202: No callers of getQueue() exist in the codebase—no verification needed.The search across the repository reveals that
getQueue()is defined inDeployFacet.s.solbut is never called. Since this getter function has no external callers, the API change does not require downstream updates.
21-21: EIP-7825 reference is correct. The gas limit constant 1 << 24 = 16,777,216 accurately reflects EIP-7825's per-transaction gas cap.
- Add require in add() to reject contracts exceeding gas limit - Simplify _calculateBatchBoundaries() since oversized contracts are blocked 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/facets/token/ERC20/ERC20.sol (1)
61-68: Consider removing the unused named return variable for consistency.The named return variable
resultis declared but never assigned; the function immediately returns with an explicit value. This pattern is valid but unnecessary and creates inconsistency—all other view functions in this file use unnamed returns.For clarity, either remove the name or use it explicitly:
🔎 Proposed refactor to remove unused named return
- function allowance(address owner, address spender) - public - view - virtual - returns (uint256 result) - { + function allowance(address owner, address spender) public view virtual returns (uint256) { return ERC20Storage.layout().inner.allowance(owner, spender); }scripts/common/DeployFacet.s.sol (1)
315-322: Minor: Bytecode loaded twice per contract.The bytecode is already loaded in
add()for gas estimation, then loaded again here for deployment. For deployment scripts, this overhead is acceptable, but if memory/performance becomes a concern, consider caching bytecode in theDeploymentstruct or a separate mapping.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (30)
scripts/common/DeployFacet.s.solscripts/common/helpers/DiamondHelper.s.solscripts/common/helpers/FacetHelper.s.solscripts/deployments/diamonds/DeployDiamond.s.solscripts/deployments/facets/DeployDiamondCut.solscripts/deployments/facets/DeployDiamondLoupe.solscripts/deployments/facets/DeployEIP712Facet.solscripts/deployments/facets/DeployERC1271Facet.solscripts/deployments/facets/DeployIntrospection.solscripts/deployments/facets/DeployManagedProxy.solscripts/deployments/facets/DeployOwnable.solscripts/deployments/facets/DeployOwnablePending.solscripts/deployments/facets/DeployPausable.solscripts/deployments/facets/DeployTokenOwnable.solscripts/deployments/facets/DeployTokenPausable.solscripts/deployments/mocks/DeployMockERC20Permit.solscripts/deployments/mocks/DeployMockERC6909.solscripts/deployments/mocks/DeployMockERC721.solscripts/deployments/utils/DeployProxyManager.solsrc/facets/accounts/ERC1271Base.solsrc/facets/loupe/DiamondLoupeBase.solsrc/facets/token/ERC20/ERC20.solsrc/facets/token/ERC721/ERC721.solsrc/primitive/ERC721.soltest/TestUtils.soltest/facets/signature/EIP712.t.soltest/mocks/MockFacet.soltest/proxy/ProxyManager.t.soltest/utils/Brutalizer.soltest/utils/TestPlus.sol
✅ Files skipped from review due to trivial changes (17)
- scripts/deployments/mocks/DeployMockERC20Permit.sol
- scripts/deployments/facets/DeployOwnable.sol
- scripts/deployments/facets/DeployOwnablePending.sol
- test/utils/Brutalizer.sol
- scripts/deployments/facets/DeployPausable.sol
- src/facets/loupe/DiamondLoupeBase.sol
- scripts/deployments/diamonds/DeployDiamond.s.sol
- src/facets/accounts/ERC1271Base.sol
- test/mocks/MockFacet.sol
- scripts/deployments/facets/DeployEIP712Facet.sol
- test/TestUtils.sol
- scripts/common/helpers/DiamondHelper.s.sol
- test/proxy/ProxyManager.t.sol
- src/primitive/ERC721.sol
- scripts/deployments/facets/DeployERC1271Facet.sol
- scripts/deployments/facets/DeployIntrospection.sol
- scripts/deployments/facets/DeployTokenOwnable.sol
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2026-01-02T21:30:26.771Z
Learnt from: shuhuiluo
Repo: towns-protocol/diamond PR: 77
File: scripts/common/DeployFacet.s.sol:268-275
Timestamp: 2026-01-02T21:30:26.771Z
Learning: In Solidity deployment scripts (e.g., scripts/**/*.sol), avoid using draft EIP parameters. Use current network values for gas estimation and constants, ensuring references reflect approved, deployed network conditions rather than any draft EIP (e.g., do not hardcode draft EIP gas values or proposals). Verify EIP status and update constants to match live network configurations.
Applied to files:
scripts/deployments/facets/DeployDiamondLoupe.solscripts/deployments/facets/DeployTokenPausable.solscripts/deployments/mocks/DeployMockERC6909.solscripts/deployments/facets/DeployDiamondCut.solscripts/deployments/mocks/DeployMockERC721.solscripts/common/DeployFacet.s.solscripts/deployments/facets/DeployManagedProxy.solscripts/common/helpers/FacetHelper.s.solscripts/deployments/utils/DeployProxyManager.sol
📚 Learning: 2025-06-27T02:25:14.443Z
Learnt from: giuseppecrj
Repo: towns-protocol/diamond PR: 67
File: src/facets/accounts/ERC1271Base.sol:176-198
Timestamp: 2025-06-27T02:25:14.443Z
Learning: EIP712Base contract does not have a public DOMAIN_SEPARATOR() function. It has an internal _domainSeparatorV4() method that computes the domain separator dynamically.
Applied to files:
test/facets/signature/EIP712.t.sol
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Foundry project
🔇 Additional comments (18)
scripts/common/helpers/FacetHelper.s.sol (1)
18-21: LGTM! Formatting change with no functional impact.The FacetCut struct initialization has been reformatted to a single line. The function logic remains unchanged and correct.
test/utils/TestPlus.sol (1)
575-575: LGTM! Formatting change aligns with repository-wide formatting updates.The function signature has been reformatted to a single line as part of the broader formatting cleanup in this PR. The function logic remains unchanged and correct.
src/facets/token/ERC721/ERC721.sol (1)
92-99: Formatting-only change, unrelated to PR objectives.This signature reformatting is part of the "fmt" commit but doesn't relate to the deployment batch splitting feature described in the PR objectives. The function logic is unchanged and correctly implements the ERC721 standard.
scripts/deployments/mocks/DeployMockERC6909.sol (1)
55-57: LGTM!The single-line struct initialization is a consistent formatting change across the PR.
scripts/common/DeployFacet.s.sol (7)
27-33: LGTM - Clean consolidation of deployment metadata.The
Deploymentstruct appropriately groups related fields (name, salt, gasEstimate, predicted address) that were previously managed separately. This improves cohesion and simplifies queue management.
112-116: Good fix for the oversized contract issue.The require guard correctly blocks contracts that would exceed the per-transaction gas limit, preventing them from entering the queue and being silently skipped during batch boundary calculation.
118-123: LGTM!Storing the predicted CREATE2 address at queue time enables consistent reference throughout the deployment lifecycle.
283-303: Batch boundary logic is sound.The algorithm correctly partitions contracts into batches that respect
PER_TRANSACTION_GAS_LIMIT. The invariant that no single contract exceedsPER_TRANSACTION_GAS_LIMIT - BASE_TX_COST(enforced inadd()) ensures the reset-and-add sequence at lines 296-298 always stays within bounds.
268-270: LGTM!Reducing visibility to
privateis appropriate since this wrapper is only used internally withinDeployFacet.
272-279: LGTM!The refactored gas estimation cleanly separates per-contract costs from per-batch overhead (
BASE_TX_COST), and the NatSpec accurately reflects this. Based on learnings, the gas constants are appropriate for current network conditions.
197-206: LGTM!The updated return type provides callers with structured access to deployment entries and aggregate gas estimates.
scripts/deployments/facets/DeployManagedProxy.sol (1)
28-30: LGTM!Consistent formatting change across deployment scripts.
scripts/deployments/facets/DeployTokenPausable.sol (1)
29-31: LGTM!Consistent formatting change across deployment scripts.
scripts/deployments/facets/DeployDiamondCut.sol (1)
28-30: LGTM!Consistent formatting change across deployment scripts.
scripts/deployments/mocks/DeployMockERC721.sol (1)
45-47: LGTM!Consistent formatting change across deployment scripts.
scripts/deployments/utils/DeployProxyManager.sol (1)
28-30: LGTM!Consistent formatting change across deployment scripts.
scripts/deployments/facets/DeployDiamondLoupe.sol (1)
31-33: Formatting consolidation looks good.The single-line struct initialization is syntactically correct and maintains the same functionality.
test/facets/signature/EIP712.t.sol (1)
106-106: The tuple unpacking change fromaddress verifyingContract,toaddress verifyingContract,,is stylistic. Both syntaxes are functionally equivalent when unpacking the 7-value return fromeip712Domain()(fields, name, version, chainId, verifyingContract, salt, extensions). The single comma implicitly ignores the remaining values; the double comma explicitly skips the 6th value (salt) before ignoring the rest. No verification is needed—either syntax works correctly with the standard EIP-5267 signature.Likely an incorrect or invalid review comment.
2 participants
🤖 Generated with Claude Code
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.