-
Notifications
You must be signed in to change notification settings - Fork 17
feat(pricing): make pricing constants mutable with owner-controlled u… #325
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
I had to make the pricing variables use camelCase ( Because without it the generated files check was failing. |
| view | ||
| returns (uint256 storagePrice, uint256 cacheMissPrice, uint256 cdnPrice) | ||
| { | ||
| return (storagePricePerTibPerMonth, cacheMissPricePerTibPerMonth, cdnPricePerTibPerMonth); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think these cdn rates should be zero after #237. Might be an oversight in that PR. I have raised this question on slack.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks like they noticed before I did #324
wjmelements
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should have a reasonable upper bound (eg 4x the initial value) on these numbers enforced by the contract. Increasing the rates above that boundary should require a contract upgrade via announcePlannedUpgrade.
I think that makes sense. Will implement |
|
Will have to be reconciled with #320 depending on which is merged first. We'll want to be able to add |
|
Happy to let #320 to get merged first, and then followup here. |
1a61736 to
ba45531
Compare
…pdates - Convert STORAGE_PRICE_PER_TIB_PER_MONTH, CACHE_MISS_PRICE_PER_TIB_PER_MONTH, and CDN_PRICE_PER_TIB_PER_MONTH from immutable to storage variables - Add updatePricing() owner-only function to update pricing rates - Add getCurrentPricingRates() getter function - Add PricingUpdated event to track pricing changes
fix: make update-abi
chore: make abi-gen
Rebased on main to incorporate floor pricing changes (commit 6cb08ce) and made MINIMUM_STORAGE_RATE_PER_MONTH mutable to enable runtime updates. Changes: - Converted minimumStorageRatePerMonth from immutable to storage variable - Added MAX_MINIMUM_STORAGE_RATE_PER_MONTH (0.24 USDFC, 4x initial 0.06) - Updated updatePricing() to accept newMinimumRate parameter - Updated PricingUpdated event to include minimumRate - Updated getCurrentPricingRates() to return both storage price and minimum rate - Added AtLeastOnePriceMustBeNonZero and PriceExceedsMaximum errors - Updated all references throughout contract to use camelCase variable names
ba45531 to
79ada50
Compare
|
I have reconciled this PR with the changes coming from #320 now, and added |
service_contracts/src/Errors.sol
Outdated
| /// @param priceType The type of price being updated (e.g., "storage", "minimumRate") | ||
| /// @param maxAllowed The maximum allowed value for this price type | ||
| /// @param actual The attempted value that exceeds the maximum | ||
| error PriceExceedsMaximum(string priceType, uint256 maxAllowed, uint256 actual); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we want to avoid strings in the code because solidity strings are a lot of codesize. They are actually the reason we are using these ABI errors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a comparable situation where we are working around this with enum AddressField and another with enum CommissionType.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alternatively you can use two different errors.
wjmelements
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Prefer
require()toif (){ revert() } - Avoid string constants.
Co-authored-by: William Morriss <[email protected]>
Co-authored-by: William Morriss <[email protected]>
Co-authored-by: William Morriss <[email protected]>
Address PR feedback to reduce code size by using enum instead of string literals in error definitions. Changes: - Add PriceType enum (Storage, MinimumRate) to Errors.sol - Update PriceExceedsMaximum error to use PriceType enum instead of string - Update error calls in updatePricing() to use Errors.PriceType.Storage/MinimumRate - Fix formatting issues in updatePricing() function
| * @return minimumRate Current minimum monthly storage rate | ||
| */ | ||
| function getCurrentPricingRates() external view returns (uint256 storagePrice, uint256 minimumRate) { | ||
| return (storagePricePerTibPerMonth, minimumStorageRatePerMonth); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rm or move to WarmStorageView lib
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewer @rjan90 We want view methods to use `extsload` because this reduces codesize. Codesize is a minor component of gas costs in the Filecoin EVM. #### Changes * mv getCurrentPricingRates to the View contract
| if (newStoragePrice > 0) { | ||
| require( | ||
| newStoragePrice <= MAX_STORAGE_PRICE_PER_TIB_PER_MONTH, | ||
| Errors.PriceExceedsMaximum( | ||
| Errors.PriceType.Storage, MAX_STORAGE_PRICE_PER_TIB_PER_MONTH, newStoragePrice | ||
| ) | ||
| ); | ||
| storagePricePerTibPerMonth = newStoragePrice; | ||
| } | ||
| if (newMinimumRate > 0) { | ||
| require( | ||
| newMinimumRate <= MAX_MINIMUM_STORAGE_RATE_PER_MONTH, | ||
| Errors.PriceExceedsMaximum( | ||
| Errors.PriceType.MinimumRate, MAX_MINIMUM_STORAGE_RATE_PER_MONTH, newMinimumRate | ||
| ) | ||
| ); | ||
| minimumStorageRatePerMonth = newMinimumRate; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't love having ifs here and am not sure "Pass 0 to keep existing value unchanged." is all that helpful. It might help with footguns, perhaps, but now there's no way to set a floor of 0, we'd have to use 1 to be our floor if we decided to undo that.
But, not a big deal if we're just trying to get this over the line at this point.
|
Will go ahead and merge as-is, since we are trying to get this over the line at this point, to unblock the release of FWSS contracts for GA |
Makes the three pricing rate constants mutable storage variables instead of immutable, enabling future price adjustments without requiring contract upgrades.
Changes
STORAGE_PRICE_PER_TIB_PER_MONTH,CACHE_MISS_PRICE_PER_TIB_PER_MONTH, andCDN_PRICE_PER_TIB_PER_MONTHfrom immutable constants to mutable storage variablesupdatePricing()- owner-only function that allows selective updates to pricing rates (pass 0 to keep existing value)getCurrentPricingRates()- public view function to query current pricing valuesPricingUpdatedevent to track pricing changesDEFAULT_CDN_LOCKUP_AMOUNTandDEFAULT_CACHE_MISS_LOCKUP_AMOUNTremain immutable as they are not expected to changeImpact
nextProvingPeriod())