Skip to content

Conversation

@0xDEnYO
Copy link
Contributor

@0xDEnYO 0xDEnYO commented Dec 19, 2025

Which Jira task belongs to this PR?

https://lifi.atlassian.net/browse/SMAR-99

Why did I implement it this way?

Checklist before requesting a review

Checklist for reviewer (DO NOT DEPLOY and contracts BEFORE CHECKING THIS!!!)

  • I have checked that any arbitrary calls to external contracts are validated and or restricted
  • I have checked that any privileged calls (i.e. storage modifications) are validated and or restricted
  • I have ensured that any new contracts have had AT A MINIMUM 1 preliminary audit conducted on by <company/auditor>

@lifi-action-bot lifi-action-bot marked this pull request as draft December 19, 2025 05:29
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Dec 19, 2025

Walkthrough

Modified package.json scripts: abi:generate removes the typechain directory cleanup step, and confirm-safe-tx now executes bun typechain before bun abi:generate. Updated demoScriptHelpers.ts to use default import bs58 instead of named import decodeBase58 from the 'bs58' library.

Changes

Cohort / File(s) Summary
Package.json script updates
package.json
Modified abi:generate script to remove rm -fr typechain/* step; updated confirm-safe-tx script to run bun typechain before bun abi:generate
Import refactoring
script/demoScripts/utils/demoScriptHelpers.ts
Changed from named import decodeBase58 to default import bs58 from 'bs58' library; updated function call from decodeBase58(solanaAddress) to bs58.decode(solanaAddress)

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

  • Verify that removing the rm -fr typechain/* step from abi:generate does not cause stale file issues during build
  • Confirm that running bun typechain before bun abi:generate in confirm-safe-tx is necessary and does not duplicate work already performed elsewhere

Possibly related PRs

Suggested labels

AuditNotRequired

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title 'updated erroneous import and script calls' accurately summarizes the two main changes: fixing an import in demoScriptHelpers.ts and adjusting script commands in package.json.
Description check ✅ Passed The description follows the template structure with Jira reference and completed checklist items, but the 'Why did I implement it this way?' section is empty, lacking explanation of the rationale behind the changes.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fix-confirm-script

Warning

Review ran into problems

🔥 Problems

Errors were encountered while retrieving linked issues.

Errors (1)
  • JIRA integration encountered authorization issues. Please disconnect and reconnect the integration in the CodeRabbit UI.

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@0xDEnYO 0xDEnYO marked this pull request as ready for review December 19, 2025 05:30
@0xDEnYO 0xDEnYO enabled auto-merge (squash) December 19, 2025 05:30
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

📜 Review details

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Disabled knowledge base sources:

  • Jira integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between abe5ed8 and ed8ea40.

📒 Files selected for processing (2)
  • package.json (1 hunks)
  • script/demoScripts/utils/demoScriptHelpers.ts (2 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
script/demoScripts/**/*.{ts,js}

📄 CodeRabbit inference engine (.cursor/rules/001-project-structure.mdc)

Place TypeScript demo scripts in script/demoScripts/ directory. Reference existing demos for patterns and place helper utilities in script/demoScripts/utils/

Files:

  • script/demoScripts/utils/demoScriptHelpers.ts
**/*.{ts,tsx}

📄 CodeRabbit inference engine (.cursor/rules/099-finish.mdc)

After TypeScript changes, run lint/tests with Bun

Files:

  • script/demoScripts/utils/demoScriptHelpers.ts
{src,script,test}/**/*.{sol,ts}

📄 CodeRabbit inference engine (.cursor/rules/105-security.mdc)

Validate all external inputs and configuration (including script/env values) explicitly; prefer existing validation helpers (e.g., Validatable, config readers) over ad-hoc checks

Files:

  • script/demoScripts/utils/demoScriptHelpers.ts
{script,tasks}/**/*.ts

📄 CodeRabbit inference engine (.cursor/rules/200-typescript.mdc)

