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

Typed Protocols: new API #1223

Merged
merged 5 commits into from
Oct 21, 2024
Merged

Typed Protocols: new API #1223

merged 5 commits into from
Oct 21, 2024

Conversation

coot
Copy link
Contributor

@coot coot commented Aug 21, 2024

Description

Use typed-protocols-0.3.0.0.

Depends on:

  • Updated to use typed-protocols-0.3.0.0
  • Added KeepAlive tracer

@coot coot self-assigned this Sep 27, 2024
@coot coot marked this pull request as ready for review September 27, 2024 05:21
@coot coot force-pushed the coot/typed-protocols-new-api branch 3 times, most recently from 9c13191 to a65b059 Compare September 28, 2024 08:49
nfrisby
nfrisby previously requested changes Oct 1, 2024
Copy link
Contributor

@nfrisby nfrisby left a comment

Choose a reason for hiding this comment

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

The PR description seems a bit off.
image

  • It's version 0.3.0.0, not version 0.2.0.0.
  • I don't see any aspects of SizeInBytes within the diff itself.

The diff itself looked totally fine. I don't follow the new Stateful stuff yet, but everything here seems very reasonable.

I'm not Approving because of:

  1. The above concerns about the PR description.
  2. I don't think we want an source-repository-package stanza on the main branch. Will the relevant dependencies be uploaded to CHaP soon enough to wait for them?

@coot
Copy link
Contributor Author

coot commented Oct 1, 2024

I started with t-p-0.2 and ended up with t-p-0.3 😁. The TxSizeInBytes patch is gone from this PR, due to consensus introducing its own sizes in a recent PR.

@crocodile-dentist
Copy link
Contributor

ekg-forward 0.6 which is going into the next node release is incompatible with new typed-protocols 0.3 so lets hold off on merging this PR.

@coot
Copy link
Contributor Author

coot commented Oct 17, 2024

@crocodile-dentist ekg-forward is not a dependency of ouroboros-consensus; I have a draft PR for it as well (as for all other cardano-node dependencies 😃).

I also left TODO notes to add PeerSharing tracer.
@coot coot force-pushed the coot/typed-protocols-new-api branch 3 times, most recently from 518a441 to cb6fb68 Compare October 21, 2024 08:30
`NodeToClientV_19` was added.
@coot coot force-pushed the coot/typed-protocols-new-api branch from cb6fb68 to c4b3116 Compare October 21, 2024 08:47
@crocodile-dentist
Copy link
Contributor

@crocodile-dentist ekg-forward is not a dependency of ouroboros-consensus; I have a draft PR for it as well (as for all other cardano-node dependencies 😃).

Yes, but it would have caused problems downstream when releasing node - consensus would have to release off branch to make it go smoothest.

@coot coot force-pushed the coot/typed-protocols-new-api branch from c4b3116 to 254aa51 Compare October 21, 2024 11:21
@coot coot force-pushed the coot/typed-protocols-new-api branch from be7c0f2 to 120d92d Compare October 21, 2024 17:48
@coot coot added this pull request to the merge queue Oct 21, 2024
Merged via the queue into main with commit 7dd936f Oct 21, 2024
14 checks passed
@coot coot deleted the coot/typed-protocols-new-api branch October 21, 2024 18:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants