Skip to content
This repository was archived by the owner on Jan 6, 2025. It is now read-only.

LSPS0: List Protocols #16

Closed
wants to merge 1 commit into from

Conversation

yanganto
Copy link
Contributor

@yanganto yanganto commented Jul 31, 2023

We need to expose the lsps0.listprotocols method and let it become queriable from others.

In #15, the return of LSPS0Response of handle_request is rejected. And there is no return after handling the lsps0 request.

If I am not misunderstanding, the LSPS0Response should pop up as an Event, and anyone sending the query can acquire the event after the query, and by comparing the RequestId to get the corresponding event for the request.

This PR handles the lsps0 request and creates an event when handling the request, such that we can do an lsps0 query and get the result.

If there is anything I can do to make this RP be merged easier, please let me know.
Thanks. 🙏

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.

If I am not misunderstanding, the LSPS0Response should pop up as an Event, and anyone sending the query can acquire the event after the query, and by comparing the RequestId to get the corresponding event for the request.

Well, this was previously discussed and we decided against exposing all messages as events to the user. Events should only be used to surface events the user actually needs to act on. Network messages should just be dealt with internally as far as possible, the API will become rather convoluted as is already.

As mentioned in the other PR: please feel free to contribute additional unit tests, or, if you really need some interfaces for integration tests, we can discuss exposing them behind a cfg(test) feature flag.

If there is anything I can do to make this RP be merged easier, please let me know. Thanks. 🙏

Mh, tbh., it would help if you first opened issues with feature requests on which we could discuss the way forward before just creating PRs that aim to change integral parts of the API design (as making modules pub or switching object types to your liking).

@@ -55,4 +56,16 @@ impl EventQueue {

/// Event which you should probably take some action in response to.
#[derive(Debug, Clone, PartialEq, Eq)]
pub enum Event {}
pub struct Event {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Note that the event was an enum for a reason: we'll have different event variants (see the other PRs, LSPS2 in particular).

@tnull
Copy link
Collaborator

tnull commented Jul 31, 2023

Closing this as not planned for now and will reach out on Discord to better understand your requirements.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants