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

TxSubmission2 protocol #2807

Merged
merged 8 commits into from
Dec 20, 2020
Merged

TxSubmission2 protocol #2807

merged 8 commits into from
Dec 20, 2020

Conversation

coot
Copy link
Contributor

@coot coot commented Dec 11, 2020

This series of patches introduce 'Hello' protocol transformer. It is included
under Protocol.Trans module name space (similar to monad transformers). The
TxSubmission2 protocol is TxSubmission wrapped in Hello transformer.

TxSubmission2 protocol will be used from version NodeToNodeV_6.

  • typed-protocols: fmapPeerPipelined

  • ouroboros-network: Hello protocol wrapper

  • tx-submission: expose more codec functions

  • TxSubmission2: protocol

  • TxSubmission2: tests

  • tx-submission: missing case in id codec

    This patch fixes a bug in codecTxSubmissionId, it also adds a test which
    would discoverit. KMsgThx is not used anywhere hance the bug wasn't
    discovered.

  • TxSubmission2: use in node-to-node v6 protocol

@coot coot added networking tx-submission Issues related to tx-submission protocol labels Dec 11, 2020
@coot coot requested a review from dcoutts December 11, 2020 09:25
@coot coot force-pushed the coot/hello-protocol-wrapper branch from 57a0917 to cfacdd8 Compare December 11, 2020 09:32
@coot coot force-pushed the coot/hello-protocol-wrapper branch from cfacdd8 to 6f4abf2 Compare December 11, 2020 09:55
Copy link
Contributor

@dcoutts dcoutts left a comment

Choose a reason for hiding this comment

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

Mostly just name suggestions.

But also an important one about the encoding for the hello codec.

@@ -16,6 +16,7 @@ module Network.TypedProtocol.Pipelined
, Nat (Zero, Succ)
, natToInt
, unsafeIntToNat
, fmapPeerPipelined
Copy link
Contributor

Choose a reason for hiding this comment

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

If we're adding something like this, we should do it consistently for the non-pipelined case too.

We want to publish this package too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, so you'd like to export an id function? ;)

fmapPeer :: (Peer ps  pr st  m a -> Peer ps' pr st' m b)
         -> Peer ps  pr st  m a
         -> Peer ps' pr st' m b
fmapPeer f = f

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But what we can do is to add 'Functor' instances to both 'Peer' and 'PeerPipelined'.

Copy link
Contributor

Choose a reason for hiding this comment

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

Fair point, that wouldn't make sense. If we can do Functor instances though then yes we should do that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

An Applicative instance be useful too. It would give poor man's multiplexer in IO, but a much nicer one using Concurrently wrapper from async. But that's for another PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, writing pure :: a -> m a requires to know which state is the final one, some protocols might not even have a single final state.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also ap is only possible for some protocols. it's type signature is similar to connect.

@coot coot force-pushed the coot/hello-protocol-wrapper branch from 6f4abf2 to 42105ea Compare December 11, 2020 16:08
@coot coot requested a review from dcoutts December 11, 2020 16:22
@coot coot dismissed dcoutts’s stale review December 11, 2020 16:28

The comment was addressed.

@coot coot force-pushed the coot/hello-protocol-wrapper branch from 42105ea to 07733ba Compare December 11, 2020 16:28
Protocol transformer which transforms initial agency.
This will allow to define the codec of TxSubmission2 protocol the using
TxSubmission codec.
This patch provides a type alias for tx-submission protocol wrapped
using the Hello protocol transformer.  Codec and protocol limits are
provided also in terms of the original codec / protocol limits.
By exposing some of TxSubmission test utilities this patch provides
a test suite for TxSubmission2 protocol.
@coot coot force-pushed the coot/hello-protocol-wrapper branch from 07733ba to c2d6245 Compare December 11, 2020 17:09
Copy link
Contributor

@dcoutts dcoutts left a comment

Choose a reason for hiding this comment

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

LGTM!

@@ -16,6 +16,7 @@ module Network.TypedProtocol.Pipelined
, Nat (Zero, Succ)
, natToInt
, unsafeIntToNat
, fmapPeerPipelined
Copy link
Contributor

Choose a reason for hiding this comment

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

Fair point, that wouldn't make sense. If we can do Functor instances though then yes we should do that.

-> Message (Hello ps stIdle) st st'
-> CBOR.Encoding
encodeHello (ClientAgency TokHello) MsgHello =
encodeListLen 1 <> encodeWord helloTag
Copy link
Contributor

Choose a reason for hiding this comment

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

Or we could generalise over the whole hello Encoding, not just a Word. Doesn't matter at the moment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately not, it would need to go through Codec.Cbor.Term.Term or something like that: the decoder is in CPS style.

@coot
Copy link
Contributor Author

coot commented Dec 20, 2020

bors merge

@iohk-bors
Copy link
Contributor

iohk-bors bot commented Dec 20, 2020

@iohk-bors iohk-bors bot merged commit 1c4b3c7 into master Dec 20, 2020
@iohk-bors iohk-bors bot deleted the coot/hello-protocol-wrapper branch December 20, 2020 08:16
mrBliss added a commit to IntersectMBO/cardano-node that referenced this pull request Jan 12, 2021
Note that this does not incorporate the latest changes in cardano-ledger-specs.

Notable changes:
* IntersectMBO/ouroboros-network#2807
* IntersectMBO/ouroboros-network#2811
* IntersectMBO/ouroboros-network#2832
mrBliss added a commit to IntersectMBO/cardano-node that referenced this pull request Jan 12, 2021
Note that this does not incorporate the latest changes in cardano-ledger-specs.

Notable changes:
* IntersectMBO/ouroboros-network#2807
* IntersectMBO/ouroboros-network#2811
* IntersectMBO/ouroboros-network#2832
coot pushed a commit to IntersectMBO/cardano-node that referenced this pull request Jan 13, 2021
Note that this does not incorporate the latest changes in cardano-ledger-specs.

Notable changes:
* IntersectMBO/ouroboros-network#2807
* IntersectMBO/ouroboros-network#2811
* IntersectMBO/ouroboros-network#2832

network tracers: moved ToObject TxSubmission instance

network tracers: added ToObjcet TxSubmission2 instance
erikd pushed a commit to IntersectMBO/cardano-node that referenced this pull request Jan 17, 2021
Note that this does not incorporate the latest changes in cardano-ledger-specs.

Notable changes:
* IntersectMBO/ouroboros-network#2807
* IntersectMBO/ouroboros-network#2811
* IntersectMBO/ouroboros-network#2832

network tracers: moved ToObject TxSubmission instance

network tracers: added ToObjcet TxSubmission2 instance

Update cabal.project index-state value
erikd pushed a commit to IntersectMBO/cardano-node that referenced this pull request Jan 17, 2021
Note that this does not incorporate the latest changes in cardano-ledger-specs.

Notable changes:
* IntersectMBO/ouroboros-network#2807
* IntersectMBO/ouroboros-network#2811
* IntersectMBO/ouroboros-network#2832

network tracers: moved ToObject TxSubmission instance

network tracers: added ToObjcet TxSubmission2 instance

Update cabal.project index-state value
erikd pushed a commit to IntersectMBO/cardano-node that referenced this pull request Jan 17, 2021
Note that this does not incorporate the latest changes in cardano-ledger-specs.

Notable changes:
* IntersectMBO/ouroboros-network#2807
* IntersectMBO/ouroboros-network#2811
* IntersectMBO/ouroboros-network#2832

network tracers: moved ToObject TxSubmission instance

network tracers: added ToObjcet TxSubmission2 instance

Update cabal.project index-state value
erikd pushed a commit to IntersectMBO/cardano-node that referenced this pull request Jan 17, 2021
Note that this does not incorporate the latest changes in cardano-ledger-specs.

Notable changes:
* IntersectMBO/ouroboros-network#2807
* IntersectMBO/ouroboros-network#2811
* IntersectMBO/ouroboros-network#2832

network tracers: moved ToObject TxSubmission instance

network tracers: added ToObjcet TxSubmission2 instance

Update cabal.project index-state value
erikd pushed a commit to IntersectMBO/cardano-node that referenced this pull request Jan 18, 2021
Note that this does not incorporate the latest changes in cardano-ledger-specs.

Notable changes:
* IntersectMBO/ouroboros-network#2807
* IntersectMBO/ouroboros-network#2811
* IntersectMBO/ouroboros-network#2832

network tracers: moved ToObject TxSubmission instance

network tracers: added ToObjcet TxSubmission2 instance

Update cabal.project index-state value
erikd pushed a commit to IntersectMBO/cardano-node that referenced this pull request Jan 18, 2021
Note that this does not incorporate the latest changes in cardano-ledger-specs.

Notable changes:
* IntersectMBO/ouroboros-network#2807
* IntersectMBO/ouroboros-network#2811
* IntersectMBO/ouroboros-network#2832

network tracers: moved ToObject TxSubmission instance

network tracers: added ToObjcet TxSubmission2 instance

Update cabal.project index-state value
erikd pushed a commit to IntersectMBO/cardano-node that referenced this pull request Jan 18, 2021
Note that this does not incorporate the latest changes in cardano-ledger-specs.

Notable changes:
* IntersectMBO/ouroboros-network#2807
* IntersectMBO/ouroboros-network#2811
* IntersectMBO/ouroboros-network#2832

network tracers: moved ToObject TxSubmission instance

network tracers: added ToObjcet TxSubmission2 instance

Update cabal.project index-state value
erikd pushed a commit to IntersectMBO/cardano-node that referenced this pull request Jan 18, 2021
Note that this does not incorporate the latest changes in cardano-ledger-specs.

Notable changes:
* IntersectMBO/ouroboros-network#2807
* IntersectMBO/ouroboros-network#2811
* IntersectMBO/ouroboros-network#2832

network tracers: moved ToObject TxSubmission instance

network tracers: added ToObjcet TxSubmission2 instance

Update cabal.project index-state value
erikd pushed a commit to IntersectMBO/cardano-node that referenced this pull request Jan 18, 2021
Note that this does not incorporate the latest changes in cardano-ledger-specs.

Notable changes:
* IntersectMBO/ouroboros-network#2807
* IntersectMBO/ouroboros-network#2811
* IntersectMBO/ouroboros-network#2832

network tracers: moved ToObject TxSubmission instance

network tracers: added ToObjcet TxSubmission2 instance

Update cabal.project index-state value
erikd pushed a commit to IntersectMBO/cardano-node that referenced this pull request Jan 18, 2021
Note that this does not incorporate the latest changes in cardano-ledger-specs.

Notable changes:
* IntersectMBO/ouroboros-network#2807
* IntersectMBO/ouroboros-network#2811
* IntersectMBO/ouroboros-network#2832

network tracers: moved ToObject TxSubmission instance

network tracers: added ToObjcet TxSubmission2 instance

Update cabal.project index-state value
erikd pushed a commit to IntersectMBO/cardano-node that referenced this pull request Jan 18, 2021
Note that this does not incorporate the latest changes in cardano-ledger-specs.

Notable changes:
* IntersectMBO/ouroboros-network#2807
* IntersectMBO/ouroboros-network#2811
* IntersectMBO/ouroboros-network#2832

network tracers: moved ToObject TxSubmission instance

network tracers: added ToObjcet TxSubmission2 instance

Update cabal.project index-state value
erikd pushed a commit to IntersectMBO/cardano-node that referenced this pull request Jan 18, 2021
Note that this does not incorporate the latest changes in cardano-ledger-specs.

Notable changes:
* IntersectMBO/ouroboros-network#2807
* IntersectMBO/ouroboros-network#2811
* IntersectMBO/ouroboros-network#2832

network tracers: moved ToObject TxSubmission instance

network tracers: added ToObjcet TxSubmission2 instance

Update cabal.project index-state value
erikd pushed a commit to IntersectMBO/cardano-node that referenced this pull request Jan 18, 2021
Note that this does not incorporate the latest changes in cardano-ledger-specs.

Notable changes:
* IntersectMBO/ouroboros-network#2807
* IntersectMBO/ouroboros-network#2811
* IntersectMBO/ouroboros-network#2832

network tracers: moved ToObject TxSubmission instance

network tracers: added ToObjcet TxSubmission2 instance

Update cabal.project index-state value
iohk-bors bot added a commit to IntersectMBO/cardano-node that referenced this pull request Jan 19, 2021
2274: Update dependencies r=kderme a=mrBliss

Note that this does not incorporate the latest changes in cardano-ledger-specs.

Notable changes:
* IntersectMBO/ouroboros-network#2807
* IntersectMBO/ouroboros-network#2811
* IntersectMBO/ouroboros-network#2832

Co-authored-by: Thomas Winant <thomas@well-typed.com>
Co-authored-by: Hamish Mackenzie <Hamish.K.Mackenzie@gmail.com>
newhoggy pushed a commit to IntersectMBO/cardano-api that referenced this pull request May 23, 2023
Note that this does not incorporate the latest changes in cardano-ledger-specs.

Notable changes:
* IntersectMBO/ouroboros-network#2807
* IntersectMBO/ouroboros-network#2811
* IntersectMBO/ouroboros-network#2832

network tracers: moved ToObject TxSubmission instance

network tracers: added ToObjcet TxSubmission2 instance

Update cabal.project index-state value
newhoggy pushed a commit to IntersectMBO/cardano-cli that referenced this pull request May 24, 2023
Note that this does not incorporate the latest changes in cardano-ledger-specs.

Notable changes:
* IntersectMBO/ouroboros-network#2807
* IntersectMBO/ouroboros-network#2811
* IntersectMBO/ouroboros-network#2832

network tracers: moved ToObject TxSubmission instance

network tracers: added ToObjcet TxSubmission2 instance

Update cabal.project index-state value
newhoggy pushed a commit to IntersectMBO/cardano-cli that referenced this pull request May 24, 2023
2274: Update dependencies r=kderme a=mrBliss

Note that this does not incorporate the latest changes in cardano-ledger-specs.

Notable changes:
* IntersectMBO/ouroboros-network#2807
* IntersectMBO/ouroboros-network#2811
* IntersectMBO/ouroboros-network#2832

Co-authored-by: Thomas Winant <thomas@well-typed.com>
Co-authored-by: Hamish Mackenzie <Hamish.K.Mackenzie@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tx-submission Issues related to tx-submission protocol
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants