Closed grunch closed 2 years ago
I've been trying to figure this out. There are some things that I couldn't understand:
How the orders that follow the paths below achieve a SUCCESS
status in database?
We can make some assumptions about the status of the lightning node connected to the bot? That would discard the situation where the node goes offline and the bot server restarts.
I'm biased by the /buy orders because I've never done a /sell order. Is the /release command executed at some point of the transaction? It is not executed? What is the flow of the program in that case?
This is the order workflow
order type=buy
Order published (PENDING) -> taken (WAITING_PAYMENT) -> bot ask buyer invoice (WAITING_BUYER_INVOICE) -> ACTIVE -> /fiatsent (FIAT_SENT) -> /release (PAID_HOLD_INVOICE) -> after buyer's invoice paid (SUCCESS)
order type=sell
Order published (PENDING) -> taken (WAITING_BUYER_INVOICE) -> bot ask seller to pay invoice (WAITING_PAYMENT) -> ACTIVE -> /fiatsent (FIAT_SENT) -> /release (PAID_HOLD_INVOICE) -> after buyer's invoice paid (SUCCESS)
On waitPayment()
we create the hold invoice to seller and create a nodejs event to listen when that invoice status change, that way we can monitorize and do stuff when we receive the seller's payment.
The issue is that I don't know how to persist that event if nodejs crash for any reason, we can save it on database and delete it after the seller release the payment but that implies that we need to change our logic on subscribeInvoice()
, that's not my first option, I am want to hear ideas before do that.
I was thinking that the we-can re subscribe to the invoice when executing the /release
command using the order.hash
. We could even recycle the subscribeInvoice
function.
On /release
execution, if invoice.is_held
some messages will be printed again and we should verify that the logic triggered under that condition doesn't roll back things in the database, . If invoice.is_confirmed
we should check again that the logic doesn't roll back changes in database, but it will put the order in path to SUCCESS
again.
WDYT?
That's sounds great! I think we should check that re-subscribe and tie again the process doesn't get some unexpected errors, but the idea is simple and what I was looking for, great idea 🥇
Good! I will start testing by my own.
I thought about this, the key is re-subscribe, I think the easiest way of doing it is:
is_held: true
we query order
by hashsubscribeInvoice(bot, hash)
Before push this to production we need to test we can test it each separately.
I've reproduced the behavior detailed above in this branch. I had to create a new function for lightning service and change some things in the files related to the server main process to make it work properly.
I've tested it in the following cases:
/fiatsent
is executed./fiatsent
./release
./release
is executed./fiatsent
./release
.Info: In both cases the path followed by the code is: resubscribeInvoices
=> subscribeInvoice
=> onGoingTakeBuyMessage
.
Drawback: The messages re sent didn't give any clue to the user about what was happening or if something gone wrong with their orders. A message explaining the situation could be helpful.
Caveat: The parameter is_unconfirmed
in the function getInvoices
seems to be not working, because the use of different values with that parameter return the same responses. I have tested it with the following code:
const unconfirmedInvoices = (await getInvoices({ lnd, is_unconfirmed: true })).invoices;
const confirmedInvoices = (await getInvoices({ lnd, is_unconfirmed: false })).invoices;
console.log(JSON.stringify(confirmedInvoices)==JSON.stringify(unconfirmedInvoices));
Great job!! I just tested and made some small fixes, here is the patch, apply it and try again
diff --git a/ln/resubscribe_invoices.js b/ln/resubscribe_invoices.js
index 52d774a..e0179c7 100644
--- a/ln/resubscribe_invoices.js
+++ b/ln/resubscribe_invoices.js
@@ -14,16 +14,17 @@ const resubscribeInvoices = async (bot) => {
if (Array.isArray(unconfirmedInvoices) && unconfirmedInvoices.length > 0) {
const heldInvoices = unconfirmedInvoices.filter(isHeld);
for (const invoice of heldInvoices) {
- const orderInDB = Order.findById(invoice.id);
+ const orderInDB = await Order.findOne({ hash: invoice.id });
if(!!orderInDB) {
- await subscribeInvoice(bot, invoice.id);
- invoicesReSubscribed++;
+ console.log(`Re-subscribing: invoice with hash: ${invoice.id} is being held!`);
+ await subscribeInvoice(bot, invoice.id, true);
+ invoicesReSubscribed++;
}
};
}
console.log(`Invoices resubscribed: ${invoicesReSubscribed}`);
} catch (error) {
- console.log(`resuscribeInvoice catch: ${error}`);
+ console.log(`ResuscribeInvoice catch: ${error}`);
return false;
}
};
diff --git a/ln/subscribe_invoice.js b/ln/subscribe_invoice.js
index 8173436..29f93f5 100644
--- a/ln/subscribe_invoice.js
+++ b/ln/subscribe_invoice.js
@@ -4,11 +4,11 @@ const { payToBuyer } = require('./pay_request');
const lnd = require('./connect');
const messages = require('../bot/messages');
-const subscribeInvoice = async (bot, id) => {
+const subscribeInvoice = async (bot, id, resub) => {
try {
const sub = subscribeToInvoice({ id, lnd });
sub.on('invoice_updated', async (invoice) => {
- if (invoice.is_held) {
+ if (invoice.is_held && !resub) {
console.log(`invoice with hash: ${id} is being held!`);
const order = await Order.findOne({ hash: invoice.id });
const buyerUser = await User.findOne({ _id: order.buyer_id });
I've just tested it. It works perfectly. For the sake of completeness I updated my test cases results:
Order = BUY.
/fiatsent
is executed./fiatsent
command./fiatsent
is executed./release
is executed. Order = BUY.
/release
is executed./release
command./release
is executed. I will open a PR with the changes.
If nodejs restarts at the time an invoice is held
Node loses tracking of this event and when releasing, if the invoice is paid, but the subscribeToInvoice does not follow the normal workflow, the status is not changed to
PAID_HOLD_INVOICE
and as a consequence the buyer is not paid.We need to think a way of avoid this situation, one is never restart nodejs if at least one invoice is held, but we need to have a procedure to follow if the server crashes.