Conversation
WalkthroughThe changes update time-based logic in both contract and test code, switching fraud proof window checks from block number to block timestamp. The Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant LinglongSlasher
participant Protocol
participant Blockchain
Caller->>LinglongSlasher: slash(delegation, commitment, committer, evidence, challenger)
LinglongSlasher->>LinglongSlasher: _prepareSlashingParams(...)
LinglongSlasher->>Protocol: _executeSlashingByProtocol(slashingParams)
Protocol-->>LinglongSlasher: slashing result
LinglongSlasher->>LinglongSlasher: mark commitment slashed
LinglongSlasher->>Caller: emit SlashingResult, return 0
Suggested reviewers
Poem
Tip ⚡️ Faster reviews with caching
Enjoy the performance boost—your workflow just got faster. 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: 0
🧹 Nitpick comments (2)
test/EigenLayerMiddleware.t.sol (1)
988-990: Avoid hard-coded “3 hours” magic number
fraudProofWindowis already stored in theRegistryconfig (7200 s).
Basing the warp on that value will keep the test correct even if the constant changes:- // Wait for the fraud proof window to pass - vm.warp(block.timestamp + 3 hours); + // Wait for fraud-proof window + 1 s to pass + vm.warp(block.timestamp + registry.getConfig().fraudProofWindow() + 1);test/SymbioticMiddleware.t.sol (1)
240-242: Use config value instead of literal “3 hours”Same reasoning as in the EigenLayer test: couple the warp to the configured window to avoid silent test failures when parameters evolve.
- // Wait for fraud proof window - vm.warp(block.timestamp + 3 hours); + // Wait past the fraud-proof window + vm.warp(block.timestamp + urcRegistry.getConfig().fraudProofWindow() + 1);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
lib/urc(1 hunks)src/libs/SlashingLib.sol(1 hunks)src/slasher/LinglongSlasher.sol(1 hunks)test/EigenLayerMiddleware.t.sol(1 hunks)test/SymbioticMiddleware.t.sol(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Foundry Project
🔇 Additional comments (3)
lib/urc (1)
1-1:Details
❓ Verification inconclusive
Submodule update looks correct; verify for breaking changes.
The
lib/urcsubproject pointer has been bumped to commit5f18225c4d027b49084605129b7666a37ea13412. No public interface changes are expected, but please confirm that:
- The new commit aligns with the intended timestamp-based logic updates.
- All dependent tests in
lib/urcstill pass.- There are no unintended API changes used by the slashing library.
🏁 Script executed:
#!/bin/bash # Fetch and diff the submodule changes git submodule update --init lib/urc cd lib/urc git fetch git diff e0c5c8a8212a04e8844df0268c7de17b9e6b8896 5f18225c4d027b49084605129b7666a37ea13412 # Run the submodule's test suite (if available) # e.g., cargo test || npm testLength of output: 70062
Confirm lib/urc submodule API & logic updates
The bump to commit 5f18225… introduces the following core changes in
lib/urc:
- Switched all on-chain timing from
block.number→block.timestampinRegistry.sol.- Updated
IRegistry&ISlasherinterfaces:
• Timestamp-based fields now useuint48/uint24widths.
• Removed the oldslashFromOptInmethod.
• Added acommitterparameter to theslashsignature.- Adjusted example contracts (
InclusionPreconfSlasher.sol,StateLockSlasher.sol) and scripts to match the new API.- Added
config/registry.json, expanded scripts folder, and updated Foundry settings.Please verify that:
- All downstream code (clients, slashing library, examples) has been updated to call the new
slash(..., committer, …)signature and no longer relies onslashFromOptIn.- There are no unintended API changes breaking upstream usage.
- All
lib/urctests still pass (e.g. viaforge testornpm test).- Integration/slashing tests in the main repo remain green.
src/libs/SlashingLib.sol (1)
308-310:Details
❓ Verification inconclusive
Possible unit-mismatch in fraud-proof window check
registeredAtis still stored as a block number elsewhere in the library (unregisteredAtis compared withblock.numbera few lines above).
By switching this clause toblock.timestampyou implicitly assume thatregisteredAtis now a Unix-timestamp. If that field still contains a block number, the comparison mixes blocks with seconds and the fraud-proof window will be orders of magnitude longer than intended.Action items
• Double-check howOperatorData.registeredAtis written inRegistryafter this change.
• If it is still a block number, convert it to seconds (or vice-versa) before the addition.
• Consider migrating every*_Atfield to a single time base to avoid future bugs (the adjacentunregisteredAtclause still usesblock.number!).Example patch if timestamps are desired everywhere:
// Treat all *_At fields as timestamps (seconds) - if (unregisteredAt < block.number) { + if (unregisteredAt < block.timestamp) { revert OperatorUnregistered(); } - if (registeredAt + registry.getConfig().fraudProofWindow > block.timestamp) { + if (uint256(registeredAt) + registry.getConfig().fraudProofWindow > block.timestamp) { revert OperatorFraudProofPeriodNotOver(); }
🏁 Script executed:
#!/bin/bash # Inspect every assignment to `registeredAt` to confirm whether # a timestamp or block number is stored. rg --no-heading -n '\.registeredAt\s*='Length of output: 146
🏁 Script executed:
#!/bin/bash # Retrieve the OperatorData struct definition to check types of registeredAt and unregisteredAt rg --no-heading -n 'struct OperatorData' -A10 # Search for assignments to registeredAt across the repository to see if it's set to block.number or block.timestamp rg --no-heading -n 'registeredAt\s*=' -g '*.sol' -A3Length of output: 1889
🏁 Script executed:
#!/bin/bash # Locate the OperatorData struct definition and its field types rg --no-heading -n 'struct OperatorData' -g 'src/**/*.sol' -A15 # Find the implementation of getOperatorData to see where OperatorData lives rg --no-heading -n 'function getOperatorData' -g 'src/**/*.sol' -A15 # Search for where operatorData.registeredAt is assigned in registry code rg --no-heading -n 'operatorData\.registeredAt\s*=' -g 'src/**/*.sol' -A5Length of output: 212
🏁 Script executed:
#!/bin/bash # Locate all references to OperatorData to find its struct definition and type of registeredAt rg --no-heading -n 'OperatorData' -g 'src/**/*.sol' # Fallback grep in case rg misses it grep -R -n 'struct OperatorData' srcLength of output: 1114
Verify OperatorData.registeredAt time unit before merging
I wasn’t able to locate the
struct OperatorDatadefinition in this repo (it lives in the externalIRegistryinterface), so please double-check whether itsregisteredAtfield is a block number or a Unix-timestamp. Mixing blocks with seconds here can drastically extend the fraud-proof window if you assume the wrong unit.Action items
• Confirm the exact type/unit ofIRegistry.OperatorData.registeredAtin the registry contract.
• If it’s still stored as a block number, convert it to seconds (or switch the comparison toblock.number) before addingfraudProofWindow.
• Consider standardizing all*_Atfields (e.g.registeredAt,unregisteredAt,slashedAt) on the same time base to prevent similar bugs in the future.src/slasher/LinglongSlasher.sol (1)
215-219: Great improvement to function signature readabilityChanging from unnamed to named parameters in the
slashfunction significantly improves code readability and maintainability. This change aligns with best practices for interface implementations, making it clearer to developers what each parameter represents while maintaining the same functionality.
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (2)
src/slasher/LinglongSlasher.sol (2)
425-443:⚠️ Potential issuePotential zero-ID slashing when operator is not allocated to EigenLayer middleware
operatorSetIddefaults to0. If the loop fails to find a matching set, the code continues withoperatorSetId == 0, meaning:
_checkSlashingNotInProgress()runs against set 0 (likely non-existent)._prepareEigenLayerSlashingParams()builds params for set 0, risking an unintended or failing slash.Add an explicit check after the loop to guarantee a valid set was found:
for (uint256 i = 0; i < eigenLayerSets.length; i++) { if (eigenLayerSets[i].avs == middleware) { operatorSetId = eigenLayerSets[i].id; break; } } -// proceeds even if operatorSetId == 0 +if (operatorSetId == 0) { + revert OperatorSetNotFound(); +}
486-499:⚠️ Potential issueSame missing-check issue for Symbiotic subnetworks
subnetworkis initialised to0and used unconditionally when no suitable subnetwork is found, leading to a silent mis-slash or revert inside the Symbiotic middleware.for (uint256 i = 0; i < subnetworks.length; i++) { ... } -if (subnetwork == 0) { - revert SubnetworkNotFound(); -}Introduce an explicit revert (or equivalent guard) before continuing.
🧹 Nitpick comments (2)
test/utils/MockChallenger.sol (1)
56-58: Replace the raw literal with a named constant for clarity & future-proofing
1e18is correct (it equals1 ether), but the intent—“100 % in WAD units” vs. “slash one ether”—remains implicit. Defining auint256 internal constant FULL_WAD = 1e18;(or similar) and returning that constant makes the purpose self-evident and avoids “magic numbers”.+uint256 internal constant FULL_WAD = 1e18; // 100 % in 18-dec WAD units function getSlashAmount() external pure returns (uint256) { - return 1e18; // 100 WAD percent + return FULL_WAD; }Please confirm that downstream middleware interprets the value as a percentage-style WAD and not an absolute token amount; otherwise a full-balance slash might unintentionally become “slash one ether”.
src/slasher/LinglongSlasher.sol (1)
225-232:delegation,committer, andchallengerparameters are unused – silences vs. semanticsThe new
_prepareSlashingParamssignature still acceptsDelegation calldata delegation, yet the variable is never touched; the same goes forcommitterandchallengerinslash().
Apart from compiler warnings, this is misleading to readers and might hide unfinished logic (e.g., verifying thatcommitter == commitment.slasher). At a minimum mark them_unusedto silence warnings; ideally remove or implement the intended checks.-function _prepareSlashingParams( - Delegation calldata delegation, +function _prepareSlashingParams( + Delegation calldata /* delegation */,Similar change inside
slash():- address committer, + address /* committer */, ... - address challenger + address /* challenger */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (5)
script/Deployments.s.sol(1 hunks)src/slasher/LinglongSlasher.sol(2 hunks)test/EigenLayerMiddleware.t.sol(2 hunks)test/utils/EigenlayerDeployer.sol(1 hunks)test/utils/MockChallenger.sol(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- script/Deployments.s.sol
🚧 Files skipped from review as they are similar to previous changes (1)
- test/EigenLayerMiddleware.t.sol
🔇 Additional comments (1)
test/utils/EigenlayerDeployer.sol (1)
111-111: Making MIN_WITHDRAWAL_DELAY public improves test accessibilityChanging the visibility modifier from internal to public is appropriate here. This allows external contracts and test code to directly access this constant value, which facilitates more precise control and readability in test scenarios involving withdrawal timing logic.
3 participants
Summary by CodeRabbit