-
Notifications
You must be signed in to change notification settings - Fork 7
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
base: main
Are you sure you want to change the base?
Conversation
@@ -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: |
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.
I think this should require a key (admin_key) to protect from malicious exploits
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 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?
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.
Anyone can trigger splits... even if the splits are for known wallets (or not... other users may have a wide range of use cases)!
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.
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.
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.
i agree with tal it has to require an admin key.
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 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 |
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.
too much indentation
@@ -77,10 +77,57 @@ async def on_invoice_paid(payment: Payment) -> None: | |||
) | |||
|
|||
|
|||
async def execute_split(wallet_id, amount): |
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.
uses very similar logic than on_invoice_paid
should be refactored into one
thanks for the contributiuon!
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 |
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 what do you think? |
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? |
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. |
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.