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

Fix Binding Outlay Tracking, update API as a result #97

Merged
merged 22 commits into from
Jul 8, 2024

Conversation

farscapian
Copy link
Collaborator

@farscapian farscapian commented Jun 19, 2024

On the production ROYGBIV prism instance, I was having some issues with the plugin not fully completing when an incoming payment was received. The plugin was making payments on mainnet and attempting to update the outlay to a negative number (mainnet is the only place we've tested where fees are non-zero). Negative numbers for outlay should be possible. The issue is, we originally defined the outlay to be a Millisatoshi type, which apparently doesn't support negative numbers. So the first part of this PR includes fixing that issue. I completely remove Millisatoshi from the picture and rely on python's built-in int type for tracking the outlays. This has the correct behavior with negative numbers. I also added RPC method for setting the outlay value (closing #95 and #50 ). I should also mention that I REMOVED the update_outlays function and moved that logic Prism.pay. This allows us to update outlays after each payment attempt rather than waiting to update outlays until all payments have completed (assuming they complete!).

Unfortunately, this change impacts the API. Since this is a necessary fix, I decided to go ahead and implement other Issues which also affect the API. Per the advice from some CLN folks, I updated the prism_id and member_id mechanism (#88 #87 #86 ). Now prisms and members each get a unique ID at creation time and in format similar to offer_ids. In addition, I changed "label" values to "description". I also changed the "split" parameter to type float. This allows users to be more expressive in terms of how they provide the split. I also made payment_threshold_msat a positive int.

There's also a bug fix: #96 and documentation updates. I updated the example to a Band Prism so we can hopefully demonstrate relatable prism infrastructure to people.

@farscapian farscapian requested a review from gudnuf June 19, 2024 03:48
@farscapian farscapian self-assigned this Jun 19, 2024
lib.py Show resolved Hide resolved
lib.py Show resolved Hide resolved
lib.py Show resolved Hide resolved
@gudnuf
Copy link
Owner

gudnuf commented Jun 29, 2024

@farscapian I pushed a test for update_outlay and opened #99 to resolve a minor bug. Otherwise, worked well, just need to make sure we fix this migration issue

deleting label and changing split type is not backwards compatible with old DB entries.

@farscapian
Copy link
Collaborator Author

farscapian commented Jun 29, 2024

I think we planned for this eventuality by namespacing the version number into the db key hierarchy (prism_db_version). So, since we have an incompatible version, we should bump the db key namespace so there are no issues. (I had to do some manual db cleanups on the ROYGBIV instance due to this issue.)

Once we change the db namespace, we have another question:

  1. Provide migration logic that we invoke on initialization. This is something that will be necessary once we have actual users.
  2. Inform people of the incompatibility in the Release Notes. They can just do a prism-list and bindinglist before they upgrade, then re-create their prisms afterward.

I think due to the newness of this plugin, we can inform users via Release Notes and Telegram channel and work on migration logic for future incompatibilities.

@gudnuf
Copy link
Owner

gudnuf commented Jul 3, 2024

I think due to the newness of this plugin, we can inform users via Release Notes and Telegram channel and work on migration logic for future incompatibilities.

Yeah this is fine with me, probably very few if any are using the plugin

Provide migration logic that we invoke on initialization. This is something that will be necessary once we have actual users.

This shouldn't be too hard. Just on initialization we look through the prism related db records, modify them as need, then save updated. The versioning will be key here.

On that note, this seems good to merge to me. Has the CI that uses PY=3.12 been failing before? Seems like some issue with the CI not our tests

PS, sorry I missed this again, gotta figure out whats up with my GH notifications.

@farscapian
Copy link
Collaborator Author

Yeah I'm not sure why that thing isn't completing. We haven't changed anything related to CI afaik, and the error isn't prism related. I have a couple of minor updates to push including bumping the db version. Plus I want to update the documentation again. I've seen a couple errors I want to correct. I'm also trying to unify the example in these docs with roygbiv.guide/example (switching to a Band Prism example). Give me a couple more days.

@@ -2,6 +2,8 @@

set -eu

docker system prune -f
Copy link
Owner

Choose a reason for hiding this comment

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

Will this prune the entire system? Is that acceptable to clear docker related things that aren't related to this plugin? Can we instead just rebuild the image and tear it down when tests complete?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You are correct, removing from latest PR fix-readme.

@@ -46,21 +46,20 @@ def validate(member):
raise Exception(
"Destination must be a valid lightning node pubkey or bolt12 offer.")

if not isinstance(member["split"], int):
raise ValueError("Member 'split' must be an integer")
if not isinstance(member["split"], float):
Copy link
Owner

Choose a reason for hiding this comment

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

deleting label and changing split type is not backwards compatible with old DB entries. I think we should add a run_migrations call when plugin starts. This would do things like update label to description and change splits to type float.

If something is not done, then you get an error like this
image

Copy link
Owner

Choose a reason for hiding this comment

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

The other option is add different logic depending on prism version, but I think we should prefer migrating prisms to latest version where posssible

@gudnuf gudnuf merged commit b21305c into main Jul 8, 2024
2 of 3 checks passed
@gudnuf gudnuf deleted the api/label-to-description branch July 8, 2024 16:32
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.

Bug: prism-pay does not respect the outlay_factor
2 participants