run update periphery before synching whitelist #1508
Conversation
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
Contributor
WalkthroughThe change adds a pre-sync prerequisite step to Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Possibly related PRs
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (1)script/**/*.sh📄 CodeRabbit inference engine (conventions.md)
Files:
🧠 Learnings (26)📓 Common learnings📚 Learning: 2024-11-26T01:50:51.809ZApplied to files:
📚 Learning: 2025-08-27T08:45:59.606ZApplied to files:
📚 Learning: 2025-06-05T10:00:01.583ZApplied to files:
📚 Learning: 2025-07-04T08:59:08.108ZApplied to files:
📚 Learning: 2025-12-02T10:08:06.478ZApplied to files:
📚 Learning: 2025-12-10T17:36:50.095ZApplied to files:
📚 Learning: 2025-08-07T10:20:01.383ZApplied to files:
📚 Learning: 2025-04-21T03:17:53.443ZApplied to files:
📚 Learning: 2025-04-21T03:17:53.443ZApplied to files:
📚 Learning: 2025-12-10T17:25:01.008ZApplied to files:
📚 Learning: 2025-06-06T16:10:04.674ZApplied to files:
📚 Learning: 2025-06-06T16:09:58.692ZApplied to files:
📚 Learning: 2024-10-04T09:21:59.708ZApplied to files:
📚 Learning: 2025-05-27T12:36:26.987ZApplied to files:
📚 Learning: 2025-09-09T10:39:26.383ZApplied to files:
📚 Learning: 2024-11-26T01:18:52.125ZApplied to files:
📚 Learning: 2025-11-06T10:38:55.953ZApplied to files:
📚 Learning: 2025-01-29T06:28:56.328ZApplied to files:
📚 Learning: 2025-06-24T01:27:58.260ZApplied to files:
📚 Learning: 2025-12-02T10:08:06.478ZApplied to files:
📚 Learning: 2025-08-01T22:11:04.478ZApplied to files:
📚 Learning: 2025-08-26T02:20:52.515ZApplied to files:
📚 Learning: 2024-11-26T01:03:43.597ZApplied to files:
📚 Learning: 2024-10-04T09:17:19.275ZApplied to files:
📚 Learning: 2025-03-21T13:48:54.006ZApplied to files:
🧬 Code graph analysis (1)script/tasks/diamondSyncWhitelist.sh (1)
⏰ 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)
🔇 Additional comments (1)
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit 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.
📒 Files selected for processing (2)
config/whitelist.json(1 hunks)script/tasks/diamondSyncWhitelist.sh(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
script/**/*.sh
📄 CodeRabbit inference engine (conventions.md)
script/**/*.sh: Bash scripts must begin with #!/bin/bash, use modular functions with clear sections (Logging, Error handling and logging, Deployment functions), follow DRY principle using helper files, use consistent indentation and naming, include proper comments and documentation
Bash scripts must use helper functions for logging (echoDebug, error, warning, success), validate inputs and environment early, check function exit status with checkFailure, use set -e where appropriate, and provide automated retry mechanisms for RPC issues
Files:
script/tasks/diamondSyncWhitelist.sh
🧠 Learnings (13)
📓 Common learnings
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: 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: 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: 874
File: script/tasks/diamondSyncSigs.sh:88-88
Timestamp: 2024-11-26T01:50:51.809Z
Learning: In the 'lifinance/contracts' repository, scripts like 'script/tasks/diamondSyncSigs.sh' are executed only on local machines where private keys are securely managed, so additional private key security improvements are not required.
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 1109
File: deployments/worldchain.json:28-28
Timestamp: 2025-04-21T03:17:53.443Z
Learning: For deployment PRs involving address updates like the RelayFacet to Worldchain, verify the actual presence of entries in files before reporting issues. The RelayFacet exists in the diamond log file and the PR diff already contains the necessary address change.
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: 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: 1109
File: deployments/worldchain.json:28-28
Timestamp: 2025-04-21T03:17:53.443Z
Learning: For deployment PRs updating contract addresses (like RelayFacet on Worldchain), verify the presence of entries in all relevant files (worldchain.json, worldchain.diamond.json, _deployments_log_file.json) before reporting inconsistencies. The RelayFacet entry exists in all required deployment files with the correct address.
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: 0xDEnYO
Repo: lifinance/contracts PR: 782
File: deployments/base.diamond.json:148-148
Timestamp: 2024-11-25T09:05:43.045Z
Learning: In deployment configuration files (e.g., `deployments/base.diamond.json`), empty addresses for contracts like `Permit2Proxy` may be placeholders and will be updated after approvals are released from the multisig safe.
Learnt from: CR
Repo: lifinance/contracts PR: 0
File: conventions.md:0-0
Timestamp: 2025-12-02T10:08:06.466Z
Learning: Applies to test/**/*.t.sol : For facets requiring whitelist functionality, test contracts should inherit from TestWhitelistManagerBase and use its functions (addToWhitelist, removeFromWhitelist, setFunctionWhitelistBySelector, removeFunctionApprovalBySelector)
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: 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-08-27T08:45:59.606Z
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.
Applied to files:
script/tasks/diamondSyncWhitelist.sh
📚 Learning: 2024-11-26T01:50:51.809Z
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 874
File: script/tasks/diamondSyncSigs.sh:88-88
Timestamp: 2024-11-26T01:50:51.809Z
Learning: In the 'lifinance/contracts' repository, scripts like 'script/tasks/diamondSyncSigs.sh' are executed only on local machines where private keys are securely managed, so additional private key security improvements are not required.
Applied to files:
script/tasks/diamondSyncWhitelist.sh
📚 Learning: 2025-06-05T10:00:01.583Z
Learnt from: mirooon
Repo: lifinance/contracts PR: 1145
File: script/tasks/diamondSyncWhitelistedAddresses.sh:208-209
Timestamp: 2025-06-05T10:00:01.583Z
Learning: Task scripts in script/tasks/ directory (like diamondSyncWhitelistedAddresses.sh) define functions that are sourced by deployAllContracts.sh and called from there, rather than executing directly. They don't need to call their own functions at the end of the file.
Applied to files:
script/tasks/diamondSyncWhitelist.sh
📚 Learning: 2025-12-02T10:08:06.466Z
Learnt from: CR
Repo: lifinance/contracts PR: 0
File: conventions.md:0-0
Timestamp: 2025-12-02T10:08:06.466Z
Learning: Applies to test/**/*.t.sol : For facets requiring whitelist functionality, test contracts should inherit from TestWhitelistManagerBase and use its functions (addToWhitelist, removeFromWhitelist, setFunctionWhitelistBySelector, removeFunctionApprovalBySelector)
Applied to files:
script/tasks/diamondSyncWhitelist.sh
📚 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:
script/tasks/diamondSyncWhitelist.sh
📚 Learning: 2025-04-21T03:17:53.443Z
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 1109
File: deployments/worldchain.json:28-28
Timestamp: 2025-04-21T03:17:53.443Z
Learning: For deployment PRs involving address updates like the RelayFacet to Worldchain, verify the actual presence of entries in files before reporting issues. The RelayFacet exists in the diamond log file and the PR diff already contains the necessary address change.
Applied to files:
script/tasks/diamondSyncWhitelist.sh
📚 Learning: 2025-01-28T14:29:00.823Z
Learnt from: ezynda3
Repo: lifinance/contracts PR: 924
File: script/deploy/zksync/utils/UpdateScriptBase.sol:112-178
Timestamp: 2025-01-28T14:29:00.823Z
Learning: The suggestion to modify `buildDiamondCut` function in `UpdateScriptBase.sol` to handle selectors from multiple old facets differently was deemed unnecessary by the maintainer.
Applied to files:
script/tasks/diamondSyncWhitelist.sh
📚 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:
script/tasks/diamondSyncWhitelist.sh
📚 Learning: 2025-04-21T03:17:53.443Z
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 1109
File: deployments/worldchain.json:28-28
Timestamp: 2025-04-21T03:17:53.443Z
Learning: For deployment PRs updating contract addresses (like RelayFacet on Worldchain), verify the presence of entries in all relevant files (worldchain.json, worldchain.diamond.json, _deployments_log_file.json) before reporting inconsistencies. The RelayFacet entry exists in all required deployment files with the correct address.
Applied to files:
script/tasks/diamondSyncWhitelist.sh
📚 Learning: 2025-11-13T09:20:21.608Z
Learnt from: ezynda3
Repo: lifinance/contracts PR: 1461
File: script/deploy/facets/UpdateWhitelistManagerFacet.s.sol:21-24
Timestamp: 2025-11-13T09:20:21.608Z
Learning: In deployment scripts that use the NO_BROADCAST flag (noBroadcast), the flag is specifically for building calldata for SAFE transactions (the diamondCut itself), not for helper contract deployments. Helper contracts like MigrateWhitelistManager need to be deployed regardless of the NO_BROADCAST flag setting, as they are prerequisites for the main diamondCut operation.
Applied to files:
script/tasks/diamondSyncWhitelist.sh
📚 Learning: 2025-11-25T02:57:58.015Z
Learnt from: 0xDEnYO
Repo: lifinance/contracts PR: 1466
File: config/whitelist.json:2546-2554
Timestamp: 2025-11-25T02:57:58.015Z
Learning: In lifinance/contracts config/whitelist.json, the SushiSwap Aggregator uses functions:
- snwap(address,uint256,address,address,uint256,address,bytes) -> selector 0x5f3bd1c8
- snwapMultiple((address,uint256,address)[],(address,address,uint256)[],(address,uint256,bytes)[]) -> selector 0xd33721a5
These selectors are canonical across networks and valid for the monad entry as well. Always compute with Keccak-256 (not SHA3-256) when verifying.
Applied to files:
config/whitelist.json
📚 Learning: 2025-11-14T12:58:18.040Z
Learnt from: ezynda3
Repo: lifinance/contracts PR: 1463
File: config/whitelist.json:24-26
Timestamp: 2025-11-14T12:58:18.040Z
Learning: In lifinance/contracts reviews, selectors must be verified by code or concrete on-chain evidence (ABI/tx) rather than assumed; provide a reproducible script or reference when proposing selector mappings.
Applied to files:
config/whitelist.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 (1)
config/whitelist.json (1)
7198-7207: No changes needed; signature and selector are correctly paired.The signature "runVM((uint8,bytes32)[],(bytes[]))" with selector 0x00a32e6c is correct. The updateWhitelistPeriphery.ts script validates all selector/signature pairs using
validateSelector(), which computes keccak256(signature) and compares the first 4 bytes against the provided selector. This entry exists in the whitelist, confirming it passed validation. The address 0x873fd892E827B9C3057f5A49A612EeecD6694cd3 is properly EIP-55 checksummed and consistent across config/whitelist.json and config/composerWhitelist.json.Likely an incorrect or invalid review comment.
mirooon
reviewed
Dec 4, 2025
script/tasks/diamondSyncWhitelist.sh
Outdated
| # Update whitelist periphery and composer entries before syncing | ||
| echo "" | ||
| echo "[info] Updating whitelist periphery and composer entries..." | ||
| bunx tsx ./script/tasks/updateWhitelistPeriphery.ts || checkFailure $? "update whitelist periphery" |
Contributor
There was a problem hiding this comment.
I would put it below ENVIRONMENT variable definition and pass ENVIRONMENT param to updateWhitelistPeriphery.ts . Like we do in deployAllContracts.sh script
Contributor
Author
There was a problem hiding this comment.
@mirooon
if we do not pass a parameter, then both whitelist files get updated.
I feel this is the correct and desired handling here, no?
(in the back of my mind there is also the point that we should soon update the whitelist on staging everytime we update on production so that we keep the staging setup in sync as Victor ran into problems there several times already)
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.
Why did I implement it this way?
This makes sure that the whitelist party maintained by the SC team are always up to date when running the sync whitelist command
Checklist before requesting a review
Checklist for reviewer (DO NOT DEPLOY and contracts BEFORE CHECKING THIS!!!)