feat(pricing): make pricing constants mutable with owner-controlled u…#325
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); |
wjmelements
left a comment
There was a problem hiding this comment.
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 |
| /// @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.
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.
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.
Alternatively you can use two different errors.
wjmelements
left a comment
There was a problem hiding this comment.
- Prefer
require()toif (){ revert() } - Avoid string constants.
Co-authored-by: William Morriss <wjmelements@gmail.com>
Co-authored-by: William Morriss <wjmelements@gmail.com>
Co-authored-by: William Morriss <wjmelements@gmail.com>
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.
rm or move to WarmStorageView lib
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.
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())