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.

Sign up for GitHub

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

Jump to bottom

Add support for liquidity mgmt. via lightning-liquidity #223

Merged
merged 8 commits into from
Feb 21, 2024
Merged

Add support for liquidity mgmt. via lightning-liquidity #223

merged 8 commits into from
Feb 21, 2024

Conversation

tnull
Copy link
Collaborator

@tnull tnull commented Dec 20, 2023 •
edited
Loading

Closes #194

We add initial, alpha-stage support for liquidity-management via LSPS2/lightning-liquidity.

Draft as still work in progress (and blocked on lightningdevkit/lightning-liquidity#73 / lightningdevkit/lightning-liquidity#74).

@tnull tnull marked this pull request as draft December 20, 2023 14:18
@tnull tnull force-pushed the 2023-12-liquidity-mgmt-integration branch 15 times, most recently from e8cfd19 to e12d8d4 Compare January 9, 2024 16:06
@tnull tnull force-pushed the 2023-12-liquidity-mgmt-integration branch 6 times, most recently from 43d5f5f to a626584 Compare January 11, 2024 15:39
jkczyz
jkczyz reviewed Jan 16, 2024
View reviewed changes
src/lib.rs Show resolved Hide resolved
src/lib.rs Show resolved Hide resolved
src/lib.rs Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Show resolved Hide resolved
@tnull tnull force-pushed the 2023-12-liquidity-mgmt-integration branch 6 times, most recently from 6644bd0 to ba1fd79 Compare January 23, 2024 15:25
@tnull tnull force-pushed the 2023-12-liquidity-mgmt-integration branch from ba1fd79 to 8db5970 Compare January 24, 2024 14:35
@tnull tnull force-pushed the 2023-12-liquidity-mgmt-integration branch from 5d597be to 753be94 Compare February 5, 2024 10:36
jkczyz
jkczyz reviewed Feb 6, 2024
View reviewed changes
src/liquidity.rs Outdated Show resolved Hide resolved
src/event.rs Outdated Show resolved Hide resolved
src/liquidity.rs
Comment on lines +91 to +100
if let Some(lsps2_service) = self.lsps2_service.as_ref() {
if counterparty_node_id != lsps2_service.node_id {
debug_assert!(
false,
"Received response from unexpected LSP counterparty. This should never happen."
);
log_error!(
self.logger,
"Received response from unexpected LSP counterparty. This should never happen."
);
return;
}
Copy link

Choose a reason for hiding this comment

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

Gotcha. Shuffling things around and doing a pre-match might help eliminated some duplication (haven't actually tried this):

match event {
	Event::LSPS2Client(client_event) => match self.lsps2_service.as_ref() {
		Some(lsps2_service) => {
			match &client_event {
				LSPS2ClientEvent::OpeningParametersReady { counterparty_node_id, .. }
					| LSPS2ClientEvent::InvoiceParametersReady { counterparty_node_id, .. } => { /* */},
			};
			match client_event {
				LSPS2ClientEvent::OpeningParametersReady { /* */ .. } => { /* */ },
				LSPS2ClientEvent::InvoiceParametersReady { /* */ .. } => { /* */ },
			};
		},
		None => { /* */},
	}
};

src/liquidity.rs Outdated Show resolved Hide resolved
@tnull tnull force-pushed the 2023-12-liquidity-mgmt-integration branch from 753be94 to 6beb61c Compare February 6, 2024 10:01
jkczyz
jkczyz reviewed Feb 6, 2024
View reviewed changes
Copy link

@jkczyz jkczyz left a comment

Choose a reason for hiding this comment

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

Code looks good. What's the testing strategy?

src/liquidity.rs
Comment on lines +91 to +100
if let Some(lsps2_service) = self.lsps2_service.as_ref() {
if counterparty_node_id != lsps2_service.node_id {
debug_assert!(
false,
"Received response from unexpected LSP counterparty. This should never happen."
);
log_error!(
self.logger,
"Received response from unexpected LSP counterparty. This should never happen."
);
return;
}
Copy link

Choose a reason for hiding this comment

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

Even if you prefer repeating the counterparty_id check, it seems better to check lsps2_service before matching any LSPS2ClientEvent. But will defer to your preference. I could see indentation getting out of hand.

@tnull
Copy link
Collaborator Author

tnull commented Feb 6, 2024

Code looks good. What's the testing strategy?

For now we can only test manually against Testnet/Regtest/Signet nodes (actually, running on Mutinynet). We will also looking to add more testing utils to the lightning-liquidity crate (I'm thinking something similar to the Node code in LDK's lightning-background-processor tests) that would allow us to write more end-to-end tests there. Here in LDK Node I'm a bit on the fence whether to add some really simple, cfg-gated server-side logic that would allow us to also do end-to-end testing in our Rust integration tests. Might be worth it, but could also be rather invasive.

@tnull
Copy link
Collaborator Author

tnull commented Feb 12, 2024

@jkczyz Let me know when I can squash the fixups.

@jkczyz
Copy link

jkczyz commented Feb 12, 2024

@jkczyz Let me know when I can squash the fixups.

Sure, feel free to squash.

@tnull tnull force-pushed the 2023-12-liquidity-mgmt-integration branch from 6beb61c to e37398e Compare February 13, 2024 09:27
@tnull
Copy link
Collaborator Author

tnull commented Feb 13, 2024

Sure, feel free to squash.

Squashed fixups without further changes.

@tnull tnull force-pushed the 2023-12-liquidity-mgmt-integration branch from e37398e to ffdf5c0 Compare February 15, 2024 14:49
@tnull
Copy link
Collaborator Author

tnull commented Feb 15, 2024

Rebased on main to fix CI and also updated to use the just-released 0.1.0-alpha version of lightning-liquidity.

Notably, the {min,max}_payment_size_msat fields are now moved to OpeningFeeParams, which we account for in some fixups.

@tnull tnull force-pushed the 2023-12-liquidity-mgmt-integration branch from ffdf5c0 to 3248f0d Compare February 15, 2024 15:04
jkczyz
jkczyz reviewed Feb 15, 2024
View reviewed changes
Copy link

@jkczyz jkczyz left a comment

Choose a reason for hiding this comment

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

LGTM. Please squash.

@tnull tnull force-pushed the 2023-12-liquidity-mgmt-integration branch from 3248f0d to f011f10 Compare February 16, 2024 10:17
@tnull
Copy link
Collaborator Author

tnull commented Feb 16, 2024

LGTM. Please squash.

Squashed without further changes.

@tnull
Minor error message fixes
2e4936f
@tnull
Have connect_open_channel return a UserChannelId
dabbb4d
@tnull
Have close_channel take a UserChannelId
4d3b600 
... which is nice for consistency reasons.

We also fix a minor bug that would have lead us to forget a peer
whenever we'd close a channel, even if it wasn't the last one.
@tnull
Have update_channel_config take a UserChannelId
86d1a1f 
... which is nice for consistency reasons.
@tnull
Implement LiquiditySource CMH
61b2f37 
We add a `LiquiditySource` object that is configurable via builder
methods, currently allowing to source inbound liquidity from an LSPS2
service.

In the next commit(s) we'll add corresponding API and event handling.
@tnull
Implement LiquidityManager event handling
338b9c5
@tnull
Implement LSPS2 invoice creation flow
49f497f
@tnull
Allow for variable amount payments
3c05b17
@tnull tnull force-pushed the 2023-12-liquidity-mgmt-integration branch from f011f10 to 3c05b17 Compare February 19, 2024 09:25
@tnull
Copy link
Collaborator Author

tnull commented Feb 19, 2024

Rebased on main to resolve minor conflict.

@yanganto
Copy link

Hi @tnull
Will this PR be the final version for an LSPS client that will be used in the hackathon in March?
Will it be merged before the event?

@tnull
Copy link
Collaborator Author

tnull commented Feb 20, 2024

Hi @tnull Will this PR be the final version for an LSPS client that will be used in the hackathon in March?

Yes, very likely.

Will it be merged before the event?

I assume it will be merged any day now, once @jkczyz came around to do a final pass.

@tnull tnull added this to the 0.3 milestone Feb 20, 2024
@tnull tnull merged commit 980b14c into lightningdevkit:main Feb 21, 2024
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jkczyz jkczyz jkczyz approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
0.3
Development

Successfully merging this pull request may close these issues.

LSP client integration
3 participants
@tnull @jkczyz @yanganto