Skip to content

Conversation

gzeoneth
Copy link
Member

@gzeoneth gzeoneth commented Jul 7, 2025

No description provided.

@cla-bot cla-bot bot added the s label Jul 7, 2025
@gzeoneth gzeoneth requested a review from Copilot July 7, 2025 19:13
Copilot

This comment was marked as outdated.

@gzeoneth gzeoneth requested a review from Copilot July 7, 2025 19:21
Copilot

This comment was marked as outdated.

@gzeoneth gzeoneth requested a review from Copilot July 8, 2025 18:15
Copy link

@Copilot 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

This PR enhances deployment tooling by enabling parallel contract deployments and updates corresponding tests for custom fee rollups.

  • Adjusts gas extraction logic and assertion in end-to-end custom fee rollup tests.
  • Introduces a SimpleMutex, nonce manager, and getNextNonce to coordinate non-CREATE2 deployments.
  • Refactors scripts/deploymentUtils.ts to deploy multiple contracts in parallel and adds a helper for one-step prover deployment.

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.

File Description
test/e2e/customFeeRollup.ts Fixed substring ranges for gas and extraGas extraction, added L1 gas usage assertion.
scripts/deploymentUtils.ts Added mutex-based nonce/verification locks, getNextNonce, and rewrote sequential deploys as Promise.all.
Comments suppressed due to low confidence (4)

scripts/deploymentUtils.ts:31

  • Add a brief doc comment above SimpleMutex explaining its behavior and intended use, so future maintainers understand the locking semantics.
class SimpleMutex {

scripts/deploymentUtils.ts:50

  • Document the purpose of verifyLock, specifying that it serializes contract verifications to avoid rate limits or race conditions.
let verifyLock = new SimpleMutex()

scripts/deploymentUtils.ts:61

  • [nitpick] The variable resolveVerifyLock represents the unlock function. Renaming it to releaseVerifyLock or unlockVerify would clarify its intent.
  const resolveVerifyLock = await verifyLock.lock()

scripts/deploymentUtils.ts:165

  • Add a JSDoc comment for getNextNonce describing how it initializes and increments nonces per address, including lock behavior.
async function getNextNonce(signer: any): Promise<number> {

Comment on lines 189 to +190
const gasPrice = BigNumber.from(
'0x' + batchSpendingReportData.substring(236, 298)
'0x' + batchSpendingReportData.substring(234, 298)
Copy link

Copilot AI Jul 8, 2025

Choose a reason for hiding this comment

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

The hard-coded substring indices (234, 298) are magic numbers. Extract these offsets into named constants or add comments explaining their origin for clarity.

Copilot uses AI. Check for mistakes.

Comment on lines 188 to +193
const bp = batchSpendingReportData.substring(66, 106)
const gasPrice = BigNumber.from(
'0x' + batchSpendingReportData.substring(236, 298)
'0x' + batchSpendingReportData.substring(234, 298)
)
const extraGas = BigNumber.from(
'0x' + batchSpendingReportData.substring(298)
'0x' + batchSpendingReportData.substring(298, 314)
Copy link

Copilot AI Jul 8, 2025

Choose a reason for hiding this comment

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

Similarly, the range (298, 314) is a magic number. Consider defining a constant or helper to document what these positions represent in the batch data.

Copilot uses AI. Check for mistakes.

Comment on lines +196 to +201
const gasUsedForL1 = BigNumber.from(
(
await l2Provider.send('eth_getTransactionReceipt', [
batchSpendingReportEvent.transactionHash,
])
).gasUsedForL1
Copy link

Copilot AI Jul 8, 2025

Choose a reason for hiding this comment

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

[nitpick] Consider extracting this receipt lookup into a helper function (e.g., getL1GasFromReceipt) to improve readability and reuse in other tests.

Suggested change
const gasUsedForL1 = BigNumber.from(
(
await l2Provider.send('eth_getTransactionReceipt', [
batchSpendingReportEvent.transactionHash,
])
).gasUsedForL1
const gasUsedForL1 = await getL1GasFromReceipt(
l2Provider,
batchSpendingReportEvent.transactionHash

Copilot uses AI. Check for mistakes.

true
)
console.log('Deploying bridge templates in parallel...')
const [
Copy link

Copilot AI Jul 8, 2025

Choose a reason for hiding this comment

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

Launching a large array of Promise.all deployments concurrently can overwhelm the RPC provider. Consider throttling concurrency or batching the promises.

Copilot uses AI. Check for mistakes.

@gzeoneth gzeoneth closed this Jul 17, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant