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

keysend: Add maxfee to keysend for consistency with pay #7653

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

Conversation

s373nZ
Copy link
Contributor

@s373nZ s373nZ commented Sep 9, 2024

Add maxfee to keysend for consistency with pay

Description

Adds maxfee parameter to keysend with similar functionality of that of pay. Needed to achieve more consistent behavior across API endpoints.

Related Issues

Changes Made

  • Feature: Adds maxfee parameter to keysend with similar functionality of that of pay.
  • Bug Fix: Fixed a minor typo in keysend documentation comments.
  • Refactor: Any code improvements or refactoring done without changing the functionality.

Checklist

Ensure the following tasks are completed before submitting the PR:

  • Changelog has been added in relevant commit/s.
  • Tests have been added or updated to cover the changes.
    • Added a test for plugin parameter format.
    • Added a test in test_pay.py to test parameter validation, keysend over route exceeding maxfee, and keysend w/ maxfee set.
  • Documentation has been updated as needed.
  • Any relevant comments or TODOs have been addressed or removed.

Additional Notes

  • I initially added the parameter directly after the exemptfee param, but realized order must be important for scripts. Regardless, I've added to the end of documented parameters for backward compatibility, yet there is a "hidden" dev_use_shadow param appearing only in the cli documentation which it appears before.

@s373nZ
Copy link
Contributor Author

s373nZ commented Sep 9, 2024

TODO:

  • Perform a more reproducible build and commit accurately generated files for:
    • contrib/pyln-grpc-proto/pyln/grpc/node_pb2.py
    • contrib/pyln-grpc-proto/pyln/grpc/node_pb2_grpc.py (wasn't modified after repro build)
    • contrib/pyln-grpc-proto/pyln/grpc/primitives_pb2.py (wasn't modified after repro build)
  • Research more about keysend and the c implementation to be more sure changes are accurate.
  • There doesn't seem to be explicitly tests in the pay plugin for maxfee functionality to emulate, but determine whether this feature needs automated testing.

@s373nZ s373nZ force-pushed the 7227-add-maxfee-to-lightning-keysend branch 2 times, most recently from cae88d3 to f6fc5ae Compare September 9, 2024 12:30
@s373nZ
Copy link
Contributor Author

s373nZ commented Sep 9, 2024

Fixed the cln-grpc Rust test. I didn't think a Draft PR would trigger all the CI. Apologies for the build spam.

@s373nZ s373nZ force-pushed the 7227-add-maxfee-to-lightning-keysend branch from f6fc5ae to df06cff Compare September 11, 2024 06:51
@s373nZ
Copy link
Contributor Author

s373nZ commented Sep 11, 2024

Added tests and fixed some of the logic for default values of the other parameters.

The maxfee handling logic is more or less a duplication of that found in the pay plugin. Perhaps this could be abstracted and shared? Will look into a little further, but I wonder where the appropriate place for that would be...

@s373nZ s373nZ force-pushed the 7227-add-maxfee-to-lightning-keysend branch from df06cff to 9de05c3 Compare September 11, 2024 14:22
@s373nZ
Copy link
Contributor Author

s373nZ commented Sep 11, 2024

Sacrificed two semicolons to the flake8 gods.

@s373nZ s373nZ force-pushed the 7227-add-maxfee-to-lightning-keysend branch from 9de05c3 to 05a068a Compare September 13, 2024 08:50
@s373nZ
Copy link
Contributor Author

s373nZ commented Sep 13, 2024

Reran mssgen and regenerated the documentation examples as per the docs for good measure. Used the following commands:

PYTHONPATH=contrib/msggen python contrib/msggen/msggen/__main__.py bundle doc/schemas/
PYTHONPATH=contrib/msggen python contrib/msggen/msggen/__main__.py generate

make check-gen-updated

rm -rf /tmp/ltests* && REGENERATE='keysend' VALGRIND=0 TIMEOUT=10 TEST_DEBUG=1 pytest -vvv -s -p no:logging -n 6 tests/autogenerate-rpc-examples.pyrm -rf /tmp/ltests* && REGENERATE='keysend' VALGRIND=0 TIMEOUT=10 TEST_DEBUG=1 pytest -vvv -s -p no:logging -n 6 tests/autogenerate-rpc-examples.py

make

The CI build is currently failing, but observation suggests this is happening with all PR branches due to something in master. Will keep an eye on the builds and continue to rebase. It looks like this change resulted in a successful build. AFAICT this is ready for an initial review.

@s373nZ s373nZ marked this pull request as ready for review September 13, 2024 09:57
@s373nZ s373nZ force-pushed the 7227-add-maxfee-to-lightning-keysend branch 2 times, most recently from 5a857fd to 5fed120 Compare September 17, 2024 12:59
@s373nZ
Copy link
Contributor Author

s373nZ commented Sep 17, 2024

Rebased, regenerated proto code and removed modifications to CHANGELOG.md for Changelog-Added: hint in commit message.

@s373nZ s373nZ force-pushed the 7227-add-maxfee-to-lightning-keysend branch 3 times, most recently from 10af5ea to 0772906 Compare September 22, 2024 08:47
…Project#7227])

Changelog-Added: keysend: Add `maxfee` to keysend for consistency with pay. ([ElementsProject#7227])
@s373nZ s373nZ force-pushed the 7227-add-maxfee-to-lightning-keysend branch from 0772906 to fdd60db Compare September 24, 2024 09:52
@ShahanaFarooqui ShahanaFarooqui added this to the v24.11 milestone Sep 25, 2024
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.

grpc api: Same fee limit handling for keysend and pay
2 participants