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

10.2.1 release preparation #6111

Merged
merged 11 commits into from
Feb 13, 2025
Merged

10.2.1 release preparation #6111

merged 11 commits into from
Feb 13, 2025

Conversation

neilmayhew
Copy link
Contributor

Update CHaP index and cardano-node version

@neilmayhew neilmayhew requested review from a team as code owners February 8, 2025 00:13
@mgmeier
Copy link
Contributor

mgmeier commented Feb 12, 2025

Maybe some context to making ForwardingVersionData an extra argument:
Its definition is just a wrapper newtype ForwardingVersionData = ForwardingVersionData{networkMagic :: NetworkMagic}. This means that no function who has the networkId in scope will ever need ForwardingVersionData as a seperate argument.

EDIT: If this is motivated by any change in simpleSingletonVersions - would it be possible to change such that we see
[\_ ->] SomeResponderApplication $ const $ forwarderApp [...]? Which would describe IMHO much better what's actually going on?

Copy link
Contributor

@carbolymer carbolymer left a comment

Choose a reason for hiding this comment

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

👍🏻 on chairman, submit-api and testnet

@neilmayhew
Copy link
Contributor Author

@crocodile-dentist @coot:

The review feedback here is on changes that came from your PRs (#6107 #6114) that were targeting release/10.2.x. I merged those and cherry-picked them onto this branch because we wanted to release from master and it's not possible to rebase a release branch because of branch protections.

Feel free to push changes to this branch, or to create a new PR targeting master for your previous PRs. I'll then rebase this branch onto master after they're merged.

@coot
Copy link
Contributor

coot commented Feb 12, 2025

@mgmeier ouroboros-network-framework's simpleSingletonsVersions function takes now a callback (the vdata -> r in the link above) with an argument - the negotiated version data. This allows to customise the application based on negotiated data.

@mgmeier
Copy link
Contributor

mgmeier commented Feb 12, 2025

@coot Thanks for confirming this is indeed motivated by simpleSingletonVersions. As I pointed out, the forwarding application does not require the extra complexity of making behaviour dependent on negotiated version data. Moreover, the way version data definition is currently implemented, it's conceptually impossible to do so - which makes sense, at this has not been an option in the past.

I'd like the code to be very discriptive and reflect that situation, ideally also in its type signatures. I'll add a commit to make the forwarderApp a constant function.

Copy link
Contributor

@mgmeier mgmeier left a comment

Choose a reason for hiding this comment

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

Adressed my concerns in an additional commit.

@neilmayhew neilmayhew enabled auto-merge February 12, 2025 17:23
@neilmayhew neilmayhew added this pull request to the merge queue Feb 12, 2025
Merged via the queue into master with commit 52b708f Feb 13, 2025
25 of 27 checks passed
@neilmayhew neilmayhew deleted the nm/10.2.1 branch February 13, 2025 01: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.

5 participants