feat: Lightning Client NG Pay Invoice #2279

m1sterc001guy · 2023-04-20T18:45:18Z

Lightning Client NG paying a bolt 11 invoice. Adds the Pay state machine.

codecov · 2023-04-20T18:58:08Z

Codecov Report

Patch coverage: 5.57% and project coverage change: -1.30 ⚠️

Comparison is base (88bb9c3) 57.46% compared to head (acc5a9b) 56.16%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2279       /-   ##
==========================================
- Coverage   57.46%   56.16%   -1.30%     
==========================================
  Files         163      167        4     
  Lines       33738    34551      813     
==========================================
  Hits        19386    19405       19     
- Misses      14352    15146      794

Impacted Files	Coverage Δ
fedimint-cli/src/lib.rs	`5.24% <0.00%> (ø)`
fedimint-cli/src/ng.rs	`0.00% <0.00%> (ø)`
fedimint-client-legacy/src/lib.rs	`74.52% <0.00%> (ø)`
gateway/ln-gateway/src/actor.rs	`46.48% <0.00%> (ø)`
gateway/ln-gateway/src/lib.rs	`48.67% <0.00%> (ø)`
gateway/ln-gateway/src/rpc/mod.rs	`80.51% <ø> (ø)`
gateway/ln-gateway/src/rpc/rpc_server.rs	`63.38% <ø> (ø)`
modules/fedimint-ln-client/src/api.rs	`0.00% <0.00%> (ø)`
modules/fedimint-ln-client/src/db.rs	`0.00% <0.00%> (ø)`
modules/fedimint-ln-client/src/pay.rs	`0.00% <0.00%> (ø)`
... and 10 more

... and 9 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

elsirion

Great progress! Just a few high-level comments about the API, will go through the actual state machines more thoroughly tomorrow :)

fedimint-cli/src/ng.rs

modules/fedimint-ln-client/src/pay.rs

modules/fedimint-ln-client/src/lib.rs

modules/fedimint-ln-client/src/db.rs

modules/fedimint-ln-client/src/pay.rs

modules/fedimint-ln-client/src/lib.rs

gateway/ln-gateway/src/lib.rs

justinmoon · 2023-04-23T00:47:56Z

The CLI commands all worked for me 🎉

modules/fedimint-ln-client/src/pay.rs

m1sterc001guy · 2023-04-26T19:48:57Z

modules/fedimint-ln-client/src/pay.rs

+        old_state: LightningPayStateMachine,
+        contract_id: ContractId,
+    ) -> LightningPayStateMachine {
+        // TODO: Retry contacting gateway


I didn't implement this yet. What do we want the policy to be here? Try 5 times then go to Refundable? Try forever until we contact the gateway?

modules/fedimint-ln-client/src/pay.rs

m1sterc001guy · 2023-04-26T19:51:42Z

modules/fedimint-ln-client/src/pay.rs

+        let rejected_common = funded_common.clone();
+        let success_context = global_context.clone();
+        let reject_context = global_context.clone();
+        let gateway = self.gateway.clone();


For simplicity's sake while implementing this I just cloned everything. I don't think that's too expensive but its probably not necessary.

elsirion

LGTM, some nits but it works and we can improve incrementally

elsirion · 2023-04-27T16:19:14Z

modules/fedimint-mint-client/src/lib.rs

+
+    // Wait for the refund transaction to complete after a lightning invoice
+    // failed to be paid.
+    async fn await_lightning_refund(
+        &self,
+        operation_id: OperationId,
+        refund_txid: TransactionId,
+    ) -> anyhow::Result<()>;


Can we just wait for the transaction to be accepted? I know I have to add a "await output finalization" function to the PrimaryModule API, but that doesn't seem high prio since just awaiting tx acceptance is sufficient to guarantee we will get the refund eventually.

So this is just a quirk with the CLI that I pinged you about offline. The CLI just needs to be alive long enough for the output to be finalized so the notes can be retrieved. The Pay state machine doesn't actually wait on this, it just waits for the refund tx to be accepted then exits. We can refactor this if needed

Ah, I see, was missing that context. I'll implement an abstraction that removes the dependency on the mint module, but good to go for now.

elsirion · 2023-04-27T16:31:07Z

modules/fedimint-ln-client/src/pay.rs

+    Refunded(TransactionId),
+    Failure(TransactionId),


I think we could remove these and instead query tx acceptance in LnClientExt::subscribe_ln_pay_updates, a lot like I did here (definitely not your fault, the API for that just became available 😆 we can patch it out later):

fedimint/modules/fedimint-mint-client/src/lib.rs

Lines 291 to 315 in 4d53190

Ok(Box::pin(stream! {

yield SpendOOBState::Created;

let refund = refund_future.await;

if refund.user_triggered {

yield SpendOOBState::UserCanceledProcessing;

match tx_subscription.await_tx_accepted(refund.transaction_id).await {

Ok(()) => {

yield SpendOOBState::UserCanceledSuccess;

},

Err(()) => {

yield SpendOOBState::UserCanceledFailure;

}

}

} else {

match tx_subscription.await_tx_accepted(refund.transaction_id).await {

Ok(()) => {

yield SpendOOBState::Refunded;

},

Err(()) => {

yield SpendOOBState::Success;

}

}

}

}))

elsirion · 2023-04-27T17:01:42Z

modules/fedimint-ln-client/src/pay.rs

+        gateway: LightningGateway,
+    ) -> LightningPayStateMachine {
+        assert!(matches!(
+            old_state.state,
+            LightningPayStates::CreatedOutgoingLnContract(_)
+        ));


If you were using the data from inside old_state.state you wouldn't need to pass in the gateway, potentially saving some clones.

modules/fedimint-ln-client/src/pay.rs

elsirion reviewed Apr 20, 2023

View reviewed changes

fedimint-cli/src/ng.rs Outdated Show resolved Hide resolved

fedimint-cli/src/ng.rs Outdated Show resolved Hide resolved

fedimint-cli/src/ng.rs Outdated Show resolved Hide resolved

m1sterc001guy commented Apr 20, 2023

View reviewed changes

modules/fedimint-ln-client/src/pay.rs Outdated Show resolved Hide resolved

m1sterc001guy commented Apr 20, 2023

View reviewed changes

modules/fedimint-ln-client/src/lib.rs Outdated Show resolved Hide resolved

m1sterc001guy commented Apr 20, 2023

View reviewed changes

modules/fedimint-ln-client/src/lib.rs Outdated Show resolved Hide resolved

elsirion reviewed Apr 21, 2023

View reviewed changes

modules/fedimint-ln-client/src/db.rs Outdated Show resolved Hide resolved

m1sterc001guy commented Apr 21, 2023

View reviewed changes

modules/fedimint-ln-client/src/pay.rs Show resolved Hide resolved

m1sterc001guy commented Apr 21, 2023

View reviewed changes

modules/fedimint-ln-client/src/lib.rs Outdated Show resolved Hide resolved

m1sterc001guy commented Apr 21, 2023

View reviewed changes

gateway/ln-gateway/src/lib.rs Outdated Show resolved Hide resolved

justinmoon reviewed Apr 23, 2023

View reviewed changes

modules/fedimint-ln-client/src/pay.rs Outdated Show resolved Hide resolved

m1sterc001guy force-pushed the lightning_pay_sm branch 5 times, most recently from a4014fe to 6eeb381 Compare April 26, 2023 19:39

m1sterc001guy commented Apr 26, 2023

View reviewed changes

modules/fedimint-ln-client/src/pay.rs Outdated Show resolved Hide resolved

m1sterc001guy commented Apr 26, 2023

View reviewed changes

m1sterc001guy force-pushed the lightning_pay_sm branch from 6eeb381 to 73abac3 Compare April 26, 2023 19:53

m1sterc001guy marked this pull request as ready for review April 26, 2023 19:57

m1sterc001guy requested review from a team as code owners April 26, 2023 19:57

m1sterc001guy force-pushed the lightning_pay_sm branch from 73abac3 to 8337930 Compare April 26, 2023 20:18

m1sterc001guy mentioned this pull request Apr 27, 2023

fix(lightning): replace bincode encoding impl #2333

Merged

elsirion previously approved these changes Apr 27, 2023

View reviewed changes

m1sterc001guy dismissed elsirion’s stale review via 909b087 April 27, 2023 17:18

m1sterc001guy force-pushed the lightning_pay_sm branch from 8337930 to 909b087 Compare April 27, 2023 17:18

feat: Lightning Pay State Machine

acc5a9b

m1sterc001guy force-pushed the lightning_pay_sm branch from 909b087 to acc5a9b Compare April 27, 2023 19:15

elsirion approved these changes Apr 27, 2023

View reviewed changes

elsirion merged commit 15830ac into fedimint:master Apr 27, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Lightning Client NG Pay Invoice #2279

feat: Lightning Client NG Pay Invoice #2279

m1sterc001guy commented Apr 20, 2023 •

edited

Loading

codecov bot commented Apr 20, 2023 •

edited

Loading

elsirion left a comment

justinmoon commented Apr 23, 2023

m1sterc001guy Apr 26, 2023

m1sterc001guy Apr 26, 2023

elsirion left a comment

elsirion Apr 27, 2023

m1sterc001guy Apr 27, 2023

elsirion Apr 27, 2023

elsirion Apr 27, 2023

elsirion Apr 27, 2023

	Ok(Box::pin(stream! {
	yield SpendOOBState::Created;

	let refund = refund_future.await;
	if refund.user_triggered {
	yield SpendOOBState::UserCanceledProcessing;
	match tx_subscription.await_tx_accepted(refund.transaction_id).await {
	Ok(()) => {
	yield SpendOOBState::UserCanceledSuccess;
	},
	Err(()) => {
	yield SpendOOBState::UserCanceledFailure;
	}
	}
	} else {
	match tx_subscription.await_tx_accepted(refund.transaction_id).await {
	Ok(()) => {
	yield SpendOOBState::Refunded;
	},
	Err(()) => {
	yield SpendOOBState::Success;
	}
	}
	}
	}))

feat: Lightning Client NG Pay Invoice #2279

feat: Lightning Client NG Pay Invoice #2279

Conversation

m1sterc001guy commented Apr 20, 2023 • edited Loading

codecov bot commented Apr 20, 2023 • edited Loading

Codecov Report

elsirion left a comment

Choose a reason for hiding this comment

justinmoon commented Apr 23, 2023

m1sterc001guy Apr 26, 2023

Choose a reason for hiding this comment

m1sterc001guy Apr 26, 2023

Choose a reason for hiding this comment

elsirion left a comment

Choose a reason for hiding this comment

elsirion Apr 27, 2023

Choose a reason for hiding this comment

m1sterc001guy Apr 27, 2023

Choose a reason for hiding this comment

elsirion Apr 27, 2023

Choose a reason for hiding this comment

elsirion Apr 27, 2023

Choose a reason for hiding this comment

elsirion Apr 27, 2023

Choose a reason for hiding this comment

m1sterc001guy commented Apr 20, 2023 •

edited

Loading

codecov bot commented Apr 20, 2023 •

edited

Loading