-
Notifications
You must be signed in to change notification settings - Fork 23
refactor(sdk): ServiceInfo rename, accurate pricing separation, reusable primitives #375
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -54,6 +54,8 @@ import { | |
| getCurrentEpoch, | ||
| METADATA_KEYS, | ||
| SIZE_CONSTANTS, | ||
| TIME_CONSTANTS, | ||
| TOKENS, | ||
| timeUntilEpoch, | ||
| } from '../utils/index.ts' | ||
| import { combineMetadata, metadataMatches, objectToEntries, validatePieceMetadata } from '../utils/metadata.ts' | ||
|
|
@@ -785,34 +787,70 @@ export class StorageContext { | |
|
|
||
| /** | ||
| * Static method to perform preflight checks for an upload | ||
| * @param size - The size of data to upload in bytes | ||
| * @param withCDN - Whether CDN is enabled | ||
| * Composes cost calculation from WarmStorageService and readiness checks from PaymentsService | ||
| * @param warmStorageService - WarmStorageService instance | ||
| * @param paymentsService - PaymentsService instance | ||
| * @param size - The size of data to upload in bytes | ||
| * @returns Preflight check results without provider/dataSet specifics | ||
| */ | ||
| static async performPreflightCheck( | ||
| warmStorageService: WarmStorageService, | ||
| paymentsService: PaymentsService, | ||
| size: number, | ||
| withCDN: boolean | ||
| size: number | ||
| ): Promise<PreflightInfo> { | ||
| // Validate size before proceeding | ||
| StorageContext.validateRawSize(size, 'preflightUpload') | ||
|
|
||
| // Check allowances and get costs in a single call | ||
| const allowanceCheck = await warmStorageService.checkAllowanceForStorage(size, withCDN, paymentsService) | ||
| // Calculate upload cost | ||
| const cost = await warmStorageService.calculateUploadCost(size) | ||
|
|
||
| // Return preflight info | ||
| return { | ||
| estimatedCost: { | ||
| perEpoch: allowanceCheck.costs.perEpoch, | ||
| perDay: allowanceCheck.costs.perDay, | ||
| perMonth: allowanceCheck.costs.perMonth, | ||
| // Calculate rate per epoch from floor-adjusted monthly price | ||
| const pricing = await warmStorageService.getServicePrice() | ||
| const ratePerEpoch = cost.withFloorPerMonth / pricing.epochsPerMonth | ||
|
|
||
| // Calculate lockup requirements | ||
| const lockupEpochs = BigInt(TIME_CONSTANTS.DEFAULT_LOCKUP_DAYS * TIME_CONSTANTS.EPOCHS_PER_DAY) | ||
| const lockupNeeded = ratePerEpoch * lockupEpochs | ||
|
Comment on lines
+808
to
+813
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. really dont like this per month thing its inaccurate and forces us to do this back and forth stuff epoch<>month exchange. also we just called getServicePrice in calculateUploadCost and we call it again just to get
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. point taken on the double call to
but the problem is twofold:
the logic here is intentionally keeping things at per-month for as long as possible to avoid the early precision loss of going per-epoch and then back again.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ok makes sense the per month thing, but isnt the precision off already like, which month? the amount of 30s in a month varies a lot in different months ? |
||
|
|
||
| // Check payment readiness | ||
| const readiness = await paymentsService.checkServiceReadiness( | ||
| warmStorageService.getContractAddress(), | ||
| { | ||
| rateNeeded: ratePerEpoch, | ||
| lockupNeeded, | ||
| lockupPeriodNeeded: lockupEpochs, | ||
| }, | ||
| TOKENS.USDFC | ||
| ) | ||
|
|
||
| // Build error message if not sufficient | ||
| let message: string | undefined | ||
| if (!readiness.sufficient) { | ||
| const issues: string[] = [] | ||
| if (!readiness.checks.hasSufficientFunds && readiness.gaps?.fundsNeeded) { | ||
| issues.push(`Insufficient funds: need ${readiness.gaps.fundsNeeded} more`) | ||
| } | ||
| if (!readiness.checks.isOperatorApproved) { | ||
| issues.push('Operator not approved for service') | ||
| } | ||
| if (!readiness.checks.hasRateAllowance && readiness.gaps?.rateAllowanceNeeded) { | ||
| issues.push(`Insufficient rate allowance: need ${readiness.gaps.rateAllowanceNeeded} more`) | ||
| } | ||
| if (!readiness.checks.hasLockupAllowance && readiness.gaps?.lockupAllowanceNeeded) { | ||
| issues.push(`Insufficient lockup allowance: need ${readiness.gaps.lockupAllowanceNeeded} more`) | ||
| } | ||
| if (!readiness.checks.hasValidLockupPeriod && readiness.gaps?.lockupPeriodNeeded) { | ||
| issues.push(`Lockup period too short: need ${readiness.gaps.lockupPeriodNeeded} more epochs`) | ||
| } | ||
| message = issues.join('; ') | ||
| } | ||
|
Comment on lines
+826
to
+846
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. imo preflight and readiness can be merged into one function and throw and good error with most of this stuff in it. |
||
|
|
||
| return { | ||
| estimatedCostPerMonth: cost.withFloorPerMonth, | ||
| allowanceCheck: { | ||
| sufficient: allowanceCheck.sufficient, | ||
| message: allowanceCheck.message, | ||
| sufficient: readiness.sufficient, | ||
| message, | ||
| checks: readiness.checks, | ||
| }, | ||
| selectedProvider: null, | ||
| selectedDataSetId: null, | ||
|
|
@@ -829,8 +867,7 @@ export class StorageContext { | |
| const preflightResult = await StorageContext.performPreflightCheck( | ||
| this._warmStorageService, | ||
| this._synapse.payments, | ||
| size, | ||
| this._withCDN | ||
| size | ||
| ) | ||
|
|
||
| // Return preflight info with provider and dataSet specifics | ||
|
|
||
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 need this as an option ?
isnt it a constant
BigInt(TIME_CONSTANTS.DEFAULT_LOCKUP_DAYS * TIME_CONSTANTS.EPOCHS_PER_DAY)?