Conversation
WalkthroughThis set of changes updates deployment and setup scripts, a configuration file, and the Changes
Sequence Diagram(s)sequenceDiagram
participant Deployer
participant TaiyiRegistryCoordinator
participant EigenLayerMiddleware
Deployer->>TaiyiRegistryCoordinator: initialize(owner, pausedStatus, allocationManager, eigenLayerMiddleware, pauserRegistry)
TaiyiRegistryCoordinator->>EigenLayerMiddleware: (assign middleware address if non-zero)
TaiyiRegistryCoordinator-->>Deployer: Initialization complete
sequenceDiagram
participant Script
participant PermissionController
participant EigenLayerMiddleware
participant AllocationManager
Script->>PermissionController: addAdmin(proxyDeployer)
Script->>PermissionController: addAdmin(implDeployer)
Script->>PermissionController: setAppointee(EigenLayerMiddleware, permissions)
Script->>AllocationManager: setAVSRegistrar(EigenLayerMiddleware)
Script->>AllocationManager: updateAVSMetadataURI("luban-local-test")
Possibly related PRs
Suggested reviewers
Poem
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
src/operator-registries/TaiyiRegistryCoordinator.sol (1)
111-118: 🛠️ Refactor suggestion
⚠️ Potential issue
initialize()lost pauser-registry initialisation – proxy will operate with a zero registry
Pausable’s constructor is executed only on the implementation contract, not on the proxy.
Because the_pauserRegistryargument is now commented-out and never forwarded, the storage slotpauserRegistryin the proxy remains the zero-address.
Any future pause/unpause attempts will revert, and_setPausedStatuscalled two lines earlier is presently operating without an actual registry.Suggested minimal fix (keeps the parameter index intact):
- address /* _pauserRegistry */ + address _pauserRegistryand after line 133:
+ require(_pauserRegistry != address(0), "Pauser registry cannot be zero address"); + pauserRegistry = IPauserRegistry(_pauserRegistry);This restores functional pausing while retaining backward-compatible ABI ordering.
🧹 Nitpick comments (3)
src/operator-registries/TaiyiRegistryCoordinator.sol (1)
125-135: Guardrail: make_allocationManagernon-optional or add an explicit defaultLeaving
allocationManagerunset when_allocationManager == address(0)pushes the onus on callers and silently produces a semi-initialised contract that will later revert inregisterOperator.
Either:
- Require a non-zero address up-front,
- if (_allocationManager != address(0)) { - allocationManager = IAllocationManager(_allocationManager); - } + require(_allocationManager != address(0), "AllocationManager cannot be zero"); + allocationManager = IAllocationManager(_allocationManager);or expose a public setter callable only-once by the owner.
script/SetRegistry.s.sol (2)
74-76:implDeployerderivation is fine – remember it’s unused below
implDeployeris computed but later never referenced outside the secondaddAdminToPermissionControllercall (see next comment).
If keeping the variable, considerunchecked{}wrapping or a short comment to explain the intent.
121-127: Hard-codingcreateOperatorSets.selectorcouples the script to contract internalsPassing a raw selector ties the deployment script to a specific function signature; any future change will silently break permissions.
Prefer reading the selector from the compiled ABI or exposing a named constant in the contract and importing it here.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
script/Deployments.s.sol(2 hunks)script/SetRegistry.s.sol(3 hunks)script/SetupContract.s.sol(1 hunks)script/configs/eigenlayer-deploy-config-devnet.json(3 hunks)script/setup-contract.sh(1 hunks)src/operator-registries/TaiyiRegistryCoordinator.sol(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Foundry Project
🔇 Additional comments (9)
script/setup-contract.sh (1)
17-23: Updated script execution order to match dependency flow.The script now properly runs SetRegistry before SetupContract, which aligns with the dependency flow where the registry needs to be set up before contracts are initialized.
script/SetupContract.s.sol (1)
91-95: Updated deployment data file path to include SLASHING context.The file path has been updated to refer to the new SLASHING-specific deployment data, which aligns with the integration of slashing mechanisms mentioned in LIN-39.
script/configs/eigenlayer-deploy-config-devnet.json (3)
37-37: Added withdrawal delay parameter for delegation.A new parameter
withdrawal_delay_blockshas been added with a value of 900 (approximately 3 hours at 12-second block time), which provides a security window for withdrawals.
51-51: Renamed parameter to align with EigenLayer naming conventions.The parameter
default_operator_split_bipshas been renamed toglobal_operator_commission_bipsto better reflect its purpose and maintain consistency with EigenLayer's terminology.
60-61: Added semantic versioning field.A new field
semverhas been added to track the version of the configuration, starting atv0.0.0. This helps with configuration management and identifying compatible versions.script/Deployments.s.sol (2)
108-112: Updated deployment data file path to include SLASHING context.The file path has been updated to refer to the SLASHING-specific deployment data, maintaining consistency with the changes in SetupContract.s.sol.
210-216: Added EigenLayerMiddleware address to TaiyiRegistryCoordinator initialization.The TaiyiRegistryCoordinator now requires the EigenLayerMiddleware address during initialization, establishing the required dependency for slashing mechanisms.
src/operator-registries/TaiyiRegistryCoordinator.sol (1)
82-85:onlyEigenLayerMiddlewarerevert string is inconsistent & typo-proneThe error selector
OnlyEigenlayerMiddleware()spells “Eigenlayer” with a lowercase “l”, whereas the variable and the protocol name elsewhere use “EigenLayer”.
Although purely cosmetic, mismatched revert selectors make downstream decoding/debugging harder.-require(msg.sender == eigenLayerMiddleware, OnlyEigenlayerMiddleware()); +require(msg.sender == eigenLayerMiddleware, OnlyEigenLayerMiddleware());Consider regenerating the custom error (or renaming the existing one) to keep casing consistent across the code-base.
Likely an incorrect or invalid review comment.
script/SetRegistry.s.sol (1)
119-120:acceptAdmin()caller/parameter order looks suspicious
PermissionController.acceptAdmin()is usually invoked by the pending admin without arguments (OE-style two-step admin).
Here it’s called byproxyDeployerand passeseigenLayerMiddlewareas parameter.
Confirm the signature:function acceptAdmin(address newAdmin) external; // ?If the function expects
msg.senderto be the new admin, this call will revert.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
test/SymbioticMiddleware.t.sol (1)
457-459: Middleware parameter added to initialization callThe addition of the EigenLayer middleware address parameter to the initialization call is consistent with the updated
TaiyiRegistryCoordinatorcontract requirements.However, using
eigenLayerPauserReg()as a placeholder for a middleware address when it's "not useful in symbiotic context" is potentially misleading and could cause confusion for future developers.Consider creating a dedicated mock middleware contract for testing purposes instead of repurposing an unrelated contract address. This would make the test more explicit about its intentions and better represent the actual system architecture.
test/DeployTest.t.sol (1)
1-42: New test for deployment flow verificationThis is a well-structured test that validates the full deployment process by executing the deployment scripts in sequence. It correctly sets up the test environment with appropriate funding for the deployment addresses.
However, there are some security and testing practice concerns:
- The test includes hardcoded private keys (even if they're test keys).
- The test lacks assertions to verify successful deployment outcomes.
Consider:
- Using dynamically generated keys for tests rather than hardcoded values.
- Adding verification assertions after deployment to confirm contracts were deployed correctly.
- Adding verification that deployed contracts have expected addresses and configurations.
Example improvement:
function testRunDeployScript() public { + // Generate random keys for testing + uint256 proxyOwnerKey = uint256(keccak256(abi.encodePacked("proxy_owner", block.timestamp))); + uint256 implOwnerKey = uint256(keccak256(abi.encodePacked("impl_owner", block.timestamp))); + vm.setEnv( "PROXY_OWNER_PRIVATE_KEY", - "c5114526e042343c6d1899cad05e1c00ba588314de9b96929914ee0df18d46b2" + vm.toString(proxyOwnerKey) ); /* rest of the function... */ deploy.run("deploy-test-config.json"); setRegistry.run(); setupContract.run(); + + // Verify deployment outcomes + // Example: verify contract addresses from deployment data file + string memory deployDataPath = "script/output/devnet/SLASHING_deploy_from_scratch_deployment_data.json"; + vm.assume(vm.exists(deployDataPath)); + string memory deployData = vm.readFile(deployDataPath); + // Add assertions to verify key contracts were deployed properly }script/configs/deploy-test-config.json (1)
1-62: New deployment configuration for testingThe configuration file provides a comprehensive set of parameters for the system deployment in test mode.
Several aspects of this configuration warrant attention:
Using the same multisig address (
0xD8F3183DEF51A987222D845be228e0Bbb932C222) for all roles would not be appropriate in production.The strategy deposit limits are set to the maximum uint256 value, which may not realistically test limit handling.
The WETH token address appears to be a test address but lacks clear documentation.
Consider adding documentation comments to the JSON file explaining the purpose of each major configuration section, particularly for test-specific values that differ from production settings. This would improve clarity for developers using this configuration.
Example JSON with comments (in a separate documentation file since JSON doesn't support comments):
# deploy-test-config.json Documentation ## Multisig Addresses All roles use the same address in test environment, but would be different in production. ## Strategies Max deposit values are set to maximum to avoid constraints during testing. ## Timing Parameters Most delays are minimized for faster test execution.🧰 Tools
🪛 Gitleaks (8.21.2)
12-12: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (6)
script/Deployments.s.sol(3 hunks)script/configs/deploy-test-config.json(1 hunks)src/operator-registries/TaiyiRegistryCoordinator.sol(2 hunks)test/DeployTest.t.sol(1 hunks)test/EigenLayerMiddleware.t.sol(1 hunks)test/SymbioticMiddleware.t.sol(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/operator-registries/TaiyiRegistryCoordinator.sol
- script/Deployments.s.sol
🧰 Additional context used
🪛 Gitleaks (8.21.2)
script/configs/deploy-test-config.json
12-12: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Foundry Project
3 participants
https://linear.app/luban-protocol/issue/LIN-39/update-taiyi-based-on-the-latest-linglong-changes-with-slashing
Summary by CodeRabbit
New Features
Improvements
Bug Fixes