refactor(sdk): ServiceInfo rename, accurate pricing separation, reusable primitives #375
Conversation
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
synapse-dev | a94dde6 | Commit Preview URL Branch Preview URL |
Nov 04 2025, 09:45 AM |
| if (readiness.gaps?.fundsNeeded) { | ||
| const fundsNeeded = readiness.gaps.fundsNeeded | ||
| actions.push({ | ||
| type: 'deposit', | ||
| description: `Deposit ${depositAmount} USDFC to payments contract`, | ||
| execute: async () => await paymentsService.deposit(depositAmount, TOKENS.USDFC), | ||
| description: `Deposit ${fundsNeeded} USDFC to payments contract`, | ||
| execute: async () => await paymentsService.deposit(fundsNeeded, TOKENS.USDFC), | ||
| }) | ||
| } | ||
|
|
||
| // Check if service approval is needed | ||
| if (!allowanceCheck.sufficient) { | ||
| if ( | ||
| !readiness.checks.isOperatorApproved || | ||
| readiness.gaps?.rateAllowanceNeeded || | ||
| readiness.gaps?.lockupAllowanceNeeded | ||
| ) { | ||
| const rateAllowanceNeeded = readiness.gaps?.rateAllowanceNeeded ?? 0n | ||
| const lockupAllowanceNeeded = readiness.gaps?.lockupAllowanceNeeded ?? 0n | ||
| actions.push({ | ||
| type: 'approveService', | ||
| description: `Approve service with rate allowance ${allowanceCheck.rateAllowanceNeeded} and lockup allowance ${allowanceCheck.lockupAllowanceNeeded}`, | ||
| description: `Approve service with rate allowance ${rateAllowanceNeeded} and lockup allowance ${lockupAllowanceNeeded}`, | ||
| execute: async () => | ||
| await paymentsService.approveService( | ||
| this._warmStorageAddress, | ||
| allowanceCheck.rateAllowanceNeeded, | ||
| allowanceCheck.lockupAllowanceNeeded, | ||
| TIME_CONSTANTS.EPOCHS_PER_MONTH, // 30 days max lockup period | ||
| rateAllowanceNeeded, | ||
| lockupAllowanceNeeded, | ||
| lockupEpochs, | ||
| TOKENS.USDFC | ||
| ), | ||
| }) | ||
| } |
There was a problem hiding this comment.
Actions consolidation using Permit: This will reduce actions to only one.
if (fundsNeeded && approvalNeeded) {
actions.push({
type: 'DepositAndApprove',
description: `Deposit ${fundsNeeded} USDFC and approve service in one transaction`,
execute: async () => await paymentsService.depositWithPermitAndApproveOperator(
fundsNeeded,
this._warmStorageAddress,
rateAllowanceNeeded,
lockupAllowanceNeeded,
lockupEpochs,
TOKENS.USDFC
),
})
}
if (approvalNeeded && !fundsNeeded) {
actions.push({
type: 'approveService',
description: `Approve service with rate allowance ${rateAllowanceNeeded} and lockup allowance ${lockupAllowanceNeeded}`,
execute: async () =>
await paymentsService.approveService(
this._warmStorageAddress,
rateAllowanceNeeded,
lockupAllowanceNeeded,
lockupEpochs,
TOKENS.USDFC
),
})
}
if (fundsNeeded && !approvalNeeded) {
actions.push({
type: 'deposit',
description: `Deposit ${fundsNeeded} USDFC to payments contract`,
execute: async () => await paymentsService.deposit(fundsNeeded, TOKENS.USDFC),
})
}
| * } | ||
| * ``` | ||
| */ | ||
| async prepareStorageUpload( |
There was a problem hiding this comment.
Handle prepayment for cdnDataSet creation
*improvement
async prepareStorageUpload(
dataSize: number,
paymentsService: PaymentsService,
// If true, adds 1 USDFC to lockup for CDN dataset creation (users must prepay egress credits)
creationOfCDNDataset
): Promise<{
estimatedCostPerMonth: bigint
allowanceCheck: {
sufficient: boolean
message?: string
}
actions: Array<{
type: 'deposit' | 'approve' | 'approveService'
description: string
execute: () => Promise<ethers.TransactionResponse>
}>
}> CDN Dataset Creation Logic
// Add 1 USDFC lockup for CDN dataset creation
// CDN datasets require users to prepay 1 USDFC for egress credits balance upfront
let additionalLockupNeeded = 0n
if (creationOfCDNDataset) {
additionalLockupNeeded = 10n ** 18n // 1 USDFC in wei (18 decimals)
}Lockup Calculation Update
// Calculate total lockup: (rate per epoch * lockup epochs) + CDN dataset fee (if applicable)
const lockupNeeded = ratePerEpoch * lockupEpochs + additionalLockupNeededHandles prepayment for egress credits on cdnDataSet creation
Problem
When creating a CDN-enabled dataset for the first time, users must prepay 1 USDFC for egress credits balance.
Without this flag, the upload fails with "Insufficient lockup" error.
Detection Pattern
// In user code (SDK usage):
const context = await synapse.storage.createContext({ withCDN: true })
// Detect first-time CDN dataset creation
const isCreatingNewCDNDataset = context.dataSetId === null && context.withCDN
// ^^^^^^^^^^^^^^^^^^^^^^ ^^^^^^^^^^^^^^
// No existing dataset CDN requested
// Pass flag to prevent lockup calculation errors
const { actions } = await warmStorageService.prepareStorageUpload(
dataSize,
paymentsService,
isCreatingNewCDNDataset // ← Adds 1 USDFC to lockup requirements
)
for (const action of actions) {
await action.execute();
}There was a problem hiding this comment.
Alternative idea create a similar method inside StorageContext
Additional Improvements
- Check if userWalletBalance >= depositNeeded and return sufficientBalance
- Bring back the lockupDays parameter that checkAllowanceForStorage had. This allows developers to pay for storage for more than 30 days, giving them this flexibility.
// Simple, clean API - context knows its own state
const context = await synapse.storage.createContext({ withCDN: true })
// Context automatically detects if it's creating a new CDN dataset
const preparation = await context.prepareStorageUpload(dataSize)
// Clear balance handling with descriptive response
if (!preparation.sufficientBalance) {
throw new Error("Insufficient USDFC balance. Please top up your wallet.")
}
// Execute required actions
for (const action of preparation.actions) {
await action.execute()
}
// Upload proceeds with confidence
await context.upload(data)
There was a problem hiding this comment.
i would rather deal with CDN in another PR, i still dont understand how that works and this 1 usdfc for cdn dataset seems like we going back..
There was a problem hiding this comment.
I didn't know about this CDN thing, it is kind of important. I've opened an issue for the contract here: FilOzone/filecoin-services#339 and I'll consider it here as well.
There was a problem hiding this comment.
i scheduled a sync with the cdn team for today to understand cdn payments better
There was a problem hiding this comment.
There are a few things here that will help reduce specifics in filecoin-pin:
-
Readiness checks —
checkServiceReadiness()can replace parts of our customvalidatePaymentCapacity()andcheckUploadReadiness()logic. We'll still need the runway-aware checks below. -
Cost calculation —
calculateUploadCost()with floor pricing will replace ourcalculateRequiredAllowances()calls, once we integrate floor pricing.
What is missing that filecoin-pin needs (runway management):
filecoin-pin adds runway/duration management not covered here:
- Runway calculations —
computeTopUpForDuration()computes how many days current funding covers based onrateUsed+lockupUsed+ balance. - Adjustment calculations —
computeAdjustmentForExactDays()/computeAdjustmentForExactDaysWithPiece()calculate exact deposits needed to maintain N days runway (including after adding new content). - Top-up orchestration —
calculateRequiredTopUp()combines piece upload needs + runway requirements with reason codes.
| rateUsed: bigint | ||
| lockupAllowance: bigint | ||
| lockupUsed: bigint | ||
| maxLockupPeriod: bigint |
There was a problem hiding this comment.
Can we explain these a little more here? What exactly is maxLockupPeriod? the max period I will allow a service to lockup funds for? or the maximum lockup period that a service provider requires?
| requirements: { | ||
| rateNeeded: bigint | ||
| lockupNeeded: bigint | ||
| lockupPeriodNeeded: bigint |
There was a problem hiding this comment.
do we need this as an option ?
isnt it a constant BigInt(TIME_CONSTANTS.DEFAULT_LOCKUP_DAYS * TIME_CONSTANTS.EPOCHS_PER_DAY) ?
| 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 |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
point taken on the double call to getServicePrice, could just do that once.
really dont like this per month thing
but the problem is twofold:
- the contract defines pricing per month
- downscaling to per-epoch loses a lot of precision (a surprising amount since we're dealing with fairly low numbers in the first place)
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.
There was a problem hiding this comment.
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 ?
| // 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('; ') | ||
| } |
There was a problem hiding this comment.
imo preflight and readiness can be merged into one function and throw and good error with most of this stuff in it.
| * } | ||
| * ``` | ||
| */ | ||
| async prepareStorageUpload( |
There was a problem hiding this comment.
i would rather deal with CDN in another PR, i still dont understand how that works and this 1 usdfc for cdn dataset seems like we going back..
| const cost = await this.calculateUploadCost(dataSize) | ||
|
|
||
| // Calculate requirements | ||
| const pricing = await this.getServicePrice() | ||
| const ratePerEpoch = cost.withFloorPerMonth / pricing.epochsPerMonth |
There was a problem hiding this comment.
again calculateUploadCost and getServicePrice just for pricing.epochsPerMonth
- Rename getStorageInfo() → getServiceInfo() - Restructure pricing fields for clarity - Simplify allowance checks Fixes: #360
- Add PaymentsService.checkServiceReadiness() for any service - Rename calculateStorageCost() to calculateUploadCost(), expose floor pricing - Simplify prepareStorageUpload() signature (dataSize only, monthly cost only) - Remove checkAllowanceForStorage() - Add getContractAddress(), export ServiceReadinessCheck type
Yeah, I agree .. the more I look at this the more I hate it from a consumption perspective. A lot of this isn't as helpful as it should be. From the experience with Filecoin Pin (and what we're trying to do with filecoin-project/filecoin-pin#76, and also see filecoin-project/filecoin-pin#202 (comment)) is solve for: do one transaction (if required) that sets me up to use the system. Taken further, we when we add storage to that, we should also be able to make it: do one transaction (if required) that sets me up to perform this upload. The combination of How about we do this: strip out all of this complexity and replace it with a single method: Basically we want to take away the pain of figuring out all of this stuff for practical use: Set me up to store this thing, on top of what I'm already storing, and I want it to be paid for this far into the future. |
rvagg
force-pushed
the
rvagg/service-pricing
branch
from
7fc2358 to
a94dde6
Compare
|
storage.prepare sounds good! what about current costs and current prepared upload costs? |
you mean in terms of current storage costs and up-coming storage costs? I think those just become options. The "months" thing is just 30 days, we have a constant for it locally in constants.ts, and in the contract (as witnessed above) 2880*30. We just treat a storage "month" as 30 days. Anything fancier than that is on you. We can't align to calendar months like a web2 storage service would do for you. I guess we should make that crystal clear. |
Two commits in here. I was originally solving for #360 and then the deeper I got the worse I realised it was. We have a lot of CDN stuff still in there, floor pricing impacts a lot of things, there's mismatch in arguments and return types. So I've tried to be consistent; I've removed "perDay" and "perEpoch" from a bunch of places, leaving just the standardised "perMonth" we do everywhere else.
I'm hoping that this will be able to replace a bunch of things in filecoin-pin. @SgtPooki when you have a spare moment would you mind having a quick think about the coverage of functionality we have here? filecoin-pin is currently not dealing with the floor price, although it is updated to the 30 day lockup. I've also pulled in all of the detailed service readiness checks in here.
refactor(sdk): decompose preflight into reusable primitives
refactor(sdk): rename StorageInfo → ServiceInfo, restructure pricing
Fixes: #360
So we can now get this kind of information from Synapse, this is from example-storage-info.js with my wallet that's not set up for current contracts: