Skip to content

Conversation

@lordshashank
Copy link
Contributor

@lordshashank lordshashank commented Oct 14, 2025

This pull request adds support for depositing funds to a different recipient address using the PaymentsService.deposit() method, and updates related documentation and type definitions to reflect this new flexibility. The change replaces the previous DepositCallbacks parameter with a more general DepositOptions object, which now includes an optional to property for specifying the recipient address, as well as callback functions for transaction visibility.

SDK API enhancements:

  • The PaymentsService.deposit() method now accepts an optional DepositOptions object, which includes an optional to property to specify a recipient address other than the signer. This replaces the previous DepositCallbacks parameter. [1] [2] [3] [4]
  • The deposit transaction now uses the address provided in options.to (if present) as the recipient; otherwise, it defaults to the signer's address. [1] [2]
  • All deposit operation callbacks are now accessed through the DepositOptions object.

Documentation updates:

  • The documentation and code examples in AGENTS.md, getting-started.mdx, and components.mdx have been updated to describe and demonstrate the new to option for deposits, including sample usage for depositing to another address. [1] [2] [3] [4] [5]

@github-project-automation github-project-automation bot moved this to 📌 Triage in FS Oct 14, 2025
@rvagg
Copy link
Collaborator

rvagg commented Oct 15, 2025

pretty ugly, I don't like this being right at the end after the callbacks.

I think @hugomrdias wants to turn all of the arguments to these calls into {} eventually, which would be much easier to deal with. But for now, how about you rename DepositCallbacks to DepositOptions and put a to property on that. Then you're not doing an undefined but a { to: wallet }

@rjan90 rjan90 moved this from 📌 Triage to 🔎 Awaiting review in FS Oct 15, 2025
@lordshashank
Copy link
Contributor Author

lordshashank commented Oct 15, 2025

ok so sorry @rvagg , this was rushed, there were few folks in cohort who wanted this and I did it fast in my branch and then thought why not make a PR as well.

I thought putting it at end is fine as it will not break existing implementations and if people want can use the to parameter. but yeah, needing to send undefined to use that is bad.

DId the change you requested but I also don't like this much as we are including parameter with callbacks which imo are better separate, also we introduce a breaking change with name change of DepositCallback, in case anyone would have imported that.

@hugomrdias idea is best imo, if he is implementing it then maybe we can close this as there were only few folks who were asking, and they can use my branch until then.

@rvagg
Copy link
Collaborator

rvagg commented Oct 16, 2025

you can type alias and @deprecate if you're worried about breakage there

another alternative is to make DepositOptions take your to but also a callbacks: DepositCallbacks, and then in that last position we take DepositOptions | DepositCallbacks and check for the callbacks property to figure out what we got - do that as an interim measure, document clearly that it's going to change, remove docs for the old way and we can ship that without breakage

We can also break things, that's ok, just semver-minor. People will pick up the type changes when the break downstream, that's one nice thing about TypeScript.

@wjmelements wjmelements changed the title Expand deposit to any address feat: deposit to any address Oct 18, 2025
@wjmelements wjmelements merged commit 4629f1f into FilOzone:master Oct 18, 2025
9 checks passed
@github-project-automation github-project-automation bot moved this from 🔎 Awaiting review to 🎉 Done in FS Oct 18, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: 🎉 Done

Development

Successfully merging this pull request may close these issues.

3 participants