Skip to content

WIP: sketch out interfaces for working with vat networks #452

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

Merged
merged 14 commits into from
Feb 25, 2023

Conversation

zenhack
Copy link
Contributor

@zenhack zenhack commented Feb 15, 2023

Similar to the VatNetwork interfaces described at the bottom of rpc.capnp; this patch introduces a sketch of what these interfaces might look like in Go, plus the beginning an in-memory implementation to use in tests. Needs testing & docs, and of course isn't actually used by anything.

@zenhack zenhack marked this pull request as draft February 15, 2023 05:37
Similar to the VatNetwork interfaces described at the bottom of
rpc.capnp; this patch introduces a sketch of what these interfaces might
look like in Go, plus the beginning an in-memory implementation to use
in tests. Needs testing & docs, and of course isn't actually used by
anything.
@zenhack
Copy link
Contributor Author

zenhack commented Feb 15, 2023

Note this is using the term "Peer" instead of "Vat" in the API, because the latter is really a unit of concurrency, not a node in the network. See ocapn/ocapn#18

@lthibault
Copy link
Collaborator

I like this. I've stared at that spec a few times, and have often wanted an implementation to play around with.

One thing I'm still not clear about, though, is what bearing this has (if any) on how applications interact with the capnp library. Are application developers going to need to provide their own implementation of these interfaces in order to support 3PH? If so, I would appreciate a quick sketch of how this interacts with the existing rpc.Conn API. I'm sure it's simple enough, but it just hasn't clicked yet.

@zenhack
Copy link
Contributor Author

zenhack commented Feb 15, 2023 via email

@zenhack
Copy link
Contributor Author

zenhack commented Feb 15, 2023

(update, went ahead and documented those fields).

@zenhack
Copy link
Contributor Author

zenhack commented Feb 15, 2023 via email

Since we've renamed PeerId -> RemotePeerId in Options, we need to do it
here as well.
Per Louis's request.
...to match the one for Dial()
@zenhack
Copy link
Contributor Author

zenhack commented Feb 15, 2023

Added a bunch of docs.

Many of these are copied from rpc.capnp, possibly slightly modified.
@lthibault
Copy link
Collaborator

I considered that, but we're going to want to store these inside a Conn,
so we're going to end up needing to hide the type anyway. I suppose we
could make the PeerId wrapper private though? But then it's still going
to propagate into Options, so you'd have to specify a type for that even
when doing point-to-point connections...

Ah ok, that makes sense.

Partly as an intellectual exercise, and partly in case there's something there: the options type is consumed internally by rpc.Conn, if I'm not mistaken. What I mean is that none of Conns methods will return a parametrized type (right?).

If that is indeed the case, it seems like we could get away with defining Conn to be an interface that is satsified by the parametrized (and possibly private) conn-struct.

I have a hunch that it will be helpful to enforce separation of vat-networks based on type. Maybe I'm overthinking this, though.

@zenhack
Copy link
Contributor Author

zenhack commented Feb 16, 2023

#348, re-running.

@zenhack
Copy link
Contributor Author

zenhack commented Feb 25, 2023

That feels overly complex. I kindof suspect applications that use more than one Network will be relatively rare, and also I see various problems with things like importClient which hold a pointer to the connection; you'd have to again propagate the type parameter... it just doesn't seem worth it to head off mixing up peer IDs.

I'm not 100% happy with this; I think there are tweaks I'll want to make to the interface, but it's at the point where it's getting hard to predict exactly what they will be before diving in and actually trying to implement the logic this is supposed to support -- what do you say to merging this as-is with the understanding that it will probably be revised a few times?

@zenhack zenhack marked this pull request as ready for review February 25, 2023 21:45
Copy link
Collaborator

@lthibault lthibault left a comment

Choose a reason for hiding this comment

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

Yeah, that sounds good.

Code looks good overall, modulo a small naming nit.

@zenhack zenhack requested a review from lthibault February 25, 2023 21:54
@zenhack zenhack merged commit b11cc56 into capnproto:main Feb 25, 2023
@zenhack zenhack deleted the network-sketch branch February 25, 2023 22:23
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