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.

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

Implement Manual Trigger Endpoint for Executing Splits on Specific Wallets #17

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

Opago-Pay
Copy link

This pull request introduces a new POST endpoint that allows for the manual triggering of split executions on specific wallets.

Automatic split execution occasionally fails to trigger under certain conditions. To ensure reliability and control in these scenarios, we have developed an endpoint that addresses the challenge.

Implementation Details:
Feature: A new POST endpoint is added.
Functionality: This endpoint accepts a request body containing a wallet_id and an amount, enabling users to manually initiate split executions for the specified wallet.

Feedback on this implementation is welcomed to refine its functionality. We hope for collaborative improvement for the benefit of the community.

@@ -21,6 +22,11 @@ async def api_targets_get(
return targets or []


@splitpayments_ext.post("/api/v1/execute_split", status_code=HTTPStatus.OK)
async def api_execute_split(wallet_id: str, amount: int) -> None:
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this should require a key (admin_key) to protect from malicious exploits

Choose a reason for hiding this comment

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

The usecase of this is to resolve this issue: #10

For me, this is exactly what I need, and I'd prefer it to be as simple as possible. Since it only executes splits that have already been set, I cannot see any obvious exploits.

What exploit do you see this could be used for?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Anyone can trigger splits... even if the splits are for known wallets (or not... other users may have a wide range of use cases)!

Choose a reason for hiding this comment

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

Anyone who knows the wallet can trigger a split, without having to know the admin key.

Which is the use case.

A person realizes it’s not getting paid out, so the trigger the split.

But receivers of the split shouldn’t necessarily have permission to alter the splits, which they could if they know the admin key.

I’m still not getting how it can be abused.

Copy link
Member

Choose a reason for hiding this comment

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

i agree with tal it has to require an admin key.

Copy link
Member

Choose a reason for hiding this comment

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

the issue is as wallet owner that is doing the splitting, i can chose not split 100% so the rest is staying in my wallet, now the others can basically repeat and can split the rest to themselves, if they just know my wallet_id

async def get_lnurl_invoice(
payoraddress, wallet_id, amount_msat, memo
payoraddress, wallet_id, amount_msat, memo
Copy link
Member

Choose a reason for hiding this comment

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

too much indentation

@@ -77,10 +77,57 @@ async def on_invoice_paid(payment: Payment) -> None:
)


async def execute_split(wallet_id, amount):
Copy link
Member

Choose a reason for hiding this comment

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

uses very similar logic than on_invoice_paid should be refactored into one

@dni
Copy link
Member

dni commented Feb 5, 2024

thanks for the contributiuon!

Automatic split execution occasionally fails to trigger under certain conditions. To ensure reliability and control in these scenarios, we have developed an endpoint that addresses the challenge.

i don't see this as a real solution for this problem, but an additional feature, endpoint where you can just split to those users and specify a custom amount, without the need of having a payment there in the first place. this could be useful if you have an initial amount in the wallet but did not create the split yet.

there could be a button on the ui, like we have for topup wallet with distribute to splits + amount utilizing this endpoint.

@dni
Copy link
Member

dni commented Feb 5, 2024

to the problem, maybe we can add an additional safety mechanism, where we fetch the latest 5 payments of this wallet and check if there is payment.extra.get("splitted") missing on some of those and if we retry splitting that payment on the on_invoice_paid event.

what do you think?

@MichaelAntonFischer
Copy link

thanks for the contributiuon!

Automatic split execution occasionally fails to trigger under certain conditions. To ensure reliability and control in these scenarios, we have developed an endpoint that addresses the challenge.

i don't see this as a real solution for this problem, but an additional feature, endpoint where you can just split to those users and specify a custom amount, without the need of having a payment there in the first place. this could be useful if you have an initial amount in the wallet but did not create the split yet.

there could be a button on the ui, like we have for topup wallet with distribute to splits + amount utilizing this endpoint.

Why not simply have an option to check after each split was triggered if there is more balance in the wallet than fee reserve and if yes, split that balance sub fee reserve?

@Opago-Pay
Copy link
Author

Opago-Pay commented Mar 22, 2024

to the problem, maybe we can add an additional safety mechanism, where we fetch the latest 5 payments of this wallet and check if there is payment.extra.get("splitted") missing on some of those and if we retry splitting that payment on the on_invoice_paid event.

what do you think?

Is the plugin flagging the incoming transactions as splitted? Upon checking it seems the extra data is only on the transactions generated by the plugin, but not the source transactions.

If it did add it to source, the best implementation would simply be both an automatic check on new invoice to see if the last few were split and an endpoint to manually check the last x transactions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants