-
Notifications
You must be signed in to change notification settings - Fork 17
Modify CDN payment rails and add methods for usage-based payments #237
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
- Remove validator from cdn payment rails - Add fixed lockup during cdn payment rail creation - Add methods for FilBeam operator contract to settle cdn payment rails - Add method for increasing fixed lockup on cdn payment rails - Cleanup variables, errors and events related to cdn service termination - Remove handling of cdn payment rail termination from `railTerminated` method
…t/usage-based-payments
…t/usage-based-payments
- Replace percentage-based lockup ratios with absolute lockup values - Remove cdnEndEpoch field from DataSetInfo as CDN rails don't track termination - Update metadata keys from ratio to value naming convention - Modify lockup calculation to use provided values directly instead of percentage calculations - Update test cases to use absolute values instead of percentages - Add validation for empty lockup value strings - Support asymmetric and zero lockup values for flexible CDN configuration
… settleCDNPaymentRails method - Combine two separate settlement methods into one with signature: settleCDNPaymentRails(uint256 dataSetId, uint256 cdnAmount, uint256 cacheMissAmount) - Consolidates validation logic and reduces code duplication - Maintains all access control and validation requirements - Add comprehensive test coverage with 13 test cases covering success scenarios, access control, validation errors, event emission, and payment processing
…t/usage-based-payments
- Remove specialized _getCDNMetadataKV helper function
- Replace all calls with the more general _getSingleMetadataKV("withCDN", value)
- Simplifies codebase by using existing generic metadata helper
…stCreateDataSetCreatesRail
- Remove redundant tests - Add CDNPaymentRailsToppedUp event and modify top-up tests
Co-authored-by: Julian Gruber <[email protected]>
Co-authored-by: Julian Gruber <[email protected]>
…t/usage-based-payments
| * @param cdnAmount Amount to settle for CDN rail | ||
| * @param cacheMissAmount Amount to settle for cache miss rail | ||
| */ | ||
| function settleCDNPaymentRails(uint256 dataSetId, uint256 cdnAmount, uint256 cacheMissAmount) |
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 are discussing this API in the PR adding code that calls this method, let's resolve that thread first:
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.
Other than the discussion around settleCDNPaymentRails API, I think the pull request looks great now!
|
|
||
| event ViewContractSet(address indexed viewContract); | ||
|
|
||
| event CDNPaymentRailsToppedUp(uint256 indexed dataSetId, uint256 totalCdnLockup, uint256 totalCacheMissLockup); |
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.
Do we really need an event for this?
| createData.payer, // from (payer) | ||
| filBeamBeneficiaryAddress, // to FilBeam beneficiary | ||
| address(this), // this contract acts as the arbiter | ||
| address(0), // no validator |
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.
are you sure? We use this validator to adjust payments according to service uptime. For example for PDP if you aren't proving possession you aren't getting paid. Maybe CDN should have its own validator?
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.
Is IValidator(filBeamController) not possible because the controller doesn't have the information yet?
| usdfcTokenAddress, // token address | ||
| createData.payer, // from (payer) | ||
| payee, // payee address from registry | ||
| address(this), // this contract acts as the arbiter |
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.
since this contract is not the rail validator any more, don't need to set railToDataSet for the rail
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 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 @rvagg CC @Chaitu-Tatipamula Closes #277 Resolves #269 (review) We are able to remove this mapping because it is only used by IValidator functions, and, since #237, FWSS is not the IValidator for these rails. This reduces codesize 23,991 -> 23,898 (-93) #### Changes * remove railToDataSet mapping for both cdn rails
## Description This pull request modifies the existing `CDNPaymentRailsToppedUp `event by adding two new fields: `cdnAmountAdded `and `cacheMissAmountAdded`. These additions are designed to simplify the tracking of remaining user egress in the off-chain component. ## Motivation Following discussions with @bajtos, we identified the need to modify the `CDNPaymentRailsToppedUp` event signature to streamline our egress accounting process. Our initial approach involved extending the `FilBeamOperator` contract with a `topUpCDNPaymentRails` method and adding authorization to the existing `FilecoinWarmStorageService.topUpCDNPaymentRails` method. However, I opted against this approach since these modifications can be implemented through a contract upgrade instead. ## Changelog - Add `cdnAmountAdded` and `cacheMissAmountAdded` fields to `CDNPaymentRailsToppedUp` event - Refactor tests to match updated event signature ## Related - filbeam/worker#333 - #237
| } | ||
|
|
||
| if (cacheMissAmount > 0) { | ||
| payments.modifyRailPayment(info.cacheMissRailId, 0, cacheMissAmount); |
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.
Setting these rates to zero here is negated by updatePaymentRates. Should we also set the payment rate to zero in there?
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.
@pyropy PTAL
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 was already a fix PR before I noticed this.
Motivation
To support FilBeam's usage-based payment model, this PR adds methods to the FWSS contract for settling CDN payment rails without payment validators
Description
Introduces two new methods:
settleCDNPaymentRails- for settling CDN payment railstopUpCDNPaymentRails- for topping up balance in CDN payment rails (increasing fixed lockup)Adding these two methods would enable usage-based payments for CDN-related payment rails as FilBeam contract would be able to calculate settlement amounts based on the reported egress usage and settle payment rails via
settleCDNPaymentRails.Validator has been removed from CDN-related payment rails as settling is done by FilBeam contract.
Users are be able to increase fixed lockup for their CDN-related payment rails by calling
topUpCDNPaymentRailsin return increasing their data set egress limit. Initial fixed lockup for CDN payment rails is set to0as we're unable to pass lockup amounts via metadata without increasing contract byte size due to addition of helper new functions.Changelog
topUpCDNPaymentRailsrailTerminatedmethodRelated to: