Tests on client code #1
Conversation
…unctionality reuse. * KontrolTest has basic helper functions, such as _establish and _storeUInt256, etc. * StorageSetup has the ...StorageSetup functions, refactored to receive the relevant contracts as parameters. * DualGovernanceSetUp has the storage variables for tests of the DualGovernance contract and the setUp function initializing them.
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (1)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including You can disable this status message by setting the 📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces several modifications across multiple Solidity contracts and test files. Key changes include the enhancement of method signatures in the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant EscrowModel
participant DualGovernanceModel
User->>EscrowModel: lock(amount)
EscrowModel->>User: confirm lock
User->>EscrowModel: unlock()
EscrowModel->>User: confirm unlock
User->>DualGovernanceModel: submitProposal(details)
DualGovernanceModel->>User: confirm proposal submission
User->>DualGovernanceModel: activateNextState()
DualGovernanceModel->>User: confirm state activation
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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: 56
🧹 Outside diff range and nitpick comments (32)
contracts/model/WithdrawalQueueModel.sol (3)
7-7: Consider initializing _lastFinalizedRequestId and adding a comment.The
_lastFinalizedRequestIdvariable is crucial for tracking withdrawal requests. Consider the following suggestions:
- Initialize it explicitly (e.g., to 0 or another appropriate starting value) to make the initial state clear.
- Add a comment explaining its purpose and any assumptions about its value range.
Example:
/// @dev Tracks the ID of the last finalized withdrawal request. Starts at 0 and increases monotonically. uint256 private _lastFinalizedRequestId = 0;This improves code readability and helps prevent potential misunderstandings about the variable's intended use.
9-11: LGTM: Add natspec documentation to the function.The
getLastFinalizedRequestIdfunction is well-implemented, providing controlled access to the private state variable. To improve documentation and developer experience, consider adding natspec comments:/// @notice Returns the ID of the last finalized withdrawal request /// @return The last finalized request ID function getLastFinalizedRequestId() external view returns (uint256) { return _lastFinalizedRequestId; }This addition will enhance the contract's readability and provide better context for developers interacting with the contract.
1-12: Consider adding functionality to update _lastFinalizedRequestId.The
WithdrawalQueueModelcontract provides a good foundation for managing STETH withdrawal limits and tracking finalized requests. However, it's missing functionality to update the_lastFinalizedRequestId. Consider adding a function like:function updateLastFinalizedRequestId(uint256 newId) external onlyAuthorized { require(newId > _lastFinalizedRequestId, "New ID must be greater than current"); _lastFinalizedRequestId = newId; }Ensure to implement proper access control (e.g.,
onlyAuthorizedmodifier) to prevent unauthorized updates. This addition would complete the basic functionality expected from this contract.Also, consider adding events to log important state changes, which can be useful for off-chain services and improved transparency.
Would you like assistance in implementing these suggestions or creating related test cases?
contracts/interfaces/IStETH.sol (2)
18-18: Approve the addition ofsharesOffunction.The new
sharesOffunction is a valuable addition to the interface. It allows querying the number of shares held by a specific account, which enhances the overall functionality of the contract.Consider adding a NatSpec comment to document the purpose and behavior of this function. For example:
/// @notice Returns the number of shares owned by the specified account /// @param _account The address of the account to query /// @return The number of shares owned by the account function sharesOf(address _account) external view returns (uint256);
Line range hint
1-20: Overall assessment: Changes enhance the IStETH interface.The modifications to this interface file are well-structured and improve its functionality. The addition of a return value to
transferSharesand the newsharesOffunction provide more capabilities to interacting contracts. These changes align well with the PR objective of enhancing tests on client code, as they allow for more comprehensive testing of share-related operations.As you continue to develop this system, consider the following:
- Ensure that all functions in this interface have clear, consistent error handling mechanisms.
- If not already present, consider adding events for significant state changes to facilitate off-chain tracking and improve transparency.
- As the interface grows, you might want to consider splitting it into smaller, more focused interfaces if it becomes too large or if certain functions are only used by specific types of clients.
test/kontrol/RageQuit.t.sol (2)
1-8: LGTM! Consider adding NatSpec comments.The contract declaration and imports look good. The use of an up-to-date Solidity version (0.8.23) is commendable. The inheritance from
DualGovernanceSetUpsuggests good test organization.Consider adding NatSpec comments to describe the purpose of the
RageQuitTestcontract. This would improve documentation and make the test's intent clearer.
9-20: Good test structure. Consider enhancing test coverage.The
testRageQuitDurationfunction is well-structured and clearly tests the RageQuit duration logic. However, there are a few areas where the test coverage could be improved:
The test only asserts the state when the timestamp is within the RageQuit duration. Consider adding an assertion for the case when the timestamp is beyond this duration.
The test assumes the initial state is RageQuit but doesn't verify this assumption. It might be beneficial to assert this condition explicitly.
The test activates the next state but doesn't verify what this next state is. It might be useful to check if the state changes as expected when outside the RageQuit duration.
Here's a suggested improvement to enhance the test coverage:
function testRageQuitDuration() external { vm.assume(dualGovernance.getCurrentState() == State.RageQuit); + assert(dualGovernance.getCurrentState() == State.RageQuit, "Initial state should be RageQuit"); dualGovernance.activateNextState(); uint40 rageQuitTimelockStartedAt = _getRageQuitTimelockStartedAt(rageQuitEscrow); uint32 rageQuitExtensionDelay = _getRageQuitExtensionDelay(rageQuitEscrow); if (block.timestamp <= rageQuitTimelockStartedAt + rageQuitExtensionDelay) { assert(dualGovernance.getCurrentState() == State.RageQuit, "State should remain RageQuit within duration"); + } else { + assert(dualGovernance.getCurrentState() != State.RageQuit, "State should change after RageQuit duration"); } }This change adds assertions to verify the initial state and the state transition after the RageQuit duration, providing more comprehensive test coverage.
test/kontrol/VetoCooldown.t.sol (1)
12-22: Add comments to improve code readability.Consider adding comments to explain the purpose of each step in the test. This will make the test easier to understand and maintain. For example:
function testVetoCooldownDuration() external { // Ensure the initial state is VetoCooldown vm.assume(dualGovernance.getCurrentState() == State.VetoCooldown); // Calculate the maximum cooldown duration Timestamp timeEnteredAt = Timestamp.wrap(_getEnteredAt(dualGovernance)); Timestamp maxCooldownDuration = addTo(config.VETO_COOLDOWN_DURATION(), timeEnteredAt); // Attempt to activate the next state dualGovernance.activateNextState(); // Check if we're still in VetoCooldown and if the duration has elapsed bool stillInVetoCooldown = (dualGovernance.getCurrentState() == State.VetoCooldown); bool durationHasElapsed = (Timestamps.now() > maxCooldownDuration); // Assert that we're not in VetoCooldown if and only if the duration has elapsed assert(stillInVetoCooldown != durationHasElapsed); }test/kontrol/scripts/run-kontrol.sh (1)
37-38: Consider quoting$LOG_PATHand the implications of suppressing tar output.
- On lines 37-38, consider adding quotes around
$LOG_PATHto handle paths with spaces:- if [ ! -d $LOG_PATH ]; then - mkdir $LOG_PATH + if [ ! -d "$LOG_PATH" ]; then + mkdir "$LOG_PATH"
- On line 48, suppressing tar output might hide potential errors. Consider redirecting stderr to a log file instead of /dev/null:
- tar -xzvf "$RESULTS_LOG" > /dev/null 2>&1 + tar -xzvf "$RESULTS_LOG" > /dev/null 2>> "$LOG_PATH/tar_errors.log"This way, you can still check for any extraction errors if needed.
Also applies to: 48-48
test/kontrol/scripts/common.sh (3)
Line range hint
175-181: LGTM with a suggestion: Consider adding error handlingThe changes to the
clean_dockerfunction simplify the process and potentially improve reliability by adding a small delay after stopping the container. This is a good improvement.However, consider adding error handling for the
docker stopcommand. For example:if ! docker stop "$CONTAINER_NAME" > /dev/null 2>&1; then notif "Failed to stop container $CONTAINER_NAME" fiThis would provide more informative feedback if the stop operation fails.
Line range hint
35-181: LGTM with a suggestion: Consider using boolean values consistentlyThe updates to the argument parsing and execution mode selection logic are well-implemented and improve the script's robustness. The handling of different scenarios (container, local, dev, script, custom tests) is now more comprehensive.
One small suggestion for improvement:
Consider usingtrueandfalseconsistently for boolean variables instead of mixing string comparisons and boolean values. For example, replace:if [ "$LOCAL" = false ]; thenwith:
if [ "$LOCAL" = "false" ]; thenThis would make the code more consistent and potentially less error-prone.
Line range hint
92-124: LGTM with a suggestion: Consider refactoring for improved readabilityThe updates to the
parse_first_argfunction significantly improve the script's flexibility, particularly in handling different test execution scenarios. The new logic for the "script" argument is a valuable addition.However, the function's complexity has increased. Consider refactoring it to improve readability and maintainability. For example:
- Extract the logic for each argument type into separate functions.
- Use a case statement instead of multiple if-elif blocks.
- Consider using associative arrays to map arguments to their corresponding actions.
This refactoring could make the code easier to understand and maintain in the future.
test/kontrol/DualGovernanceSetUp.sol (1)
49-55: Ensure consistent naming in storage labels for better readabilityThe storage labels transition from
// ?STORAGEto// ?STORAGE0. For clarity and consistency, consider updating the first storage label to// ?STORAGE0or adjust subsequent labels accordingly.contracts/model/WstETHAdapted.sol (4)
79-85: Remove unnecessary commented-out codeThere is a commented-out
receivefunction present in the contract. If this code is not intended for future use, consider removing it to keep the codebase clean and maintainable.Apply this diff to remove the commented-out code:
/* receive() external payable { uint256 shares = stETH.submit{value: msg.value}(address(0)); _mint(msg.sender, shares); } */
114-114: Correct typo in the documentation commentIn the return description of the
tokensPerStEthfunction, remove the unnecessary article "a" for clarity.Apply this diff:
* @return Amount of wstETH for a 1 stETH - * @return Amount of wstETH for a 1 stETH + * @return Amount of wstETH for 1 stETH
42-43: Improve wording in documentation commentConsider updating the description to "Exchanges stETH for wstETH" for clearer wording.
Apply this diff:
* @notice Exchanges stETH to wstETH - * @notice Exchanges stETH to wstETH + * @notice Exchanges stETH for wstETH
61-62: Improve wording in documentation commentConsider updating the description to "Exchanges wstETH for stETH" for clearer wording.
Apply this diff:
* @notice Exchanges wstETH to stETH - * @notice Exchanges wstETH to stETH + * @notice Exchanges wstETH for stETHcontracts/model/StETHModel.sol (2)
74-79: Clarify error message indecreaseAllowancefunctionThe error message "ALLOWANCE_BELOW_ZERO" in the
decreaseAllowancefunction may be misleading. It suggests that the allowance itself is below zero, which is impossible since uint256 variables cannot be negative. The actual issue is that the subtraction would result in a negative allowance.Consider updating the error message to more accurately reflect the condition:
uint256 currentAllowance = allowances[msg.sender][_spender]; - require(currentAllowance >= _subtractedValue, "ALLOWANCE_BELOW_ZERO"); + require(currentAllowance >= _subtractedValue, "DECREASED_ALLOWANCE_BELOW_ZERO");
4-4: Ensure comprehensive documentation and commentsWhile the code includes some comments, adding NatSpec documentation for public and external functions can improve code readability and facilitate usage by other developers and tools.
Consider adding function-level comments using the Ethereum Natural Specification Format (NatSpec):
+/// @notice Sets the total pooled Ether +/// @param _value The new total pooled Ether value function setTotalPooledEther(uint256 _value) external onlyOwner {Apply similar documentation to other functions.
test/kontrol/EscrowAccounting.t.sol (2)
45-47: Remove or Clarify Obscure CommentsThe comments
// ?WORD: totalPooledEther,// ?WORD0: totalShares, and// ?WORD1: shares[escrow]are unclear and may confuse readers. They do not provide meaningful information about the code and may clutter the codebase.Consider removing these comments or replacing them with descriptive comments that clarify their purpose.
58-67: Clarify the Purpose of Sequential Comments with '?WORD' PrefixThe comments from lines 58 to 67 (e.g.,
// ?WORD4: lockedShares,// ?WORD5: claimedETH, etc.) are not standard and may be confusing. These comments appear to reference storage slots or symbolic variables without clear context.Please clarify the intent of these comments or remove them if they are not necessary. If they are placeholders or references for symbolic execution tools, consider documenting their purpose for future maintainability.
test/kontrol/EscrowOperations.t.sol (1)
115-162: Offer assistance: Implement 'testCannotWithdrawBeforeEthClaimTimelockElapsed'The test function
testCannotWithdrawBeforeEthClaimTimelockElapsedis currently commented out with a TODO note to adapt it to the client code.Would you like assistance in adapting this test function to the current codebase? I can help implement the required changes to ensure the test accurately verifies that a user cannot withdraw funds before the
RageQuitEthClaimTimelockhas elapsed.test/kontrol/ProposalOperationsSetup.sol (3)
23-63: Add function comments to_initializeAuxDualGovernance()to explain its purpose and implementation details.The
_initializeAuxDualGovernance()function performs complex initialization involving direct storage manipulation and bitmask operations. Adding a detailed function comment explaining its purpose, the rationale behind the storage manipulations, and any assumptions will enhance code readability and maintainability.
167-171: Add comments to explain storage slot calculations in_getProposalsSlot().The
_getProposalsSlot()function uses specific calculations involving magic numbers to compute storage slots. Adding comments to explain the logic behind these calculations and the significance of the numbers used will enhance code readability.
83-94: Clarify the purpose of inline assembly when storing slot 3.In lines 83 to 94, inline assembly is used to manipulate and store a
bytes32value into storage. Providing comments to explain whatslot3Abirepresents and detailing the steps within the assembly block will help maintainers understand the code's intent.test/kontrol/lido-lemmas.k (2)
49-71: Consider removing or documenting large blocks of commented-out codeLines 49-71 contain a sizeable block of commented-out code related to the
[create-valid-enhanced]rule. If this code is deprecated or no longer necessary, consider removing it to improve code clarity. If it's meant to be retained for future reference, adding a comment explaining its purpose would be beneficial.
135-135: Maintain consistent variable naming conventionsAt line 135, variables
_WORD4:Intand_WORD3:Intare used with leading underscores, whereas elsewhere, variables likeWORD4:Intare used without underscores. For consistency and readability, consider standardizing the variable naming convention throughout the code.test/kontrol/EscrowLockUnlock.t.sol (1)
245-251: Clean up commented-out assertions to maintain code quality.There are commented-out assertions between lines 245 and 251. If these assertions are no longer needed, consider removing them. If they are kept intentionally, add comments explaining their purpose.
contracts/model/DualGovernanceModel.sol (3)
56-56: Potential inconsistency in parameter namingIn the constructor, the parameter is named
emergencyProtectionTimelock, but the state variable isemergencyProtectedTimelock. For clarity and consistency, consider aligning the parameter name with the state variable.Suggested change:
-constructor(address _fakeETH, uint256 emergencyProtectionTimelock) { +constructor(address _fakeETH, uint256 _emergencyProtectedTimelock) { currentState = State.Normal; lastStateChangeTime = block.timestamp; fakeETH = _fakeETH; - emergencyProtectedTimelock = new EmergencyProtectedTimelockModel(address(this), emergencyProtectionTimelock); + emergencyProtectedTimelock = new EmergencyProtectedTimelockModel(address(this), _emergencyProtectedTimelock); signallingEscrow = new EscrowModel(address(this), _fakeETH); }
191-194: Redundant timestamp checksThe require statements:
require(lastStateChangeTime <= block.timestamp); require(lastSubStateActivationTime <= block.timestamp); require(lastStateReactivationTime <= block.timestamp);These checks may be redundant since it's unlikely for these timestamps to be in the future under normal conditions. Removing them could save gas and simplify the code.
240-247: Simplify conditional logic involving timestampsIn the conditions:
if ( block.timestamp != lastStateChangeTime && block.timestamp - lastStateChangeTime > DYNAMIC_TIMELOCK_MAX_DURATION && rageQuitSupport > SECOND_SEAL_RAGE_QUIT_SUPPORT ) { /* ... */ } if ( block.timestamp != lastStateChangeTime && block.timestamp - lastStateChangeTime > calculateDynamicTimelock(rageQuitSupport) && block.timestamp - Math.max(lastStateChangeTime, lastStateReactivationTime) > VETO_SIGNALLING_MIN_ACTIVE_DURATION ) { /* ... */ }The check
block.timestamp != lastStateChangeTimemight be unnecessary because the subsequent conditionblock.timestamp - lastStateChangeTime > Xwould be false when the timestamps are equal.Consider removing
block.timestamp != lastStateChangeTimeto simplify the condition.test/kontrol/VetoSignalling.t.sol (1)
251-274: Clarify complex control flow with additional comments.The control flow within the
ifstatements from lines 251-274 is intricate and may benefit from additional comments explaining each branch's purpose. This will aid in understanding and maintaining the code in the future.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (4)
.github/workflows/lido-ci.ymlis excluded by!**/*.ymlfoundry.tomlis excluded by!**/*.tomlkontrol.tomlis excluded by!**/*.tomltest/kontrol/scripts/versions.jsonis excluded by!**/*.json
📒 Files selected for processing (26)
- contracts/interfaces/IStETH.sol (1 hunks)
- contracts/libraries/DualGovernanceState.sol (4 hunks)
- contracts/libraries/WithdrawalBatchesQueue.sol (1 hunks)
- contracts/model/DualGovernanceModel.sol (14 hunks)
- contracts/model/EmergencyProtectedTimelockModel.sol (2 hunks)
- contracts/model/EscrowModel.sol (1 hunks)
- contracts/model/StETHModel.sol (1 hunks)
- contracts/model/WithdrawalQueueModel.sol (1 hunks)
- contracts/model/WstETHAdapted.sol (1 hunks)
- lib/kontrol-cheatcodes (1 hunks)
- test/kontrol/ActivateNextState.t.sol (1 hunks)
- test/kontrol/DualGovernanceSetUp.sol (1 hunks)
- test/kontrol/EscrowAccounting.t.sol (1 hunks)
- test/kontrol/EscrowInvariants.sol (1 hunks)
- test/kontrol/EscrowLockUnlock.t.sol (1 hunks)
- test/kontrol/EscrowOperations.t.sol (1 hunks)
- test/kontrol/KontrolTest.sol (1 hunks)
- test/kontrol/ProposalOperations.t.sol (1 hunks)
- test/kontrol/ProposalOperationsSetup.sol (1 hunks)
- test/kontrol/RageQuit.t.sol (1 hunks)
- test/kontrol/StorageSetup.sol (1 hunks)
- test/kontrol/VetoCooldown.t.sol (1 hunks)
- test/kontrol/VetoSignalling.t.sol (7 hunks)
- test/kontrol/lido-lemmas.k (4 hunks)
- test/kontrol/scripts/common.sh (2 hunks)
- test/kontrol/scripts/run-kontrol.sh (4 hunks)
✅ Files skipped from review due to trivial changes (1)
- lib/kontrol-cheatcodes
🧰 Additional context used
🪛 Shellcheck
test/kontrol/scripts/run-kontrol.sh
[warning] 16-16: GHCRTS appears unused. Verify use (or export if used externally).
(SC2034)
🔇 Additional comments (45)
contracts/model/WithdrawalQueueModel.sol (2)
1-1: LGTM: Pragma statement is appropriate.Using a specific, recent Solidity version (0.8.23) is a good practice. It ensures consistency across different environments and takes advantage of the latest language features and bug fixes.
4-5: Consider increasing the MIN_STETH_WITHDRAWAL_AMOUNT.The current minimum withdrawal amount of 100 wei (approximately 1.8e-16 STETH) seems impractically low for real-world use. Consider increasing this to a more meaningful value to prevent dust amounts and reduce unnecessary transactions.
The use of constants for these limits is excellent for gas optimization and easier future updates. Ensure these values align with your business requirements and economic model.
To verify the appropriateness of these constants, please run the following script:
contracts/interfaces/IStETH.sol (1)
12-12: Approve the addition of return value totransferShares.The updated function signature now includes a
uint256return value, which is a good improvement. This change provides more information to the caller, potentially the number of shares transferred or another relevant value.Please ensure that the implementation of this function and any contracts calling it have been updated accordingly. Run the following script to check for potential inconsistencies:
test/kontrol/VetoCooldown.t.sol (1)
1-10: LGTM: File structure and imports are well-organized.The file structure is clean, and the imports are appropriate for the test's functionality. The use of the latest Solidity version (0.8.23) is commendable.
test/kontrol/scripts/run-kontrol.sh (4)
70-70: Good addition of TERM signal to the trap command.Adding the
TERMsignal to the trap command is a positive change. It allows the script to handle termination signals gracefully, improving its robustness.
24-28: Simplifiedkontrol_provefunction looks good, but verify impact.The
kontrol_provefunction has been simplified in the same manner askontrol_build. This consistency is good for maintainability. However, please ensure that:
- The removed options are not necessary for the prove process.
- If the options are still needed, they are set elsewhere (e.g., in a configuration file).
To check if the removed options are used elsewhere, run:
#!/bin/bash # Description: Search for previously used kontrol prove options in shell scripts rg --type sh '(run kontrol prove.*--verbose|--require|--module-import|--max-depth|--max-iterations|--smt-timeout|--workers)' .
16-21: Simplifiedkontrol_buildfunction looks good, but verify impact.The
kontrol_buildfunction has been significantly simplified by removing all command-line options. While this improves readability, please ensure that:
- The removed options are not necessary for the build process.
- If the options are still needed, they are set elsewhere (e.g., in a configuration file).
To check if the removed options are used elsewhere, run:
#!/bin/bash # Description: Search for previously used kontrol build options in shell scripts rg --type sh '(run kontrol build.*--verbose|--require|--module-import|--max-depth|--max-iterations|--smt-timeout|--workers)' .🧰 Tools
🪛 Shellcheck
[warning] 16-16: GHCRTS appears unused. Verify use (or export if used externally).
(SC2034)
16-16: Verify the usage of theGHCRTSvariable.The
GHCRTSvariable is declared but appears to be unused in the visible parts of the script. If it's not used elsewhere, consider removing it to improve code clarity.To check for any usage of
GHCRTSin the project, run the following command:✅ Verification successful
GHCRTS variable is unused and can be removed.
The
GHCRTSvariable is declared inrun-kontrol.shbut is not used anywhere else in the script or the codebase. Removing it will improve code clarity.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for usage of GHCRTS variable in shell scripts rg --type sh 'GHCRTS' .Length of output: 128
Script:
#!/bin/bash # Description: Comprehensive search for usage of GHCRTS variable in shell scripts # Search for direct usage rg --type sh 'GHCRTS' . # Search for exported usage rg --type sh 'export GHCRTS' . # Search for usage within strings or other contexts rg --type sh '"GHCRTS"' .Length of output: 132
🧰 Tools
🪛 Shellcheck
[warning] 16-16: GHCRTS appears unused. Verify use (or export if used externally).
(SC2034)
contracts/libraries/WithdrawalBatchesQueue.sol (1)
18-27: Approve the relocation of theStatusenum with a note on potential impact.The relocation of the
Statusenum inside theWithdrawalsBatchesQueuelibrary improves encapsulation and reduces global namespace pollution. This change is syntactically correct and aligns with best practices for library design in Solidity.However, this is a breaking change if the
Statusenum was previously used outside this library. Ensure that all references to this enum throughout the codebase have been updated accordingly.To verify the impact of this change, run the following script:
✅ Verification successful
Verified: No external issues found with the relocation of the
Statusenum.The
Statusenum has been successfully relocated inside theWithdrawalsBatchesQueuelibrary. All references in the codebase, including those intest/kontrol/StorageSetup.solandcontracts/libraries/Proposals.sol, correctly utilize the enum through the library without introducing any conflicts or breaking changes.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any external usage of the Status enum outside the WithdrawalBatchesQueue library # Test: Search for Status enum usage outside the WithdrawalBatchesQueue library rg --type solidity -g '!WithdrawalBatchesQueue.sol' 'enum\s+Status|WithdrawalsBatchesQueue\.Status' # Note: If this search returns results, it indicates that the Status enum is being used elsewhere # in the codebase and those occurrences need to be updated.Length of output: 134
Script:
#!/bin/bash # Description: Check for any external usage of the Status enum outside the WithdrawalBatchesQueue library # Test: Search for Status enum usage in .sol files excluding WithdrawalBatchesQueue.sol rg -g '*.sol' -g '!WithdrawalBatchesQueue.sol' 'enum\s+Status|WithdrawalsBatchesQueue\.Status'Length of output: 390
test/kontrol/scripts/common.sh (2)
35-37: LGTM: Improved clarity in commentThe updated comment provides clearer context about the location of the
foundry.tomlfile, which is helpful for users and developers.
Line range hint
7-22: LGTM: Improved usage instructionsThe updates to the
usage_run_kontrolfunction significantly improve the clarity of the usage instructions. The refined descriptions for execution modes, particularly the recommendation for the "local" mode, provide valuable guidance to users. This change will help users make more informed decisions about which mode to use, potentially leading to better reproducibility of test results.contracts/model/EmergencyProtectedTimelockModel.sol (2)
127-133: LGTM. Efficiency improvement incancelAllNonExecutedProposals.The addition of the
if (nextProposalId > 0)check before the loop is a good optimization. It prevents unnecessary iterations when there are no proposals, improving gas efficiency and avoiding potential issues that could arise from iterating over an empty set of proposals.
Line range hint
1-233: Summary: Minor improvements with no functional changes.The changes in this contract are minimal and beneficial:
- The contract has been renamed to better reflect its role as a model.
- An efficiency improvement has been added to the
cancelAllNonExecutedProposalsfunction.These changes do not alter the core functionality of the contract and do not introduce any security vulnerabilities or logical errors. The contract's behavior remains consistent with its previous version, with slight improvements in clarity and efficiency.
test/kontrol/KontrolTest.sol (1)
58-62: Logic for overflow assumption is correct.The
_assumeNoOverflowfunction correctly uses unchecked arithmetic to assume thataugend + addenddoes not overflow. The use ofvm.assumewith the conditionaugend < augend + addendensures that the sum does not wrap around, which is appropriate.test/kontrol/EscrowInvariants.sol (1)
47-51: Verify the correctness of the user invariant checkIn the
escrowUserInvariantsfunction, the invariant ensures that a user's locked shares do not exceed the total locked shares:_establish( mode, escrow.getVetoerState(user).stETHLockedShares <= escrow.getLockedAssetsTotals().stETHLockedShares );While this check seems logical, please verify that the aggregation of all users'
stETHLockedSharescorrectly accounts for the totalstETHLockedShares, and that there are no edge cases where this inequality might not hold due to rounding errors or synchronization issues.To automate this verification, you can run the following script to ensure that the sum of all users'
stETHLockedSharesdoes not exceedtotals.stETHLockedShares:Please adjust the script as necessary to fit the actual structure of your codebase and data sources.
test/kontrol/DualGovernanceSetUp.sol (1)
28-28: Verify ifsetUp()function should be marked withoverrideSince
DualGovernanceSetUpinherits fromStorageSetup, ifStorageSetupdefines asetUp()function, thesetUp()function here should be marked with theoverridekeyword to correctly override the base function.Run the following script to verify if
StorageSetupcontains asetUp()function:contracts/model/StETHModel.sol (1)
150-153: Re-evaluate the restriction on transferring shares to the contract addressIn the
_transferSharesfunction, there is a requirement that prevents transferring shares to the contract itself:require(_recipient != address(this), "TRANSFER_TO_STETH_CONTRACT");Determine if this restriction is necessary. In some ERC20 implementations, allowing transfers to the contract address is acceptable and can be useful. If there's no specific reason to prevent this, consider removing this restriction.
If you decide to remove it, modify the code as follows:
-require(_recipient != address(this), "TRANSFER_TO_STETH_CONTRACT");test/kontrol/EscrowAccounting.t.sol (1)
19-25: Contract Initialization Appears CorrectThe
EscrowAccountingTestcontract is properly declared, extendingEscrowInvariants, and the state variables are appropriately defined.test/kontrol/EscrowOperations.t.sol (1)
28-47: Function 'testCannotUnlockBeforeMinLockTime' is correctly implementedThe test function successfully verifies that unlocking stETH before the minimum lock time has elapsed reverts as expected.
test/kontrol/ActivateNextState.t.sol (1)
16-80: Ensure proper casting and type safety when usingaddress(this)In the
activateNextStatefunction,address(this)is cast toDualGovernance. Ensure that this contract correctly implements or simulates theDualGovernanceinterface to avoid runtime errors due to incorrect type casting.To verify that
ActivateNextStateMockis compatible withDualGovernance, check the inheritance hierarchy:contracts/model/EscrowModel.sol (2)
185-202: Ensure proper handling of Ether in the contractGiven that the contract primarily deals with
stETHtokens, and considering thatstEthis an ERC20 token representing staked Ether, the contract should ensure a mechanism to handle conversions betweenstETHand ETH if it intends to transfer ETH during withdrawals. Without such a mechanism, thewithdraw()function may fail or behave unexpectedly.Would you like assistance in designing a mechanism to handle
stETHto ETH conversion for withdrawals?
185-202:⚠️ Potential issuePotential mismatch between stETH holdings and ETH balance for withdrawals
In the
withdraw()function, the contract attempts to transfer ETH to the user based on their staked shares:uint256 totalEth = address(this).balance; // Total ETH held by contract uint256 amount = stEth.getPooledEthByShares(stakedShares); require(totalEth >= amount, "Not enough balance."); shares[msg.sender] = 0; // Transfer ETH equivalent payable(msg.sender).transfer(amount);However, the contract holds
stETH, not ETH. Unless there is an internal mechanism that swapsstETHfor ETH and ensures the contract has sufficient ETH balance, this withdrawal may fail due to insufficient ETH in the contract.Run the following script to verify if the contract receives ETH or swaps
stETHto ETH:test/kontrol/lido-lemmas.k (8)
4-4: Ensure the use of the[symbolic]attribute is appropriateThe addition of the
[symbolic]attribute to theLIDO-LEMMASmodule at line 4 changes its operational context to work with symbolic execution. Please verify that this change aligns with the intended use of the module and that all dependent modules and specifications are compatible with this alteration.
14-14: Extension ofStepSortsyntax to includeMapandSetAdding
MapandSetto theStepSortsyntax at line 14 extends the types that can be processed by the module. This change seems appropriate if mappings and sets are now being handled within the lemmas. Ensure that all usages ofStepSortare updated accordingly.
23-26: Validation of the arithmetic rule involving division and multiplicationThe rule at lines 23-26 correctly transforms
C <=Int A *Int BintoC /Int A <=Int Bunder the given conditions. The requires clauses ensure non-negative values and proper divisibility, making the simplification valid.
28-30: Correct simplification of equality between positive and negative integersThe rule at lines 28-30 asserts that
A ==Int BisfalsewhenA >= 0andB < 0. This is logically sound, as a non-negative integer cannot equal a negative integer, and helps in simplifying expressions under these conditions.
32-33: Simplification of inequality involving subtractionThe rule at lines 32-33 simplifies
0 <=Int A -Int BtoB <=Int A. This transformation is valid and serves to streamline inequalities within the symbolic reasoning.
35-38: Verify the complex arithmetic simplification for correctnessThe rule at lines 35-38 introduces a complex simplification involving integer division and multiplication with specific constraints on
A,D, andC. Given the complexity, please verify that this transformation accurately preserves the intended arithmetic relationships and does not introduce errors.
40-43: Validation of byte array comparison ruleThe
[asWord-lt-concat-left]rule at lines 40-43 transforms a comparison of concatenated byte arrays into a comparison involving only the left byte array and adjusted integer values. This seems correct under the specified requires clause and enhances the efficiency of symbolic comparisons.
121-125: Ensure intentional multiple updates to the same map keyIn the
claim [storage-simplification]starting at line 119, theMapSTORAGE0is updated multiple times at lines 121, 123, and 125 with the key5. This results in the previous values being overwritten. Please confirm that this is intentional and that the overwriting does not affect the lemma's correctness.test/kontrol/EscrowLockUnlock.t.sol (4)
25-28: Ensurevmandkevmare properly initialized.The code uses
vm.assumewithaddress(vm)andaddress(kevm). Verify that bothvmandkevmare correctly initialized and available in the testing context to avoid runtime issues.
223-225: Verify the behavior of theActivateNextStateMockcontract.When mocking the
activateNextStatefunction, ensure that the mock accurately represents the desired behavior for the test. This verification helps prevent misleading test results.
162-164: Check the assumption onstEth.balanceOf(sender).The assumption
stEth.balanceOf(sender) < ethUpperBounddepends onethUpperBound. Verify thatethUpperBoundis set appropriately to avoid invalid test scenarios.
198-201: Validate time-based assumptions for unlocking stETH.The test assumes that the current time is greater than the last locked time plus the minimum lock time (
Timestamps.now() > addTo(config.SIGNALLING_ESCROW_MIN_LOCK_TIME(), pre.userLastLockedTime)). Ensure that this condition is met in the test environment to prevent incorrect test failures.contracts/model/DualGovernanceModel.sol (5)
90-90: Verify tuple unpacking in proposal retrievalThe tuple unpacking in the line:
(,, uint256 submissionTime,,) = emergencyProtectedTimelock.proposals(proposalId);Ensure that the syntax is correct and that you're retrieving the intended value. Unused variables are ignored using commas, but verify that the positions align with the returned values from the
proposalsfunction.Check the
proposalsfunction signature to confirm the order of returned values.
298-298: MakingfromRageQuitfunction public may introduce security risksChanging the
fromRageQuitfunction fromprivatetopublicexposes it to external calls. This could allow unauthorized state transitions if not properly secured.Confirm that this change is intentional and that adequate access control is in place to prevent misuse.
25-27: Check compatibility with updated model classesThe state variables have been updated to use the new model classes:
EmergencyProtectedTimelockModelforemergencyProtectedTimelockEscrowModelforsignallingEscrowandrageQuitEscrowPlease verify that all interactions with these variables are compatible with the new model classes and that any changes in their interfaces are handled appropriately.
Run the following script to review usage of these variables:
#!/bin/bash # Description: Find all usages of the updated variables rg --type solidity "(emergencyProtectedTimelock|signallingEscrow|rageQuitEscrow)" -A 3 -B 1
16-16: Update all references to the contract nameThe contract has been renamed from
DualGovernancetoDualGovernanceModel. Ensure that all references to the old contract name are updated throughout the codebase, including deployment scripts and documentation.Run the following script to find any remaining references to the old contract name:
#!/bin/bash # Description: Find all occurrences of "DualGovernance" in Solidity files rg --type solidity "DualGovernance\b" --files-with-matches
6-7: Ensure the new model contracts are properly integratedThe imports have been updated to use
EmergencyProtectedTimelockModelandEscrowModel. Please verify that these new contracts are correctly implemented and that any differences in their interfaces are accounted for in this contract.Run the following script to check for any mismatches in function signatures or properties used from the new contracts:
contracts/libraries/DualGovernanceState.sol (4)
5-5: Import of Math Library is AppropriateThe addition of
import {Math} from "@openzeppelin/contracts/utils/math/Math.sol";is necessary for utilizingMath.maxin timestamp calculations.
301-301: Verify Inclusion of Threshold Values in ComparisonsThe comparison operators in
_isFirstSealRageQuitSupportCrossedand_isSecondSealRageQuitSupportCrossedhave changed from>to>=. This adjustment means that the thresholdsfirstSealRageQuitSupportandsecondSealRageQuitSupportare now inclusive. Please confirm that this change aligns with the intended governance logic and that it correctly handles edge cases whererageQuitSupportequals the threshold values.Also applies to: 308-308
331-341: Review Logic in Veto Signalling Reactivation Duration CalculationThe
_isVetoSignallingReactivationDurationPassedfunction has been refactored to useMath.maxbetweenvetoSignallingActivationTimeandvetoSignallingReactivationTime. Ensure that this modification accurately reflects the intended logic for determining when the veto signalling reactivation duration has passed, particularly in scenarios where reactivation time may affect state transitions.
395-404:⚠️ Potential issuePrevent Potential Division by Zero in Dynamic Timelock Calculation
In the
_calcDynamicTimelockDurationfunction, there is a division operation:(secondSealRageQuitSupport - firstSealRageQuitSupport)To avoid a division by zero error, ensure that
secondSealRageQuitSupportis always greater thanfirstSealRageQuitSupport. Please verify that the configuration enforces this condition.test/kontrol/VetoSignalling.t.sol (2)
279-279:⚠️ Potential issueRemove unreachable code after
returnstatement.At line 279,
vm.assume(currentState == State.VetoSignalling || currentState == State.VetoSignallingDeactivation);appears after areturnstatement at line 277. This code is unreachable and should be removed to improve code clarity.Apply this diff to remove unreachable code:
} return; - vm.assume(currentState == State.VetoSignalling || currentState == State.VetoSignallingDeactivation); }Likely invalid or redundant comment.
97-98: Ensure safe casting when wrapping timestamps.In line 98,
Math.maxreturns auint256, which is cast touint40before wrapping into aTimestamp. SinceTimestampvalues areuint40, ensure that the casting does not result in overflow. Although inputs areuint40, explicit checks can prevent unexpected issues.Run the following script to verify that all unwrapped
Timestampvalues fit withinuint40:✅ Verification successful
Safe casting verified for
Timestampvalues.All
Timestamp.unwrapusages returnuint40values, ensuring that casting touint40does not cause overflow.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that all Timestamp unwraps result in uint40 values. # Test: Search for all unwrapping of Timestamps. rg --type solidity 'Timestamp.unwrap\(' # Expected: All usages should involve values within the uint40 range.Length of output: 74
Script:
#!/bin/bash # Description: Verify that all Timestamp unwraps result in uint40 values by searching .sol files. # Find all Solidity files and search for 'Timestamp.unwrap(' fd --extension sol --type file | xargs rg 'Timestamp.unwrap\('Length of output: 1826
Script:
#!/bin/bash # Description: Confirm that the Timestamp type is defined as uint40. # Search for the definition of Timestamp in Solidity files rg --files-with-matches 'type Timestamp' --type sol | xargs rg 'type Timestamp'Length of output: 165
| function testVetoCooldownDuration() external { | ||
| vm.assume(dualGovernance.getCurrentState() == State.VetoCooldown); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using vm.mockCall instead of vm.assume.
Using vm.assume doesn't guarantee that the initial state will be VetoCooldown. Consider using vm.mockCall or a similar method to explicitly set the initial state to VetoCooldown. This will ensure the test always starts in the correct state.
| Timestamp timeEnteredAt = Timestamp.wrap(_getEnteredAt(dualGovernance)); | ||
| Timestamp maxCooldownDuration = addTo(config.VETO_COOLDOWN_DURATION(), timeEnteredAt); | ||
|
|
||
| dualGovernance.activateNextState(); | ||
|
|
||
| bool stillInVetoCooldown = (dualGovernance.getCurrentState() == State.VetoCooldown); | ||
| bool durationHasElapsed = (Timestamps.now() > maxCooldownDuration); | ||
| assert(stillInVetoCooldown != durationHasElapsed); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add more specific assertions for better test coverage.
While the current assertion checks the relationship between the state and time, consider adding more specific assertions to validate:
- The initial state is indeed
VetoCooldown. - The
maxCooldownDurationis calculated correctly. - The state transition occurs at the expected time.
This will provide more comprehensive test coverage and make debugging easier if the test fails.
| @@ -26,7 +26,7 @@ struct Proposal { | |||
|
|
|||
| // This contract manages the timelocking of proposals with emergency intervention capabilities. | |||
| // It provides controls for entering and managing emergency states as well as executing proposals under normal and emergency conditions. | |||
| contract EmergencyProtectedTimelock { | |||
| contract EmergencyProtectedTimelockModel { | |||
There was a problem hiding this comment.
💡 Codebase verification
Incomplete contract renaming detected.
References to EmergencyProtectedTimelock still exist in the following files:
test/utils/scenario-test-blueprint.soltest/unit/EmergencyProtectedTimelock.t.soltest/kontrol/StorageSetup.soltest/kontrol/ProposalOperationsSetup.soltest/kontrol/ProposalOperations.t.soltest/kontrol/EscrowLockUnlock.t.soltest/kontrol/EscrowInvariants.soltest/kontrol/EscrowAccounting.t.soltest/kontrol/DualGovernanceSetUp.solcontracts/ResealManager.solcontracts/EmergencyProtectedTimelock.solcontracts/committees/EmergencyExecutionCommittee.solcontracts/committees/EmergencyActivationCommittee.sol
Please update these references to EmergencyProtectedTimelockModel to ensure consistency across the codebase.
🔗 Analysis chain
LGTM. Verify contract name usage across the codebase.
The contract renaming from EmergencyProtectedTimelock to EmergencyProtectedTimelockModel is appropriate as it better reflects the contract's role as a model. However, ensure that all references to this contract in other parts of the codebase have been updated accordingly.
Run the following script to check for any remaining references to the old contract name:
Adjust the search commands to target .sol files and re-verify contract references.
The previous search command did not recognize the solidity file type. Please update the search to target .sol files and rerun the verification scripts to ensure all references to EmergencyProtectedTimelockModel are correctly updated.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for any remaining references to the old contract name
# Test: Search for the old contract name. Expect: No results.
rg --type solidity "EmergencyProtectedTimelock"
# Test: Search for the new contract name. Expect: At least one result in this file.
rg --type solidity "EmergencyProtectedTimelockModel"
Length of output: 166
Script:
#!/bin/bash
# Description: Search for any remaining references to the old contract name using the correct file extension
# Test: Search for the old contract name. Expect: No results.
rg -g '*.sol' "EmergencyProtectedTimelock"
# Test: Search for the new contract name. Expect: At least one result in this file.
rg -g '*.sol' "EmergencyProtectedTimelockModel"
Length of output: 9345
test/kontrol/KontrolTest.sol
Outdated
| uint256 constant timeUpperBound = 2 ** 35; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add explicit visibility to constant declaration.
Similarly, the constant timeUpperBound lacks an explicit visibility modifier. Adding it enhances clarity.
-uint256 constant timeUpperBound = 2 ** 35;
+uint256 constant internal timeUpperBound = 2 ** 35;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| uint256 constant timeUpperBound = 2 ** 35; | |
| uint256 constant internal timeUpperBound = 2 ** 35; | |
test/kontrol/KontrolTest.sol
Outdated
| uint256 constant ethMaxWidth = 96; | ||
| uint256 constant ethUpperBound = 2 ** ethMaxWidth; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add explicit visibility to constant declarations.
The constants ethMaxWidth and ethUpperBound do not have explicit visibility specified. In Solidity, it's good practice to declare visibility for state variables and constants to improve code readability and maintainability.
Consider specifying the visibility as internal or public:
-uint256 constant ethMaxWidth = 96;
-uint256 constant ethUpperBound = 2 ** ethMaxWidth;
+uint256 constant internal ethMaxWidth = 96;
+uint256 constant internal ethUpperBound = 2 ** ethMaxWidth;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| uint256 constant ethMaxWidth = 96; | |
| uint256 constant ethUpperBound = 2 ** ethMaxWidth; | |
| uint256 constant internal ethMaxWidth = 96; | |
| uint256 constant internal ethUpperBound = 2 ** ethMaxWidth; |
test/kontrol/StorageSetup.sol
Outdated
| function _getCurrentState(DualGovernance _dualGovernance) internal view returns (uint8) { | ||
| return uint8(_loadUInt256(address(_dualGovernance), 5)); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider renaming overloaded _getCurrentState functions for clarity
The _getCurrentState function is defined twice with different parameters:
- Lines 59-61:
function _getCurrentState(DualGovernance _dualGovernance) internal view returns (uint8) - Lines 172-174:
function _getCurrentState(IEscrow _escrow) internal view returns (uint8)
While Solidity allows function overloading based on parameter types, having functions with the same name but different parameters can lead to confusion and potential misuse. Renaming one of the functions to reflect its specific context can improve code readability and maintainability.
Also applies to: 172-174
test/kontrol/StorageSetup.sol
Outdated
| uint8 currentState = uint8(kevm.freshUInt(1)); | ||
| vm.assume(currentState <= 4); | ||
| uint40 enteredAt = uint40(kevm.freshUInt(5)); | ||
| vm.assume(enteredAt <= block.timestamp); | ||
| vm.assume(enteredAt < timeUpperBound); | ||
| uint40 vetoSignallingActivationTime = uint40(kevm.freshUInt(5)); | ||
| vm.assume(vetoSignallingActivationTime <= block.timestamp); | ||
| vm.assume(vetoSignallingActivationTime < timeUpperBound); | ||
| bytes memory slot5Abi = abi.encodePacked( | ||
| uint8(0), | ||
| uint160(address(_signallingEscrow)), | ||
| uint40(vetoSignallingActivationTime), | ||
| uint40(enteredAt), | ||
| uint8(currentState) | ||
| ); | ||
| bytes32 slot5; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Define constants or enums for state values and storage slot numbers
In the dualGovernanceStorageSetup function, magic numbers are used for currentState and storage slot numbers:
currentStateis assumed to be<= 4(lines 90-91).- Storage slots
5and6are used directly (lines 90 and 110).
Consider defining an enum for currentState and constants for storage slot numbers to enhance readability and reduce the risk of errors.
Example:
enum GovernanceState { State0, State1, State2, State3, State4 }
uint256 constant SLOT_CURRENT_STATE = 5;
uint256 constant SLOT_ADDITIONAL_DATA = 6;
test/kontrol/StorageSetup.sol
Outdated
| // FIXME: This branching is done to avoid the fresh existential generation bug | ||
| if (_currentState == EscrowState.RageQuitEscrow) { |
There was a problem hiding this comment.
Address the FIXME comment and clarify the workaround
There's a FIXME comment indicating that branching is done to avoid the "fresh existential generation bug":
// FIXME: This branching is done to avoid the fresh existential generation bug
if (_currentState == EscrowState.RageQuitEscrow) {
// ...
}It's important to either resolve this issue or provide additional context about the workaround. This will help maintain code clarity and assist future developers in understanding the necessity of this branching.
Would you like assistance in addressing this FIXME comment or providing a more detailed explanation of the workaround? I can help refactor the code or open a GitHub issue to track this task.
test/kontrol/StorageSetup.sol
Outdated
| // FIXME: This branching is done to avoid the fresh existential generation bug | ||
| if (_currentState == EscrowState.RageQuitEscrow) { |
There was a problem hiding this comment.
Address the FIXME comment and clarify the workaround
Similar to the previous comment, there's a FIXME comment in the escrowStorageSetup function:
// FIXME: This branching is done to avoid the fresh existential generation bug
if (_currentState == EscrowState.RageQuitEscrow) {
// ...
}Ensuring that this issue is resolved or adequately documented will improve code maintainability and reduce potential confusion.
Would you like assistance in resolving this FIXME or enhancing the documentation around this workaround?
test/kontrol/StorageSetup.sol
Outdated
| return uint8(_loadUInt256(address(_escrow), 0)); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use descriptive constants for storage slot indices
In functions accessing storage slots directly, such as _getCurrentState and _getBatchesQueueStatus, hardcoded storage slot numbers are used:
_getCurrentStateuses slot0(line 173)._getBatchesQueueStatususes slot5(line 201).
Defining descriptive constants for these storage slot indices can improve code readability and reduce the likelihood of errors when modifying storage structures.
Example:
uint256 constant SLOT_ESCROW_CURRENT_STATE = 0;
uint256 constant SLOT_BATCHES_QUEUE_STATUS = 5;
// Usage:
return uint8(_loadUInt256(address(_escrow), SLOT_ESCROW_CURRENT_STATE));Also applies to: 201-202
No description provided.