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

[ADP-3350] Conversion to/from Read.ChainPoint #4540

Merged
merged 2 commits into from
Apr 16, 2024

Conversation

HeinrichApfelmus
Copy link
Contributor

This pull request adds conversion function to/from the data type ChainPoint that belongs to the Cardano.Wallet.Read hierarchy.

This includes

  • Conversions to/from legacy types in primitive
  • Conversions to/from types in ouroboros-consensus for the networking layer

Comments

  • The goal is to eventually remove the old primitive types. However, we add conversions Read.ChainPoint ⟷ ChainPoint here in order to have more control over the source code impact of future pull requests. Specifically, we can then change the types in NetworkLayer from ChainPoint to Read.ChainPoint while touching only a small portion of the code in Cardano.Wallet thanks to a cleverly inserted conversion.

Issue Number

ADP-3350

Copy link
Contributor

@paweljakubas paweljakubas left a comment

Choose a reason for hiding this comment

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

lgtm!
Some food for thought ... Should we at some point in future think about adding roundtrips checking

fromSomething . toSomething `isEquivalentTo` id

? I would be probably in favor of this .

@HeinrichApfelmus
Copy link
Contributor Author

HeinrichApfelmus commented Apr 16, 2024

Thanks for the review! 🙏

Some food for thought ... Should we at some point in future think about adding roundtrips checking

Ah. Fair enough. I consider this particular code to be "obviously correct", and the type checker imposes strong constraints on what it can do, but there is always undefined or typos. Testing the roundtrip ouroboros-consensusChainPoint may be a sensible idea, whereas I would skip it for the Read.ChainPointChainPoint as it involves a legacy type.

@HeinrichApfelmus HeinrichApfelmus added this pull request to the merge queue Apr 16, 2024
Merged via the queue into master with commit 352ed60 Apr 16, 2024
3 checks passed
@HeinrichApfelmus HeinrichApfelmus deleted the HeinrichApfelmus/ADP-3350/convert-chainpoint branch April 16, 2024 13:50
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.

None yet

2 participants