search

gudnuf / bolt12-prism

CLN plugin for lightning prisms using BOLT 12
15 stars 5 forks source link

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

Closed farscapian closed 2 months ago

farscapian commented 2 months ago

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.

gudnuf commented 2 months ago

@farscapian I pushed a test for update_outlay and opened https://github.com/gudnuf/bolt12-prism/pull/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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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.