Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Transfer Processors #3476

Merged
merged 19 commits into from
Apr 24, 2022
Merged

Transfer Processors #3476

merged 19 commits into from
Apr 24, 2022

Conversation

Kukks
Copy link
Member

@Kukks Kukks commented Feb 18, 2022 •
edited

This PR introduces a few things:

  • Payouts can now be directly nested under a store instead of through a pull payment.
  • The Wallet Send screen now has an option to "schedule" instead of simply creating a transaction. When you click on schedule, all transaction destinations are converted into approved payouts. Any options relating to fees or coin selection are discarded.
  • There is a new concept introduced, called "Transfer Processors". Transfer Processors are services for stores that process payouts that are awaiting payment. Each processor specifies which payment methods it can handle. BTCPay Server will have some forms of transfer processors baked in but it has been designed to allow the Plugin System to provide additional processors.
  • The initial transfer processors provided are "automated processors", for on chain and lightning payment methods. They can be configured to process payouts every X amount of minutes. For on-chain, this means payments are batched into one transaction, resulting in more efficient and cheaper fees for processing.

@Kukks Kukks force-pushed the scheduledtxs branch 2 times, most recently from 92c0cbb to 50770cc Compare February 23, 2022 13:18
@Kukks Kukks force-pushed the scheduledtxs branch 4 times, most recently from 2c25d56 to 0f45694 Compare March 11, 2022 10:21
@Kukks Kukks changed the title Scheduled Transfers + Automated batcher for LN + OnChain Transfer Processors Mar 11, 2022
@Kukks Kukks force-pushed the scheduledtxs branch 6 times, most recently from ff37910 to abd2026 Compare March 14, 2022 15:44
@pavlenex
Copy link
Contributor

Great work so far @Kukks 🚀

Would you need #1851 to proceed with this one or not? Do we want #1851 in at all?

@Kukks
Copy link
Member Author

Kukks commented Mar 14, 2022 •
edited

Ideally both #1851 and #3428 make it in alongside this

@Kukks Kukks marked this pull request as ready for review March 17, 2022 08:42
@Kukks Kukks force-pushed the scheduledtxs branch 7 times, most recently from 2599809 to 5cdda5e Compare March 21, 2022 11:34
@Kukks Kukks added this to the 1.5.0 milestone Mar 21, 2022
@pavlenex pavlenex mentioned this pull request Mar 29, 2022
Closed
@Kukks
Automated Transfer processors
8286318 
This PR introduces a few things:
* Payouts can now be directly nested under a store instead of through a pull payment.
* The Wallet Send screen now has an option to "schedule" instead of simply creating a transaction. When you click on schedule, all transaction destinations are converted into approved payouts. Any options relating to fees or coin selection are discarded.
* There is a new concept introduced, called "Transfer Processors".  Transfer Processors are services for stores that process payouts that are awaiting payment. Each processor specifies which payment methods it can handle.  BTCPay Server will have some forms of transfer processors baked in but it has been designed to allow the Plugin System to provide additional processors.
* The initial transfer processors provided are "automated processors", for on chain and lightning payment methods. They can be configured to process payouts every X amount of minutes. For  on-chain, this means payments are batched into one transaction, resulting in more efficient and cheaper fees for processing.
*
@Kukks
Merge remote-tracking branch 'origin/master' into scheduledtxs
1a405c8 
# Conflicts:
#	BTCPayServer.Data/ApplicationDbContext.cs
#	BTCPayServer.Data/Data/StoreData.cs
#	BTCPayServer.Data/Migrations/ApplicationDbContextModelSnapshot.cs
#	BTCPayServer/Hosting/MigrationStartupTask.cs
#	BTCPayServer/Services/MigrationSettings.cs
@Kukks
extract
648be01
@Kukks
remove magic string stuff
64f7f64
@NicolasDorier
Copy link
Member

@Kukks , I scheduled for later, then send the selected payout, then Schedule for later, here is the result:
image

@NicolasDorier
Copy link
Member

NicolasDorier commented Apr 20, 2022 •
edited

Actually the problem happen when we send two times to the same destination.

But this is also a problem somebody can Schedule for later a payout he is trying to pay.

@Kukks
Copy link
Member Author

Kukks commented Apr 20, 2022

@Kukks , I scheduled for later, then send the selected payout, then Schedule for later, here is the result: image

fixed, was meant to show like this:
image

@dennisreimann dennisreimann mentioned this pull request Apr 20, 2022
Closed
NicolasDorier
NicolasDorier reviewed Apr 21, 2022
View reviewed changes
BTCPayServer/Views/UIStorePullPayments/Payouts.cshtml Outdated
@@ -49,6 +52,13 @@

<partial name="_StatusMessage" />

@if (Context.GetStoreData()?.TransferProcessors?.Any(data => data.PaymentMethod.Equals(Model.PaymentMethodId, StringComparison.InvariantCultureIgnoreCase)) is not true && TransferProcessorFactories.Any(factory => factory.GetSupportedPaymentMethods().Contains(paymentMethodId)))
{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We shouldn't push users to use a transfer processor, this is still experimental stuff now.
On top of it, this warning can't be removed, it should be removable like what we did to announce the new label's status in invoices page.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only if Lord @pavlenex agrees as he really wanted more helper alerts around this feature.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fine with either removing a warning, in which case we'll probably loose on valuable feedback as the feature matures or by making it dismissible and make it go away once payment method is configured with processor.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The thing is that a warning is warranted when this feature will be used by all users, but that's not the case here.

The danger of this warning is that noobs don't know what it does and just blindly follow the warning just to make it go away. Ending up in a situation where money is being sent without them understanding why.

If you want to show the new feature, an info box as the one we did for Invoices List over the status's rename that can be dismissed forever is better than a warning which try to prompt user into action.

@Kukks
Copy link
Member Author

Kukks commented Apr 22, 2022

Renamed it to Payout processors (if you deployed this pr, you need to nuke db)

@NicolasDorier NicolasDorier merged commit 51690b4 into btcpayserver:master Apr 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@NicolasDorier NicolasDorier NicolasDorier left review comments

@pavlenex pavlenex pavlenex left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
1.5.0
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@Kukks @pavlenex @NicolasDorier