Combine connect methods

wuan commented 1 year ago

Description

This PR replaces the functions

    pub async fn connect_without_code(
        config: AppConfig<impl serde::Serialize + Send + Sync + 'static>,
        code_length: usize,
    ) -> Result<
        (
            WormholeWelcome,
            impl std::future::Future<Output = Result<Self, WormholeError>>,
        ),
        WormholeError,
    >

and

pub async fn connect_with_code(
        config: AppConfig<impl serde::Serialize + Send + Sync + 'static>,
        code: Code,
        expect_claimed_nameplate: bool,
    ) -> Result<(WormholeWelcome, Self), WormholeError>

by separating the setup of the connection to the mailbox from the wormhole connection setup in two steps:

Create a MailboxConnection (with at least two variants):

async MailboxConnection::create(
    config: AppConfig<V>,
    code_length: usize
) -> Result<Self, WormholeError>

or

async MailboxConnection::connect(
    config: AppConfig<V>,
    code: Code,
    allocate: bool,
) -> Result<Self, WormholeError>

Create the Wormhole connection using the MailboxConnection:

async Wormhole::connect(
    mailbox_connection: MailboxConnection<impl serde::Serialize + Send + Sync + 'static>,
) -> Result<Self, WormholeError>

This has the advantage that setting up the initial connection and connecting to an existing mailbox have similar code.

Waiting for the nameplate (within the generated code) from the initial connection has been made explicit.

Examples

The new connect(...) function can be used with or without code:

    let mailbox_connection = MailboxConnection::create(app_config, 2).await?;
    <show welcome message and show or transfer code>
    let wormhole = Wormhole::connect(mailbox_connection).await?;

or

    let mailbox_connection = MailboxConnection::connect(app_config, code, false).await?;
    <show welcome message>
    let wormhole = Wormhole::connect(mailbox_connection).await?;

codecov[bot] commented 1 year ago

Codecov Report

Patch coverage: 67.56% and project coverage change: +0.47 :tada:

Comparison is base (e76d46a) 43.74% compared to head (0228052) 44.21%.

:exclamation: Current head 0228052 differs from pull request most recent head 187f585. Consider uploading reports for the commit 187f585 to get more accurate results

Additional details and impacted files

```diff @@ Coverage Diff @@ ## master #195 +/- ## ========================================== + Coverage 43.74% 44.21% +0.47% ========================================== Files 18 18 Lines 2549 2576 +27 ========================================== + Hits 1115 1139 +24 - Misses 1434 1437 +3 ``` | [Impacted Files](https://app.codecov.io/gh/magic-wormhole/magic-wormhole.rs/pull/195?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=magic-wormhole) | Coverage Δ | | |---|---|---| | [cli/src/main.rs](https://app.codecov.io/gh/magic-wormhole/magic-wormhole.rs/pull/195?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=magic-wormhole#diff-Y2xpL3NyYy9tYWluLnJz) | `0.00% <0.00%> (ø)` | | | [src/lib.rs](https://app.codecov.io/gh/magic-wormhole/magic-wormhole.rs/pull/195?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=magic-wormhole#diff-c3JjL2xpYi5ycw==) | `40.00% <ø> (ø)` | | | [src/core.rs](https://app.codecov.io/gh/magic-wormhole/magic-wormhole.rs/pull/195?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=magic-wormhole#diff-c3JjL2NvcmUucnM=) | `78.02% <81.96%> (-0.51%)` | :arrow_down: | ... and [7 files with indirect coverage changes](https://app.codecov.io/gh/magic-wormhole/magic-wormhole.rs/pull/195/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=magic-wormhole)

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

piegamesde commented 1 year ago

Judging from the diff of the tests, I think this is a downgrade in API usability. Moreover, importing enum variants is frowned upon in many contexts (although I think it is kind of fine here?).

I think you can still make your deduplication by having the old API methods as shims delegating to the actual generic implementation.

Also keep in mind the use-case of connecting directly to a mailbox without nameplate, even though we don't currently make use of it. This is definitely a feature that will eventually be supported some time in the future.

wuan commented 1 year ago

Judging from the diff of the tests, I think this is a downgrade in API usability. Moreover, importing enum variants is frowned upon in many contexts (although I think it is kind of fine here?).

Having boolean values in APIs is some kind of anti-pattern. Enums help to group parameters for the different use cases and use names for clarification.

I think you can still make your deduplication by having the old API methods as shims delegating to the actual generic implementation.

This is about deduplication and removing boolean flags from argument lists.

Also keep in mind the use-case of connecting directly to a mailbox without nameplate, even though we don't currently make use of it. This is definitely a feature that will eventually be supported some time in the future.

The change should not have removed any use-cases which have existed before. Can you give an example what will not work after the change? Adding other connections variants is as simple as expanding the Enum definition and implementing the details.

piegamesde commented 1 year ago

Having boolean values in APIs is some kind of anti-pattern. Enums help to group parameters for the different use cases and use names for clarification.

I agree in general, but this was not my point. I was referring to having WormholeConnect::GenerateCode etc. in the file's global scope. But then if you don't import it like that the simple WithCode becomes WormholeConnect::WithCode or even magic_wormhole::WormholeConnect::WithCode …

The change should not have removed any use-cases which have existed before. Can you give an example what will not work after the change?

I'm not saying it does, I'm just saying to keep it in mind so that it doesn't.

Let's take a step back and explore some more API ideas that might our current issues with it.

We could do a full-blown builder pattern
- It would require two structs for the type state, and one of them would have a expect_claimed_nameplate() function or something like that
We could have WormholeConnect::connect(self, AppConfig)
- Downside is that the Wormhole struct has no constructor on itself anymore, but that's not uncommon in Rust APIs
- Maybe call it WormholeConnector then

wuan commented 1 year ago

I agree in general, but this was not my point. I was referring to having WormholeConnect::GenerateCode etc. in the file's global scope. But then if you don't import it like that the simple WithCode becomes WormholeConnect::WithCode or even magic_wormhole::WormholeConnect::WithCode …

I do not see a problem here, as it still will be type safe and the user of the library can decide what to do adapting to the use case.

And yes, a builder pattern can be added on top of this any time.

piegamesde commented 1 year ago

Hmm, I am sorry for being nitpicky but I'm still not convinced.

The motivation of this PR is to remove the bool argument in the function signature, which it does and is a good thing.
The PR also claims to remove code duplication by merging two similar functions, but actually it has little effect. If you look at initiate_connect, almost all the relevant business logic is within the match/case distinction, except for a couple of lines of boilerplate code for input and return. But also this is not surprising, as all the duplicated code already got factored out into connect_custom.
The connect_with_code and connect_without_code functions, while being similar, nevertheless have a distinct return type. This is now modeled using connect and initiate_connect functions. But which one of those should be used when? Previously the function signature just took care about that part, now users must think about it.

So overall this feels like a "one step forward one step back" kind of change to me. (Don't get me wrong, the proposed API is okay, but I don't see the value in replacing one okay API with small flaws by another API with other small flaws). I think this could be improved so that it does not have these problems, we could discuss that if desired. But as of now, IMHO this is mainly about the boolean argument flag, and for that there is a minimal solution:

You can give the argument a name by wrapping it in a tuple struct:

/// Wormhole.connect_with_code(config, code, Allocate(false))
#[derive(Clone, Copy, Debug, Default, Eq, PartialEq)]
pub struct Allocate(pub bool);

The Gtk-rs bindings already make use of this pattern to map boolean arguments from the C side, and quite successfully so: https://gtk-rs.org/gtk-rs-core/stable/latest/docs/glib/signal/struct.Inhibit.html https://gtk-rs.org/gtk-rs-core/stable/latest/docs/glib/source/struct.Continue.html

Also I chose Allocate as the name, because if we are touching the expect_claimed_nameplate flag while we are at it I'd like to invert it and call it "allocate", in accordance with the spec proposal. (It is an improvement Meejah came up with, and I really like it so why not also apply it here)

wuan commented 1 year ago

Hmm, I am sorry for being nitpicky but I'm still not convinced.

The motivation of this PR is to remove the bool argument in the function signature, which it does and is a good thing.

The PR also claims to remove code duplication by merging two similar functions, but actually it has little effect. If you look at initiate_connect, almost all the relevant business logic is within the match/case distinction, except for a couple of lines of boilerplate code for input and return. But also this is not surprising, as all the duplicated code already got factored out into connect_custom.

The old code duplicates the function definition, calling RendezvousServer::connect(...) and building up the response. That is already duplication.

The connect_with_code and connect_without_code functions, while being similar, nevertheless have a distinct return type. This is now modeled using connect and initiate_connect functions. But which one of those should be used when? Previously the function signature just took care about that part, now users must think about it.

initiate_connect(...) is only there for the test setup. It should be removed from the public API to avoid confusion. Even worse, the slightly different return type of connect_without_code(...) has been added to a public API only for purpose of internal testing. That is something I would call unexpected and confusing.

So overall this feels like a "one step forward one step back" kind of change to me. (Don't get me wrong, the proposed API is okay, but I don't see the value in replacing one okay API with small flaws by another API with other small flaws). I think this could be improved so that it does not have these problems, we could discuss that if desired. But as of now, IMHO this is mainly about the boolean argument flag, and for that there is a minimal solution:

You can give the argument a name by wrapping it in a tuple struct:
/// Wormhole.connect_with_code(config, code, Allocate(false))
#[derive(Clone, Copy, Debug, Default, Eq, PartialEq)]
pub struct Allocate(pub bool);

I'm still not convinced and I propose to hide initiate_connect(...) from the user. In the end just having a single connect method which can be configured by a type safe parameter (Enums allow grouping of required parameters per use case) seem to be quite promising, especially as there are more ways to connect in the pipeline.

Besides that, I do not see the difference between importing a struct or an Enum.

The Gtk-rs bindings already make use of this pattern to map boolean arguments from the C side, and quite successfully so: https://gtk-rs.org/gtk-rs-core/stable/latest/docs/glib/signal/struct.Inhibit.html https://gtk-rs.org/gtk-rs-core/stable/latest/docs/glib/source/struct.Continue.html

Also I chose Allocate as the name, because if we are touching the expect_claimed_nameplate flag while we are at it I'd like to invert it and call it "allocate", in accordance with the spec proposal. (It is an improvement Meejah came up with, and I really like it so why not also apply it here)

Renaming the boolean flag to "allocate" should be straightforward.

piegamesde commented 1 year ago

initiate_connect(...) is only there for the test setup. It should be removed from the public API to avoid confusion. Even worse, the slightly different return type of connect_without_code(...) has been added to a public API only for purpose of internal testing.

This is news to me, could you please elaborate? Unifying the return types / getting rid of initiate_connect would indeed be an interesting improvement.

Btw if you need a method only for testing, the usual way to handle it is to annotate it with #[cfg(test)].

wuan commented 1 year ago

You are right, that is a general requirement. The basic problem seems to be that we want to access the generated code as a partial result, before we open the wormhole connection, which blocks until the other side enters exactly this code. That is for me the reason why we have that asymmetry in return types.

To overcome this, I now split up creating the mailbox connection and creating the wormhole:

Setting up the wormhole on the code generating side looks like this:

let mailbox_connection = MailboxConnection::create(app_config, code_length).await?;
let code = mailbox_connection.code.clone();
<display/send code>
let wormhole = Wormhole::connect(mailbox_connection).await?;

and on the other side like this:

let mailbox_connection = MailboxConnection::connect(app_config, code, false).await?;
let wormhole = Wormhole::connect(mailbox_connection).await?;

which can even be simplified to the following, if you do not care about the welcome message:

let wormhole = Wormhole::connect_with_code(config, code, false).await?;

Does that help to make the steps more clear? At least now you can do one thing after another.

Names are still "in progress", better ideas are welcome.

wuan commented 1 year ago

Code looking good so far. The doc comments still need adapting though

After having lots of fun with doctests there should be some more doc comments now.

meejah commented 1 year ago

Would being more like the Python API help here?

There, we do not conflate "connecting" or "creating" the Wormhole with the code at all -- instead you must call .set_code() or .allocate_code() (i.e. depending on whether you have one or wish to create one) at some point, before calling .get_code() (all code-paths call this).

This is more like "Builder" I suppose, superficially. But "the wormhole" has state, so there's no real getting around that.

You could also get rid of the bool in the API by declaring "the code" as a Maybe: if it's Nothing then allocate one. (I guess in Rust that's spelled Option and None).

wuan commented 1 year ago

You could also get rid of the bool in the API by declaring "the code" as a Maybe: if it's Nothing then allocate one. (I guess in Rust that's spelled Option and None).

That sounds different. In this case we always have a code. The boolean only determines the behaviour if the nameplate cannot be found: create it (true) or fail (false).

meejah commented 1 year ago

How do you know if the nameplate can be found without trying to claim it?

(Anyway, sorry I thought you were talking about the create_with_password vs. connect, basically)

edit: I see it's depending on list definitely working. That's not going to work in general, as the server doesn't have to answer that query (i.e. deployments can turn it off with --disallow-list).

piegamesde commented 1 year ago

Yeah that is a temporary until we have the allocate flag in the claim command merged

meejah commented 1 year ago

I don't anticipate merging that anytime soon; it breaks the protocol. But, okay, understood.

(edit: oh, wait, that's the one change in that PR that could be done forward/backwards compatible I suppose)

Can you remind me what is the use-case for failing here? (You have a code .. but you sometimes want to fail if the nameplate doesn't already exist?)

wuan commented 1 year ago

I don't anticipate merging that anytime soon; it breaks the protocol. But, okay, understood.

This PR only renames and inverts the semantics of the boolean parameter, it does not change functionality. We can even rename and invert the parameter to have no change at all.

meejah commented 1 year ago

This PR only renames and inverts the semantics [..]

I'm not talking about this PR in the sentence you quote.

wuan commented 1 year ago

I'm not talking about this PR in the sentence you quote.

Then I propose to add a reference to an issue / PR to link this discussion there.

meejah commented 1 year ago

I'm not talking about this PR in the sentence you quote.

Then I propose to add a reference to an issue / PR to link this discussion there.

I don't think it's relevant, but others can link it if they believe it is. I was just trying to clarify here that the PR piegames mentioned (and to which my reply pertained) wasn't this one, as you seemed to believe with your reply.

The only relevance is that the "depend on list working" code in this repository is intended to be a temporary situation.

wuan commented 1 year ago

Thanks, rebased and squashed all commits.

You can also "Allow squash merging" in the General settings to keep history linear.

magic-wormhole / magic-wormhole.rs