-
Notifications
You must be signed in to change notification settings - Fork 4
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
New API for typed-protocols #52
Conversation
30e4c45
to
5741f30
Compare
I removed |
516b253
to
cdb6259
Compare
runPeer | ||
:: forall ps (st :: ps) pr failure bytes m a . | ||
:: forall ps (st :: ps) pr failure bytes m a. | ||
(MonadThrow m, Exception failure) | ||
=> Tracer m (TraceSendRecv ps) | ||
-> Codec ps failure m bytes | ||
-> Channel m bytes | ||
-> Peer ps pr st m a | ||
-> m a | ||
-> Peer ps pr 'NonPipelined Z st m a | ||
-> m (a, Maybe bytes) | ||
runPeer tracer codec channel peer = | ||
fst <$> runPeerWithDriver driver peer (startDState driver) | ||
runPeerWithDriver driver peer Nothing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
want to do: IntersectMBO/ouroboros-network#594 here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is in typed-protocols-examples
, it doesn't matter than match, does it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In typed-protocols
we anyway need io-classes
dependency for Monad{STM,Async}
to implement runPipelinedPeerWithDriver
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking more closely, this will require to annotate Driver
with a failure
type, which runPeer
will return, so it will be quite substantial change.
I'd leave it for later, when we also include implementation of Driver.Simple
and Driver.WithLimits
in typed-protocols
. This way we wouldn't pull a dependency on a logging framework - error handling will be up to the user.
cdb6259
to
304b101
Compare
9e162be
to
ea29919
Compare
ea29919
to
f360046
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very clean! I looked through most of it except tests which I only skimmed, the API looks much more polished! :) I just made a some comments, which don't impact the design at all.
-- | A proof that if one side has terminated, then the other side terminated as | ||
-- well. | ||
-- |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here:
forall (pr :: PeerRole) (a :: Agency) (ra :: RelativeAgency).
ra == Relative pr a ->
NobodyHasAgency == Relative (FlipAgency pr) a ->
Relative pr a == NobodyHasAgency && Relative (FlipAgency pr) a == NobodyHasAgency
-> ![SomeF f] -- ^ head | ||
-> ![SomeF f] -- ^ tail | ||
-> SingQueueF f q |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you need SomeF ?
can't you "monomorphise" it to f st st'
since it is what is going to be used for? This would avoid unsafeCoerce
usage
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This whole module is not needed. It's an artifact of the previous approach.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why dont we remove it then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's what I did 😄
f360046
to
c7b34a4
Compare
The new API provides proofs for all protocols, and thus proof obligations are not part of `Protocol` type class but instead an internal detail of the library.
The client / server 'Peer's pattern synonyms make it easier to write a 'Peer'. They automatically provide the 'RelativeAgencyEq' singleton. See "Network.TypedProtocol.PingPong.Client".
The 'IsActiveAgency' type class allows to limit cases in codecs. We don't need to write decoders for inactive states, ``notActiveState` allows to reduce them. Note that the advantage of the new approach to typed-protocols is that we have access to protocol state singleton in the decoder, without distinguishing which side has the agency. This simplifies writing codecs.
There's no need to to pass initial `dstate` it is already passed inside `Driver` record.
* WeHaveAgencyProof * TheyHaveAgencyProof * NobodyHasAgencyProof
Illustrative example how to define a 'Channel' for a socket. The implementation is only given for posix platforms. It is using a non-blocking `recv` foreign call to provide `tryRecv`.
An example which demonstrates that one can pipeline two kinds of requests, and collect all the responses.
We don't provide pipelined stateful peer, as safe API requires tracking of pipelined transitions rather than just the depth of pipelining at the type level of a `Peer`. See the `coot/typed-protocols-rewrite` branch how this can be done.
c7b34a4
to
2fa9364
Compare
^ I rebased the branch on top of recent |
3a117a2
to
9fec582
Compare
This is because stylish-parser cannot parse the export list which starts with ```hs module Network.TypedProtocol.Core ( -- * Introduction -- $intro -- * Defining protocols -- $defining Protocol (..) ``` But this is required by haddock, to include the next section.
9fec582
to
f451040
Compare
New API for typed-protocols
# Description Use `typed-protocols-0.3.0.0`. Depends on: * [x] input-output-hk/typed-protocols#52 * [x] input-output-hk/typed-protocols#61 * [x] IntersectMBO/ouroboros-network#4935 - **Updated to use typed-protocols-0.3.0.0** - **Added KeepAlive tracer**
Takes lessons learned in #3, but preserving original (parallel) pipelining design.
The original pipelining is limited to transitions which can be fulfilled by remote transitions, this could be lifted by the more general scheme of tracking pipelined transition at type level done in #3 in the future. But what's important we run the receiver for pipelined transition in parallel, which preserves performance. Below is the comparison of plain and modified
cardano-node-9.1.0
syncing mainet: