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

LSPS0 message handling #4

Merged
merged 1 commit into from
Jun 15, 2023
Merged

Conversation

johncantrell97
Copy link
Contributor

Implements LSPS0 message handling and lays groundwork for other LSPS protocol handling.

.github/workflows/build.yml Outdated Show resolved Hide resolved
src/transport/message_handler.rs Outdated Show resolved Hide resolved
src/transport/message_handler.rs Outdated Show resolved Hide resolved
src/transport/message_handler.rs Outdated Show resolved Hide resolved
src/transport/msgs.rs Outdated Show resolved Hide resolved
src/transport/msgs.rs Outdated Show resolved Hide resolved
src/transport/msgs.rs Outdated Show resolved Hide resolved
src/transport/msgs.rs Outdated Show resolved Hide resolved
src/transport/msgs.rs Show resolved Hide resolved
@johncantrell97
Copy link
Contributor Author

Ok, addressed all of your comments and updated CI to run against 1.48.0 successfully.

@johncantrell97 johncantrell97 mentioned this pull request May 19, 2023
Copy link
Collaborator

@tnull tnull left a comment

Choose a reason for hiding this comment

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

Sorry for the delay here. Generally it's great that we're separating layers now, but I'm still a bit confused about the current design.

In particular I'm not entirely sure if there is no way around splitting TransportMessageHandler and ProtocolMessageHandler? Also, all of this needs docs, which would make it a lot clearer and would also help think about it further.

src/lib.rs Show resolved Hide resolved
.github/workflows/build.yml Show resolved Hide resolved
src/transport/mod.rs Show resolved Hide resolved
src/transport/message_handler.rs Outdated Show resolved Hide resolved
src/transport/message_handler.rs Show resolved Hide resolved
src/transport/msgs.rs Show resolved Hide resolved
src/transport/msgs.rs Outdated Show resolved Hide resolved
src/transport/msgs.rs Outdated Show resolved Hide resolved
src/transport/protocol.rs Outdated Show resolved Hide resolved
src/transport/protocol.rs Outdated Show resolved Hide resolved
src/transport/protocol.rs Outdated Show resolved Hide resolved
Cargo.toml Show resolved Hide resolved
.github/workflows/build.yml Outdated Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
src/events.rs Outdated Show resolved Hide resolved
src/events.rs Outdated Show resolved Hide resolved
src/events.rs Outdated Show resolved Hide resolved
src/transport/message_handler.rs Outdated Show resolved Hide resolved
src/transport/message_handler.rs Outdated Show resolved Hide resolved
src/transport/message_handler.rs Outdated Show resolved Hide resolved
src/transport/protocol.rs Outdated Show resolved Hide resolved
src/transport/protocol.rs Show resolved Hide resolved
@johncantrell97
Copy link
Contributor Author

I made the minor nit doc changes and linked where made sense. I added a little bit more information to LSPManager around expected usage. The problem is that in this version there's really no real use without any of the protocols added. There's no events or even public api to use.

The only outstanding "issue" imo is naming. The only exposed objects are the LSPConfig and LSPManager. "LSP" typically refers to the server, the one doing the providing. I think you said the spec uses "lsp" and "client".

The challenge is currently there is a singular config/'manager' for both use cases. I don't think the LSP prefixed words are the worst but am open to suggestions if you have some.

I could see maybe moving to using a word like Liquidity instead of LSP but I think an "LSP" encompasses more than just liquidity.

@tnull
Copy link
Collaborator

tnull commented Jun 14, 2023

I think we need to incorporate this fix also to unbreak CI: lightningdevkit/rust-lightning#2353

Copy link
Collaborator

@tnull tnull left a comment

Choose a reason for hiding this comment

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

LGTM for now, let us land this and address anything else in follow-ups.

Please squash!

CustomMessageHandler that implements the transport layer
to be used as basis for future LSP protocol implementations
@tnull tnull merged commit aaf2fcb into lightningdevkit:main Jun 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants