[go: nahoru, domu]

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

[RFC] offer: allow re-enabling a previously disabled offer #7362

Merged

Conversation

vincenzopalazzo
Copy link
Collaborator
@vincenzopalazzo vincenzopalazzo commented Jun 2, 2024

Sometimes, for various reasons, a user disables an offer and then wants to re-enable it. This should be allowed because, from the CLN point of view, it is just an internal state.

If a user has constraints on the description of the invoice because they are using services that link some sort of user ID to an offer, it is important for the user to be able to re-enable the offer, not create a new one. Creating a new offer would require a different description.

P.S: This is in an RFC, if it will be accepted I will finish the documentation but I still need to write the test

Fixes: #7360

@rustyrussell
Copy link
Contributor

When I copied my test from the other PR, it shows why we need tests.

The final commit, which needs to be squashed, shows why...

@rustyrussell rustyrussell added this to the v24.08 milestone Aug 5, 2024
@vincenzopalazzo
Copy link
Collaborator Author

The final commit, which needs to be squashed, shows why...

Yes, this RFC was not finished, I was waiting to know if we want to have an enableoffer in cln. Thanks for the fixup I will add the docs and push the changes by the end of the day

@vincenzopalazzo
Copy link
Collaborator Author

When I copied my test from the other PR, it shows why we need tests.
The final commit, which needs to be squashed, shows why...

By accident, I force push the previous version with your code, so I remake the work, sorry about that.

NNow the code should be ready, probably I need to fix some failure with the docs, but should be a minor

lightningd/offer.c Outdated Show resolved Hide resolved
@rustyrussell
Copy link
Contributor

Rebased, and added a new return code a-la @Lagrang3 suggestion.

@rustyrussell rustyrussell force-pushed the macros/enable-an-offer branch 2 times, most recently from 1a908b1 to ebe9453 Compare August 10, 2024 05:45
vincenzopalazzo and others added 2 commits August 10, 2024 09:30
Sometimes, for various reasons, a user disables an offer
and then wants to re-enable it. This should be allowed because,
from the CLN point of view, it is just an internal state.

If a user has constraints on the description of the invoice
because they are using services that link some sort of user ID
to an offer, it is important for the user to be able to re-enable the
offer, not create a new one. Creating a new offer would
require a different description.

Link: ElementsProject#7360
Co-Developed-by: Rusty Russell <rusty@rustcorp.com.au>
Signed-off-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>
Suggested-by: https://github.com/Lagrang3
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
@vincenzopalazzo
Copy link
Collaborator Author
vincenzopalazzo commented Aug 10, 2024

Thanks for the @Lagrang3 review, sorry for the 3-day silence from my side, but I was taking a small holiday, and now I am back.

Added Rusty ad Co-Developed-by inside the 7bdc8db so when people will complain about this commit, can git blame also him :-P

In addition, I fixed a test failure regarding the code error in df46f0e now the CI should be happy!

Thanks to everyone for keeping this PR going also when I was off <3

@rustyrussell rustyrussell merged commit 975dd76 into ElementsProject:master Aug 11, 2024
37 checks passed
@vincenzopalazzo vincenzopalazzo deleted the macros/enable-an-offer branch August 12, 2024 07:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Please provide a way to enable disabled BOLT12 offer
3 participants