Split maxSettlementFee into maxSyncFee and maxAsyncFee
#508
+58
−133
Conversation
This file contains 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
EdNoepel
reviewed
Dec 9, 2024
kbrizzle
reviewed
Dec 12, 2024
| /// @param maxSyncFee The max sync fee | ||
| /// @param maxAsyncFee The max async fee | ||
| /// @param callbacks The number of settlement callbacks to be made | ||
| function applyFeeMaximum(PriceResponse memory self, UFixed6 maxSyncFee, UFixed6 maxAsyncFee, uint256 callbacks) internal pure { |
There was a problem hiding this comment.
instead of
if (self.syncFee.gt(maxSyncFee))
self.syncFee = maxSyncFee;
it's a bit cleaner to write these caps as:
self.syncFee = self.syncFee.min(maxSyncFee);
| function applyFeeMaximum(PriceResponse memory self, UFixed6 maxSyncFee, UFixed6 maxAsyncFee, uint256 callbacks) internal pure { | ||
| if (self.syncFee.gt(maxSyncFee)) | ||
| self.syncFee = maxSyncFee; | ||
| UFixed6 asyncFee = self.asyncFee.mul(UFixed6Lib.from(callbacks)); |
There was a problem hiding this comment.
The async fee maximum will cap the individual async fees charges (so that if we have tons of orders in a single version we're not capping the total amount charged).
So no need to multiply this by the callback amount, that won't be needed anymore.
kbrizzle
reviewed
Dec 12, 2024
| @@ -207,8 +207,8 @@ contract KeeperOracle is IKeeperOracle, Instance { | |||
| priceResponse.asyncFee = UFixed6Lib.from(factory.settlementGasOracle().cost(0), true); | |||
| priceResponse.oracleFee = keeperOracleParameter.oracleFee; | |||
There was a problem hiding this comment.
actually now that applyFeeMaximum is so simple, maybe we can just apply the .min(oracleParameter....) directly on these above and delete the helper function altogether.
There was a problem hiding this comment.
Removed helper method in aac4ffe
[Periphery] Unit Test Coverage ReportCoverage after merging prateek/pe-1982-split-maxSettlementFee into v2.4 will be
Coverage Report
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Code Size Diff:View Report |
Gas Report Diff:View Report |
[Core] Integration Test Coverage ReportCoverage after merging prateek/pe-1982-split-maxSettlementFee into v2.4 will be
Coverage Report
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
[Core] Unit Test Coverage ReportCoverage after merging prateek/pe-1982-split-maxSettlementFee into v2.4 will be
Coverage Report
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
[Core] Combined Test Coverage ReportCoverage after merging prateek/pe-1982-split-maxSettlementFee into v2.4 will be
Coverage Report
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
[Periphery] Integration Test Coverage ReportCoverage after merging prateek/pe-1982-split-maxSettlementFee into v2.4 will be
Coverage Report
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
[Periphery] Combined Test Coverage ReportCoverage after merging prateek/pe-1982-split-maxSettlementFee into v2.4 will be
Coverage Report
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
kbrizzle
approved these changes
Dec 16, 2024
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.
Issue: https://linear.app/perennial/issue/PE-1982/[feature]-split-maxsettlementfee
Migration Note: