Fix: ToB-04 (summit tip upper bound) #1432
Conversation
WalkthroughThe changes primarily focus on the implementation of a maximum limit for Changes
TipsChat with CodeRabbit Bot (
|
github-actions
bot
added
M-contracts
ChiTimesChi
force-pushed
the
fix/tob/04-summit-tip-upper-bound
branch
from
bfa2b10 to
4b65beb
Compare
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #1432 +/- ##
===================================================
+ Coverage 50.51623% 50.81805% +0.30181%
===================================================
Files 343 356 +13
Lines 24117 24204 +87
Branches 277 268 -9
===================================================
+ Hits 12183 12300 +117
+ Misses 10724 10695 -29
+ Partials 1210 1209 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Files selected for processing (4)
- packages/contracts-core/contracts/GasOracle.sol (4 hunks)
- packages/contracts-core/contracts/libs/Constants.sol (1 hunks)
- packages/contracts-core/contracts/libs/Errors.sol (1 hunks)
- packages/contracts-core/test/suite/GasOracle.GasData.t.sol (2 hunks)
Files skipped from review due to trivial changes (2)
- packages/contracts-core/contracts/libs/Constants.sol
- packages/contracts-core/contracts/libs/Errors.sol
Additional comments (Suppressed): 8
packages/contracts-core/test/suite/GasOracle.GasData.t.sol (4)
2-8: The import statement for
SummitTipTooHighfrom theErrors.sollibrary is new. Ensure that this error type is correctly defined and used in the contract.21-26: This test checks if the
setSummitTipfunction reverts when called by a non-owner. It's good to see tests for access control.28-30: This test verifies that the
setSummitTipfunction allows setting the summit tip to the upper bound (0.01 ether). This is a good check for boundary conditions.33-36: This test checks if the
setSummitTipfunction reverts when trying to set the summit tip higher than the upper bound. This is an important test to ensure the hard cap is enforced.packages/contracts-core/contracts/GasOracle.sol (4)
1-10: The import statements have been updated to include the
MAX_SUMMIT_TIPconstant fromConstants.soland theSummitTipTooHigherror fromErrors.sol. Ensure that these new imports are used correctly in the code.48-54: The
_summitTipWeivariable has been made public and renamed tosummitTipWei. This change increases transparency but also exposes this variable to external contracts. Make sure this is intended and does not introduce any security risks.87-93: The
setSummitTipfunction now checks if the providedsummitTipWei_exceedsMAX_SUMMIT_TIPand reverts with aSummitTipTooHigherror if it does. This is a good practice for enforcing limits and preventing misuse.- function setSummitTip(uint256 summitTipWei) external onlyOwner { - _summitTipWei = summitTipWei; - } + function setSummitTip(uint256 summitTipWei_) external onlyOwner { + if (summitTipWei_ > MAX_SUMMIT_TIP) revert SummitTipTooHigh(); + summitTipWei = summitTipWei_; + }
- 144-150: The calculation of
summitTipnow uses the publicsummitTipWeiinstead of the previously internal_summitTipWei. Ensure that all references to_summitTipWeiin the codebase have been updated tosummitTipWei.- uint256 summitTip = (_summitTipWei << NumberLib.BWAD_SHIFT) / localEtherPrice; + uint256 summitTip = (summitTipWei << NumberLib.BWAD_SHIFT) / localEtherPrice;
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Files selected for processing (1)
- packages/contracts-core/contracts/GasOracle.sol (4 hunks)
Additional comments (Suppressed): 3
packages/contracts-core/contracts/GasOracle.sol (3)
1-10: The import statements have been updated to include the
MAX_SUMMIT_TIPconstant fromConstants.soland theSummitTipTooHigherror fromErrors.sol. Ensure that these new imports do not conflict with existing code.48-54: The
_summitTipWeivariable has been made public and renamed assummitTipWei. This change increases transparency but also exposes the variable to external contracts. Make sure this does not introduce any security risks.145-151: The
calculateTotalCostfunction now usessummitTipWeiinstead of_summitTipWei. Ensure that all references to_summitTipWeiin the codebase have been updated tosummitTipWei.
| } | ||
|
|
||
| /// @notice MVP function to set the summit tip. | ||
| function setSummitTip(uint256 summitTipWei) external onlyOwner { | ||
| _summitTipWei = summitTipWei; | ||
| function setSummitTip(uint256 summitTipWei_) external onlyOwner { | ||
| if (summitTipWei_ > MAX_SUMMIT_TIP) revert SummitTipTooHigh(); | ||
| summitTipWei = summitTipWei_; | ||
| // TODO: emit event | ||
| } |
There was a problem hiding this comment.
The setSummitTip function now checks if the provided summitTipWei_ exceeds the MAX_SUMMIT_TIP constant, and if so, reverts with a SummitTipTooHigh error. This is a good practice for enforcing upper limits. However, an event emission is missing after setting the summitTipWei, which could be useful for tracking changes.
function setSummitTip(uint256 summitTipWei_) external onlyOwner {
if (summitTipWei_ > MAX_SUMMIT_TIP) revert SummitTipTooHigh();
summitTipWei = summitTipWei_;
+ emit SummitTipChanged(summitTipWei_);
}There was a problem hiding this comment.
This event will be added in a separate PR
There was a problem hiding this comment.
@ChiTimesChi: Understood. Please ensure to include the event emission in the upcoming PR as it is crucial for tracking changes to summitTipWei. This will enhance the transparency and traceability of the contract's operations.
| function setSummitTip(uint256 summitTipWei_) external onlyOwner { | ||
| if (summitTipWei_ > MAX_SUMMIT_TIP) revert SummitTipTooHigh(); | ||
| summitTipWei = summitTipWei_; | ||
| // TODO: emit event |
There was a problem hiding this comment.
Do we want to add an event now?
There was a problem hiding this comment.
@aureliusbtc I'm going to add the missing events in the PR that fixes ToB-20 later on
Description
0.01 ether.GasOracleSummary by CodeRabbit
Release Notes for Pull Request
_summitTipWeivariable in the GasOracle contract has been made public and renamed tosummitTipWei.setSummitTipfunction when the provided value is above the maximum limit, at the limit, or when the caller is not the owner of the contract.