feat: add filecoin pay funding planner #286
Conversation
FilOzzy
added
team/filecoin-pin
There was a problem hiding this comment.
Pull request overview
This PR introduces a comprehensive Filecoin Pay funding planner API that consolidates and improves upon the legacy top-up calculation logic. The new API provides a cleaner separation between planning (calculation) and execution phases, with detailed insights about current and projected funding states.
Key changes:
- Added
planFilecoinPayFunding,executeFilecoinPayFunding, andgetFilecoinPayFundingInsightsas the new core funding APIs - Removed legacy
calculateRequiredTopUpandformatTopUpReasonfunctions to eliminate duplication - Refactored CLI
payments fundand auto-fund commands to use the new planner - Added comprehensive unit tests covering wallet shortfall detection, minimum mode, and zero-spend scenarios
Reviewed changes
Copilot reviewed 9 out of 10 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
src/core/payments/funding.ts |
New file introducing the funding planner with calculation, planning, and execution functions |
src/core/payments/types.ts |
Added new types (FundingMode, FundingReasonCode, FilecoinPayFundingPlan, etc.) and updated documentation for existing types |
src/core/payments/index.ts |
Exports new funding module (export * from './funding.js') |
src/core/payments/top-up.ts |
Removed calculateRequiredTopUp and formatTopUpReason, kept executeTopUp for backward compatibility |
src/core/payments/README.md |
Added documentation and usage examples for the new funding planner API |
src/payments/types.ts |
Updated to re-export FundingMode from core, replaced deprecated FundMode type |
src/payments/fund.ts |
Refactored to use planFilecoinPayFunding and executeFilecoinPayFunding, but contains a critical bug with parameter passing |
upload-action/src/filecoin.js |
Migrated to use calculateFilecoinPayFundingPlan and formatFundingReason |
upload-action/package-lock.json |
Version bump from 0.13.0 to 0.14.0 reflecting the minor version change |
src/test/unit/payments-funding.test.ts |
New comprehensive unit tests for the funding planner functions |
Files not reviewed (1)
- upload-action/package-lock.json: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const { plan } = await planFilecoinPayFunding({ | ||
| synapse, | ||
| targetRunwayDays: 30, | ||
| ensureAllowances: true, // also sets WarmStorage allowances |
There was a problem hiding this comment.
| ensureAllowances: true, // also sets WarmStorage allowances | |
| ensureAllowances: true, // also checks and sets WarmStorage allowances |
I feel like there should be a stronger recommendation to always use this, could even be an opt-out instead of opt-in, but I'll leave it to you
There was a problem hiding this comment.
Although .. depositUSDFC does it for you automatically, so maybe this is not even needed as an option here, we just do it by default anyway?
You might want to look through the flow in there and where it's called from your code in here to see if you're not doing too many calls. Probably not a big deal but also might be some flow optimisation that can be done.
| status: PaymentStatus, | ||
| overrides?: { depositedBalance?: bigint; rateUsed?: bigint; lockupUsed?: bigint } | ||
| ): FilecoinPayFundingInsights { | ||
| return buildFilecoinPayFundingInsights(status, overrides) |
There was a problem hiding this comment.
maybe just delete buildFilecoinPayFundingInsights and use getFilecoinPayFundingInsights internally
There was a problem hiding this comment.
obviously I mean inline buildFilecoinPayFundingInsights into getFilecoinPayFundingInsights
| case 'withdrawal-excess': | ||
| return 'Excess funds available for withdrawal' | ||
| default: | ||
| return 'Required funding' |
There was a problem hiding this comment.
not tempted to just throw in this case?
| if (days === 0) { | ||
| reasonCode = delta > 0n ? 'piece-upload' : 'none' |
There was a problem hiding this comment.
what is this case? you want zero days runway but still need funding to onboard the piece? is this a normal path we experience?
rvagg
left a comment
rvagg
left a comment
There was a problem hiding this comment.
I guess this is OK, I haven't reviewed it all in depth, if it works in practice I'll take your word for it!
github-project-automation
bot
moved this from 📌 Triage
to ✔️ Approved by reviewer
in FS
3 participants
Summary
Expose Filecoin Pay funding logic as a reusable core API, refactor CLI funding to use it, and add coverage/docs. Remove legacy top-up calc in favor of the new planner.
What Changed
planFilecoinPayFunding,executeFilecoinPayFunding, andgetFilecoinPayFundingInsightsinsrc/core/payments/funding.ts, exported viacore/payments.FilecoinPayFundingPlan/Execution/Insights, options) and refactoredpayments fund/ auto-fund to consume the new API.src/core/payments/README.mdwith usage examples.Removal
calculateRequiredTopUp/formatTopUpReasonpath to avoid duplication; callers should useplanFilecoinPayFunding+executeFilecoinPayFunding.