{script,tasks}/**/*.ts: TS scripts use .eslintrc.cjs rules, bunx tsx, citty, consola, and env validated via helpers (e.g., getEnvVar()).
DO NOT use deprecated ethers-based helpers (getProvider, getWalletFromPrivateKeyInDotEnv, ethers sendTransaction, ensureBalanceAndAllowanceToDiamond).
Obey .eslintrc.cjs; avoid any; use TypeChain types from typechain/ directory (e.g., ILiFi.BridgeDataStruct).
Import organization: Group imports as: external libs (viem, consola, citty, dotenv) → TypeChain types → config files → internal utils/helpers. Use type imports for types-only.
Error handling: Use try/catch with consola.error() for logging; exit with process.exit(1) on fatal errors. Provide meaningful error messages.
CLI: use citty; logging via consola; validate env via getEnvVar(); exit 0/1 appropriately.
New TypeScript helpers must be covered by a colocated *.test.ts file using Bun (describe / it / expect) with 100% coverage. Cover edge cases and error paths.

Files:

  • script/demoScripts/utils/demoScriptHelpers.ts
script/demoScripts/**/*.ts

📄 CodeRabbit inference engine (.cursor/rules/200-typescript.mdc)

script/demoScripts/**/*.ts: MUST use viem for all contract interactions in demo/operational scripts; ethers.js helpers are deprecated.
Demo scripts follow similar structural patterns (e.g., main() function, setupEnvironment(), helpers from demoScriptHelpers). See demoLidoWrapper.ts, demoUnit.ts, demoEco.ts as reference examples.

Files:

  • script/demoScripts/utils/demoScriptHelpers.ts
