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

feat(miniprotocols): Implement tx submission client #220

Merged
merged 9 commits into from
Feb 4, 2023

Conversation

scarmuega
Copy link
Member

No description provided.

@scarmuega scarmuega requested a review from jmhrpr February 1, 2023 22:04
Also includes some documentation for how to use both the client and the server
@@ -0,0 +1,194 @@
# TxSubmission
Copy link
Collaborator

Choose a reason for hiding this comment

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

}

pub fn wait_for_init(&mut self) -> Result<(), Error> {
if self.0 != State::Init {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Was wondering if I should put some kind of loop here, in case we're waiting for a connection or something, but kept it very minimal;

If there's interest, I could make a recv_init that does the dumb thing, and a wait_for_init that loops until someone connects and sends the init message.

Copy link
Member Author

Choose a reason for hiding this comment

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

well, the recv_message blocks until there's something to read from the channel, the "wait" is imposed to the consumer, even if they don't want it.

Ideally, every blocking call should provide an optional timeout, so that the control flow logic can escape the blocking call if desired. This is an improvement we should make at the multiplexer level, there should be a way to read from a channel with a timeout.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Right but if someone hasn't even connected yet this errors immediately; or is there a construct at the multiplexer / channel level that lets me wait for a connection?

Copy link
Member Author

@scarmuega scarmuega Feb 3, 2023

Choose a reason for hiding this comment

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

I see. I think that our server agent should be created as a result of a successful bearer connection (aka: tcp or unix socket connection).

I imagine the root process waiting for connections on a specific port, this would be a blocking call that is handled by the OS (something like TcpListener::bind("127.0.0.1:3000")). Upon each connection, the process would create a new agent passing a channel connected to our new bearer (inbound socket stream). By this point in time, the calls to recv_message should not fail, but rather block waiting for data.

Summarizing, I think that the initial loop / block is out-of-scope for this crate. I say we create a new example crate (different PR) that shows how to connect our agent to a tcp listener. What do you think?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Makes sense to me; there's nothing in the type system that currently enforces this, though; i.e. I would expect something guiding me to "hey, you opened a connection to the relay, not the other way around, so you can't use this socket/channel to create a server"; see the integration test I added for example.

Is it worth adding a ClientChannel and ServerChannel type to distinguish this?

@Quantumplation
Copy link
Collaborator

clippy pls

@Quantumplation Quantumplation force-pushed the feat/txsubmit branch 2 times, most recently from 8966f69 to 3337e8c Compare February 3, 2023 15:25
@scarmuega
Copy link
Member Author

@Quantumplation I think this is ready to merge. We can implement the improvements we discussed as independent PRs. Can you please give it a final review?

@Quantumplation
Copy link
Collaborator

@scarmuega LGTM!

@Quantumplation Quantumplation merged commit 16d0211 into main Feb 4, 2023
@scarmuega scarmuega deleted the feat/txsubmit branch February 4, 2023 11:36
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