-
Couldn't load subscription status.
- Fork 4
fix(action): use fileSize to determine capacity when spendrate=0 #132
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
Co-authored-by: BigLep <85411+BigLep@users.noreply.github.com>
Co-authored-by: BigLep <85411+BigLep@users.noreply.github.com>
|
Issue explanation sounds right. |
|
@SgtPooki : I'll let you review/merge this one when it's ready to go |
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.
change is good enough.. needed to unblock some action users for their first usage.
| * @param {any} logger - Logger instance | ||
| * @returns {Promise<PaymentStatus>} Updated payment status | ||
| */ | ||
| export async function handlePayments(synapse, options, logger) { |
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.
@BigLep should we allow skipping payments adjustments entirely in the gh action, and allow some folks to handle that completely outside of the action?
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.
Sure. But maybe that can be done specifying minStorageDays as 0?
| const rateUsed = initialStatus.currentAllowances.rateUsed ?? 0n | ||
|
|
||
| // If there's an upcoming file upload and no existing usage, calculate deposit based on file size | ||
| if (rateUsed === 0n && carSizeBytes != null && carSizeBytes > 0) { | ||
| // Get pricing information to calculate file requirements | ||
| const storageInfo = await synapse.storage.getStorageInfo() | ||
| const pricePerTiBPerEpoch = storageInfo.pricing.noCDN.perTiBPerEpoch | ||
|
|
||
| // Calculate required deposit accounting for the new file | ||
| const { delta } = computeAdjustmentForExactDaysWithFile( | ||
| initialStatus, | ||
| minStorageDays, | ||
| carSizeBytes, | ||
| pricePerTiBPerEpoch | ||
| ) | ||
|
|
||
| requiredTopUp = delta > 0n ? delta : 0n | ||
| console.log( | ||
| `Required top-up for ${minStorageDays} days of storage (including upcoming upload): ${formatUSDFC(requiredTopUp)} USDFC` | ||
| ) | ||
| } else { | ||
| // Use existing logic for maintaining current usage | ||
| const { topUp } = computeTopUpForDuration(initialStatus, minStorageDays) | ||
| requiredTopUp = topUp | ||
| console.log(`Required top-up for ${minStorageDays} days of storage: ${formatUSDFC(requiredTopUp)} USDFC`) | ||
| } |
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 could fix this in filecoin-pin with a helper function that handles all the things under the hood.
cc @rvagg
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.
On the surface, this seems useful to have in filecoin-pin, I agree.
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.
the lens should be: minimal bespoke code needed in the action that's not related to dealing with being an action; if it's doing Synapse or Pin things, let's push them down
| initialStatus, | ||
| minStorageDays, | ||
| carSizeBytes, | ||
| pricePerTiBPerEpoch |
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 should probably be getting pricePerTiBPerEpoch internally via a util function instead of always passing it around. This will help when we eventually migrate to support withCDN payments
|
@SgtPooki : thanks. I left a couple of comments. Feel free to create issues if they are things we won't just knock out quick. |
Problem
The GitHub action bot was incorrectly calculating the required payment balance as
0.0000 USDFCwhen there was no existing storage (rateUsed = 0) but a file upload was pending. This caused uploads to fail with the error:The issue manifested as:
Followed immediately by an upload failure due to insufficient funds.
Root Cause
The
handlePaymentsfunction was usingcomputeTopUpForDurationwhich only considers existing storage usage (rateUsed). WhenrateUsed = 0(no active storage), it would returntopUp: 0neven though a file requiring lockup funds was about to be uploaded. The function had no visibility into the upcoming upload's requirements.Solution
This PR modifies the payment calculation to be file-aware when there's no existing storage:
handlePaymentsrateUsed = 0and a file size is provided, the function usescomputeAdjustmentForExactDaysWithFileto calculate deposit requirements that account for:computeTopUpForDurationlogic is preserved for cases with active storageCode Changes
The fix leverages the existing
computeAdjustmentForExactDaysWithFilefunction from the payments module, which was designed for exactly this scenario but wasn't being used in the GitHub action workflow.Testing
Behavior After Fix
The required balance is now correctly calculated based on the file size and target storage duration, ensuring sufficient funds are deposited before the upload begins.
Fixes #[issue-number]
Original prompt
Fixes #129
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.