🧠 Learnings (14)
📓 Common learnings
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 1256
File: deployments/zksync.diamond.json:81-87
Timestamp: 2025-07-04T08:59:08.108Z
Learning: When analyzing deployment PRs in the lifinance/contracts repository, carefully verify that target state configuration files (like script/deploy/_targetState.json) have been updated before flagging missing entries. The AI summary section should be consulted to understand all file changes, as manual searches might miss entries due to formatting differences or search limitations.
Learnt from: mirooon
Repo: lifinance/contracts PR: 1283
File: deployments/ronin.diamond.json:65-68
Timestamp: 2025-08-07T10:20:01.383Z
Learning: When analyzing deployment PRs in the lifinance/contracts repository, carefully verify that target state configuration files (like script/deploy/_targetState.json) and deployment log files have been updated before flagging missing entries. The AI summary section should be consulted to understand all file changes, as manual searches might miss entries due to formatting differences or search limitations.
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 1168
File: script/deploy/_targetState.json:1564-1589
Timestamp: 2025-05-27T12:36:26.987Z
Learning: When reviewing deployment PRs in the lifinance/contracts repository, target state configuration files (like script/deploy/_targetState.json) may be updated for multiple networks even when the PR is focused on deploying to a specific network. The scope should be determined by the PR title and description, not just by all configuration changes present in the files.
Learnt from: mirooon
Repo: lifinance/contracts PR: 1328
File: script/deploy/facets/LDA/DeployAlgebraFacet.s.sol:4-4
Timestamp: 2025-08-26T15:19:07.800Z
Learning: DeployScriptBase.sol is located at script/deploy/facets/utils/DeployScriptBase.sol, not script/deploy/utils/DeployScriptBase.sol. Import paths from script/deploy/facets/LDA/ should use "../utils/DeployScriptBase.sol" to reference it correctly.
Learnt from: ezynda3
Repo: lifinance/contracts PR: 953
File: script/deploy/zksync/utils/UpdateScriptBase.sol:32-50
Timestamp: 2025-03-05T14:41:05.791Z
Learning: In the LiFi contracts repository, explicit error handling for file operations and address validation in deployment scripts like UpdateScriptBase.sol is not considered necessary by the maintainer.
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 1169
File: script/deploy/zksync/DeployFeeCollector.zksync.s.sol:32-37
Timestamp: 2025-05-27T12:00:43.940Z
Learning: The lifinance/contracts repository has deployment scripts that perform validation checks (including zero-address validation) before executing individual deploy scripts, making runtime validation checks in the deploy scripts themselves redundant.
Learnt from: mirooon
Repo: lifinance/contracts PR: 1406
File: script/deploy/facets/UpdateUnitFacet.s.sol:1-3
Timestamp: 2025-10-02T18:14:45.047Z
Learning: For update scripts in script/deploy/facets/, the SPDX license identifier should be immediately followed by the pragma statement without a blank line in between, as confirmed by mirooon for script/deploy/facets/UpdateUnitFacet.s.sol.
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 1334
File: deployments/mainnet.json:54-54
Timestamp: 2025-08-26T02:20:52.515Z
Learning: For deployment PRs in the lifinance/contracts repository, carefully verify the specific scope mentioned in the PR title and description before suggesting updates to other networks. Not all deployments are cross-network updates - some are targeted to specific chains only.
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 1266
File: script/deploy/safe/execute-pending-timelock-tx.ts:334-334
Timestamp: 2025-07-17T04:21:55.549Z
Learning: In the lifinance/contracts repository, 0xDEnYO prefers to keep private key processing simple in scripts like execute-pending-timelock-tx.ts without adding format validation, prioritizing code simplicity over strict validation in controlled environments.
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 1266
File: script/deploy/safe/execute-pending-timelock-tx.ts:129-129
Timestamp: 2025-07-17T04:21:50.790Z
Learning: In the lifinance/contracts repository, 0xDEnYO prefers not to add private key format validation in deployment scripts like execute-pending-timelock-tx.ts, prioritizing code simplicity over strict validation in their controlled environment.
Learnt from: CR
Repo: lifinance/contracts PR: 0
File: .cursor/rules/001-project-structure.mdc:0-0
Timestamp: 2025-12-17T10:27:05.468Z
Learning: Applies to script/demoScripts/**/*.{ts,js} : Place TypeScript demo scripts in `script/demoScripts/` directory. Reference existing demos for patterns and place helper utilities in `script/demoScripts/utils/`
Learnt from: CR
Repo: lifinance/contracts PR: 0
File: .cursor/rules/200-typescript.mdc:0-0
Timestamp: 2025-12-17T10:29:05.622Z
Learning: Applies to script/demoScripts/**/*.ts : MUST use viem for all contract interactions in demo/operational scripts; ethers.js helpers are deprecated.
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 1299
File: script/demoScripts/demoAcrossV4.ts:728-737
Timestamp: 2025-08-22T10:03:58.794Z
Learning: Demo scripts in `script/demoScripts` are exempt from the citty CLI argument parsing requirement that applies to other TypeScript scripts. Templates for demo scripts don't use citty and this is acceptable.
Learnt from: CR
Repo: lifinance/contracts PR: 0
File: .cursor/rules/200-typescript.mdc:0-0
Timestamp: 2025-12-17T10:29:05.622Z
Learning: Applies to script/demoScripts/**/*.ts : Demo scripts follow similar structural patterns (e.g., `main()` function, `setupEnvironment()`, helpers from `demoScriptHelpers`). See `demoLidoWrapper.ts`, `demoUnit.ts`, `demoEco.ts` as reference examples.
Learnt from: CR
Repo: lifinance/contracts PR: 0
File: .cursor/rules/200-typescript.mdc:0-0
Timestamp: 2025-12-17T10:29:05.622Z
Learning: Always reuse existing helpers and types: Search `script/common/`, `script/utils/`, `script/demoScripts/utils/`, and other helper directories before implementing new functionality. Key helpers: `script/utils/deploymentHelpers.ts` (deployment loading), `script/demoScripts/utils/demoScriptHelpers.ts` (viem-based demo helpers and swap helpers), `script/common/types.ts` (shared types).
Learnt from: CR
Repo: lifinance/contracts PR: 0
File: .cursor/rules/200-typescript.mdc:0-0
Timestamp: 2025-12-17T10:29:05.622Z
Learning: Applies to {script,tasks}/**/*.ts : DO NOT use deprecated ethers-based helpers (`getProvider`, `getWalletFromPrivateKeyInDotEnv`, ethers `sendTransaction`, `ensureBalanceAndAllowanceToDiamond`).
Learnt from: CR
Repo: lifinance/contracts PR: 0
File: .cursor/rules/200-typescript.mdc:0-0
Timestamp: 2025-12-17T10:29:05.622Z
Learning: Applies to {script,tasks}/**/*.ts : CLI: use `citty`; logging via `consola`; validate env via `getEnvVar()`; exit 0/1 appropriately.
Learnt from: CR
Repo: lifinance/contracts PR: 0
File: .cursor/rules/200-typescript.mdc:0-0
Timestamp: 2025-12-17T10:29:05.622Z
Learning: Applies to {script,tasks}/**/*.ts : Obey `.eslintrc.cjs`; avoid `any`; use TypeChain types from `typechain/` directory (e.g., `ILiFi.BridgeDataStruct`).
Learnt from: CR
Repo: lifinance/contracts PR: 0
File: .cursor/rules/200-typescript.mdc:0-0
Timestamp: 2025-12-17T10:29:05.622Z
Learning: Applies to {script,tasks}/**/*.ts : Import organization: Group imports as: external libs (viem, consola, citty, dotenv) → TypeChain types → config files → internal utils/helpers. Use `type` imports for types-only.
Learnt from: mirooon
Repo: lifinance/contracts PR: 945
File: script/demoScripts/demoGlacisAirlift.ts:42-59
Timestamp: 2025-01-22T12:40:50.355Z
Learning: In TypeScript/JavaScript demo scripts using ethers.js, nonce is automatically managed by the library and doesn't need to be explicitly set. Similarly, the default gas limit is typically sufficient for standard operations like ERC20 approvals.
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 1196
File: script/helperFunctions.sh:1447-1462
Timestamp: 2025-06-19T06:23:47.848Z
Learning: 0xDEnYO prefers to keep eval usage in local bash scripts when the security risk is acceptable in their controlled environment, prioritizing simplicity over security hardening for local tooling.
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 1266
File: script/deploy/safe/execute-pending-timelock-tx.ts:627-628
Timestamp: 2025-07-17T04:21:26.825Z
Learning: In the lifinance/contracts repository, 0xDEnYO prefers to keep '0x0' as a fallback address in gas estimation calls rather than throwing errors when the wallet account address is not available, prioritizing code simplicity over strict validation.
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 1325
File: script/tasks/diamondSyncDEXs.sh:116-116
Timestamp: 2025-08-27T08:45:59.606Z
Learning: In script/tasks/diamondSyncDEXs.sh, user 0xDEnYO has chosen to selectively apply ShellCheck fixes, keeping array assignments using $() construct and other patterns as-is in their controlled deployment environment, prioritizing functionality over strict ShellCheck compliance.
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 1212
File: .github/workflows/convertForkedPRsToInternal.yml:81-106
Timestamp: 2025-07-16T06:18:02.682Z
Learning: 0xDEnYO prefers to use printf "%q" for shell escaping in GitHub workflows to increase security and protection from potential injections, even when it might cause formatting issues, prioritizing security over convenience.
📚 Learning: 2025-12-17T10:29:05.622Z
Learnt from: CR
Repo: lifinance/contracts PR: 0
File: .cursor/rules/200-typescript.mdc:0-0
Timestamp: 2025-12-17T10:29:05.622Z
Learning: Applies to script/demoScripts/**/*.ts : MUST use viem for all contract interactions in demo/operational scripts; ethers.js helpers are deprecated.

Applied to files:

  • script/demoScripts/utils/demoScriptHelpers.ts
📚 Learning: 2025-12-17T10:29:05.622Z
Learnt from: CR
Repo: lifinance/contracts PR: 0
File: .cursor/rules/200-typescript.mdc:0-0
Timestamp: 2025-12-17T10:29:05.622Z
Learning: Applies to {script,tasks}/**/*.ts : DO NOT use deprecated ethers-based helpers (`getProvider`, `getWalletFromPrivateKeyInDotEnv`, ethers `sendTransaction`, `ensureBalanceAndAllowanceToDiamond`).

Applied to files:

  • package.json
📚 Learning: 2025-07-04T08:59:08.108Z
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 1256
File: deployments/zksync.diamond.json:81-87
Timestamp: 2025-07-04T08:59:08.108Z
Learning: When analyzing deployment PRs in the lifinance/contracts repository, carefully verify that target state configuration files (like script/deploy/_targetState.json) have been updated before flagging missing entries. The AI summary section should be consulted to understand all file changes, as manual searches might miss entries due to formatting differences or search limitations.

Applied to files:

  • package.json
📚 Learning: 2025-12-17T10:29:05.622Z
Learnt from: CR
Repo: lifinance/contracts PR: 0
File: .cursor/rules/200-typescript.mdc:0-0
Timestamp: 2025-12-17T10:29:05.622Z
Learning: Applies to {script,tasks}/**/*.ts : Obey `.eslintrc.cjs`; avoid `any`; use TypeChain types from `typechain/` directory (e.g., `ILiFi.BridgeDataStruct`).

Applied to files:

  • package.json
📚 Learning: 2025-08-07T10:20:01.383Z
Learnt from: mirooon
Repo: lifinance/contracts PR: 1283
File: deployments/ronin.diamond.json:65-68
Timestamp: 2025-08-07T10:20:01.383Z
Learning: When analyzing deployment PRs in the lifinance/contracts repository, carefully verify that target state configuration files (like script/deploy/_targetState.json) and deployment log files have been updated before flagging missing entries. The AI summary section should be consulted to understand all file changes, as manual searches might miss entries due to formatting differences or search limitations.

Applied to files:

  • package.json
📚 Learning: 2025-12-17T10:29:05.622Z
Learnt from: CR
Repo: lifinance/contracts PR: 0
File: .cursor/rules/200-typescript.mdc:0-0
Timestamp: 2025-12-17T10:29:05.622Z
Learning: Applies to {script,tasks}/**/*.ts : CLI: use `citty`; logging via `consola`; validate env via `getEnvVar()`; exit 0/1 appropriately.

Applied to files:

  • package.json
📚 Learning: 2025-12-17T10:29:05.622Z
Learnt from: CR
Repo: lifinance/contracts PR: 0
File: .cursor/rules/200-typescript.mdc:0-0
Timestamp: 2025-12-17T10:29:05.622Z
Learning: Applies to {script,tasks}/**/*.ts : TS scripts use `.eslintrc.cjs` rules, `bunx tsx`, `citty`, `consola`, and env validated via helpers (e.g., `getEnvVar()`).

Applied to files:

  • package.json
📚 Learning: 2025-12-17T10:28:51.714Z
Learnt from: CR
Repo: lifinance/contracts PR: 0
File: .cursor/rules/107-solidity-scripts.mdc:0-0
Timestamp: 2025-12-17T10:28:51.714Z
Learning: Applies to script/deploy/**/*.sol : Keep deployment scripts under `script/deploy/` directory structure (and `script/deploy/zksync/` when relevant) and reuse helper/config files like `deployRequirements.json` and `targetState.json` instead of creating new patterns

Applied to files:

  • package.json
📚 Learning: 2025-12-17T10:29:05.622Z
Learnt from: CR
Repo: lifinance/contracts PR: 0
File: .cursor/rules/200-typescript.mdc:0-0
Timestamp: 2025-12-17T10:29:05.622Z
Learning: Applies to {script,tasks}/**/*.ts : New TypeScript helpers must be covered by a colocated `*.test.ts` file using Bun (`describe` / `it` / `expect`) with 100% coverage. Cover edge cases and error paths.

Applied to files:

  • package.json
📚 Learning: 2025-12-17T10:27:39.700Z
Learnt from: CR
Repo: lifinance/contracts PR: 0
File: .cursor/rules/099-finish.mdc:0-0
Timestamp: 2025-12-17T10:27:39.700Z
Learning: Applies to **/*.sol : After Solidity changes, run `forge test` (or note suites remaining)

Applied to files:

  • package.json
📚 Learning: 2025-12-17T10:29:05.622Z
Learnt from: CR
Repo: lifinance/contracts PR: 0
File: .cursor/rules/200-typescript.mdc:0-0
Timestamp: 2025-12-17T10:29:05.622Z
Learning: Applies to {script,tasks}/**/*.ts : Import organization: Group imports as: external libs (viem, consola, citty, dotenv) → TypeChain types → config files → internal utils/helpers. Use `type` imports for types-only.

Applied to files:

  • package.json
📚 Learning: 2025-12-17T10:27:39.700Z
Learnt from: CR
Repo: lifinance/contracts PR: 0
File: .cursor/rules/099-finish.mdc:0-0
Timestamp: 2025-12-17T10:27:39.700Z
Learning: Applies to **/*.{ts,tsx} : After TypeScript changes, run lint/tests with Bun

Applied to files:

  • package.json
📚 Learning: 2024-12-02T08:19:07.783Z
Learnt from: ezynda3
Repo: lifinance/contracts PR: 875
File: script/utils/fetch-missing-deployments.ts:46-46
Timestamp: 2024-12-02T08:19:07.783Z
Learning: In our codebase, scripts like `script/utils/fetch-missing-deployments.ts` are intended to run in Node.js version 18 or newer, so global `fetch` is available without importing.

Applied to files:

  • package.json
⏰ 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). (2)
  • GitHub Check: run-unit-tests
  • GitHub Check: run-unit-tests
🔇 Additional comments (2)
script/demoScripts/utils/demoScriptHelpers.ts (2)

150-164: LGTM - Usage updated correctly.

The function call has been updated to use bs58.decode() method, which is correct for the default import pattern. The logic remains sound, assuming the import verification passes.


5-7: No issue detected — default import is the standard pattern for bs58.

The code correctly uses import bs58 from 'bs58', which aligns with official bs58 documentation and is properly supported by modern versions used via the transitive dependency. The @ts-expect-error and eslint-disable comments appropriately suppress warnings for this transitive dependency access pattern.

Comment on lines +60 to +66
"abi:generate": "forge clean && forge build --skip script --skip test --skip Base --skip Test --skip '*.t.sol' && bun tasks/generateDiamondABI.ts",
"abi:removeDuplicateEvents": "bunx tsx ./script/removeDuplicateEventsFromABI.ts",
"add-network-rpc": "bunx tsx ./script/mongoDb/add-network-rpc.ts",
"add-safe-owners-and-threshold": "bunx tsx ./script/deploy/safe/add-safe-owners-and-threshold.ts",
"codegen": "plop",
"compile": "forge build",
"confirm-safe-tx": "bun abi:generate && bunx tsx ./script/deploy/safe/confirm-safe-tx.ts",
"confirm-safe-tx": "bun typechain && bun abi:generate && bunx tsx ./script/deploy/safe/confirm-safe-tx.ts",
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Consider optimizing the script sequence to avoid double cleaning.

The confirm-safe-tx script now runs bun typechain && bun abi:generate, which causes forge clean to execute twice in sequence:

  1. typechain runs: forge clean && rm -rf typechain/* && forge build src && ...
  2. abi:generate runs: forge clean && forge build --skip script... (immediately after)

The second forge clean discards the build artifacts from the first build, making the sequence wasteful and potentially causing inconsistencies between TypeChain-generated types and the ABIs.

🔎 Suggested optimization

Consider one of these approaches:

Option 1: Create a dedicated script that runs the full sequence without double-cleaning:

+    "prepare-safe-tx": "forge clean && rm -rf typechain/* && forge build src && bun abi:removeDuplicateEvents && typechain --target ethers-v5 'out/!(*.t).sol/*.json' --out-dir typechain && forge build --skip script --skip test --skip Base --skip Test --skip '*.t.sol' && bun tasks/generateDiamondABI.ts",
-    "confirm-safe-tx": "bun typechain && bun abi:generate && bunx tsx ./script/deploy/safe/confirm-safe-tx.ts",
+    "confirm-safe-tx": "bun prepare-safe-tx && bunx tsx ./script/deploy/safe/confirm-safe-tx.ts",

Option 2: Use incremental builds where possible:

-    "confirm-safe-tx": "bun typechain && bun abi:generate && bunx tsx ./script/deploy/safe/confirm-safe-tx.ts",
+    "confirm-safe-tx": "bun typechain && bun typechain:incremental && bun tasks/generateDiamondABI.ts && bunx tsx ./script/deploy/safe/confirm-safe-tx.ts",

Note: typechain:incremental (line 105) avoids the forge clean step.

Committable suggestion skipped: line range outside the PR's diff.

🤖 Prompt for AI Agents
In package.json around lines 60-66, the confirm-safe-tx script chains `bun
typechain && bun abi:generate` which triggers `forge clean` twice (once in
typechain and again in abi:generate); to fix, replace that sequence with either
(A) call the incremental typechain target that skips the clean (e.g., `bun
typechain:incremental && bun abi:generate`) or (B) create a new combined script
that runs `forge clean` once, then runs the build/typechain/abi-generation steps
in order (e.g., `forge clean && forge build ... && bunx tsx path/to/typechain &&
bunx tsx path/to/generateDiamondABI.ts`) and use that new script in place of
confirm-safe-tx so you avoid re-cleaning and preserve consistent artifacts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants