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

Sendonion: add total amount #8015

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Lagrang3
Copy link
Collaborator

@Lagrang3 Lagrang3 commented Jan 20, 2025

Trying to use sendonion in renepay instead of sendpay I noticed that these two rpc's are not compatible
with each other in multi-part payments, ie. if I send a partial payment with sendpay and try to complete
the payment by sending another part with sendonion I get an error because in the internal wallet
sendpay sets the total_amount as specified by the user while sendonion does not making this amount to 0.
Lightningd will only accept further payment parts if the total amounts match. With sendonion there
isn't a field to pass the total amount so we cannot proceed.

With this PR I have added a parameter to sendonion called total_amount_msat in order to make the
sendpay and sendonion interfaces compatible.

@Lagrang3 Lagrang3 requested a review from cdecker as a code owner January 20, 2025 12:00
@Lagrang3 Lagrang3 force-pushed the sendonion-total-amount branch 2 times, most recently from 7be5a67 to 7593735 Compare January 20, 2025 12:34
@Lagrang3 Lagrang3 added this to the v25.02 milestone Feb 6, 2025
@endothermicdev
Copy link
Collaborator

The parameter is accepted, but, unless I'm missing something, I think sendonion needs more modification to be used for this application. At least when I tried writing a test to validate this by sending two parts, sendonion comes back with Never attempted payment part 0. I think it would be premature to add the parameter when we can't use or test it yet. Let's wait and get that modification in at the same time. (Or if I'm just messing up the test and it works as is, let me know.)

@Lagrang3 Lagrang3 force-pushed the sendonion-total-amount branch from 7593735 to 8e542ca Compare February 15, 2025 08:23
@Lagrang3
Copy link
Collaborator Author

Lagrang3 commented Feb 15, 2025

@endothermicdev, I've added a test.
Thanks for reviewing this.

@Lagrang3 Lagrang3 force-pushed the sendonion-total-amount branch from 8e542ca to a2fd84b Compare February 15, 2025 11:12
@Lagrang3
Copy link
Collaborator Author

Rebased and fixed conflicts.

@Lagrang3
Copy link
Collaborator Author

@endothermicdev, help.
I don't know why, but make check-gen-updated is failing.

@endothermicdev
Copy link
Collaborator

Ah, just need to run make gen - some of the msggen diff got lost. Nice test.

@endothermicdev endothermicdev force-pushed the sendonion-total-amount branch 5 times, most recently from 839e0ef to dbcb0cb Compare February 19, 2025 16:31
@rustyrussell
Copy link
Contributor

I suspect this will break pay, which uses sendonion only, and relies on the 0 total_msat?

@endothermicdev
Copy link
Collaborator

As this seems to be causing some downstream issues, but nothing is currently depending on it, I'm going to defer to 25.05 at this point.

@endothermicdev endothermicdev modified the milestones: v25.02, v25.05 Feb 21, 2025
@Lagrang3
Copy link
Collaborator Author

I suspect this will break pay, which uses sendonion only, and relies on the 0 total_msat?

I will check.

Changelog-Added: sendonion: a new paramter total_amount_msat to make MPP payments with sendpay and sendonion compatible.

Signed-off-by: Lagrang3 <[email protected]>
@Lagrang3 Lagrang3 force-pushed the sendonion-total-amount branch from 57a534b to 436b370 Compare February 24, 2025 09:22
@Lagrang3
Copy link
Collaborator Author

I fixed the backwards compatibility with pay and rebased.

@Lagrang3
Copy link
Collaborator Author

@endothermicdev make gen did not produce any new files for me...

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.

3 participants