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.

Sign up for GitHub

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

Jump to bottom

Replace NominalDiffTime with a newtype wrapper. #3208

Merged
merged 3 commits into from
Dec 15, 2022
Merged

Replace NominalDiffTime with a newtype wrapper. #3208

merged 3 commits into from
Dec 15, 2022

Conversation

aniketd
Copy link
Contributor

@aniketd aniketd commented Dec 12, 2022 •
edited
Loading

Description

Remove CBOR instances of NominalDiffTime and only keep those for the newtype wrapper. This is because we don't use full precision during serialisation.

Closes #3185

Checklist

  • Commit sequence broadly makes sense
  • Commits have useful messages
  • New tests are added if needed and existing tests are updated
  • Any changes are noted in the changelog
  • Code is formatted with ormolu (which can be run with scripts/ormolise.sh
  • Self-reviewed the diff

@aniketd aniketd force-pushed the aniketd/nominaldifftimerounded branch from 92c6dd8 to d66e89e Compare December 12, 2022 18:41
@aniketd aniketd marked this pull request as ready for review December 12, 2022 18:45
lehins
lehins requested changes Dec 12, 2022
View reviewed changes
Copy link
Collaborator

@lehins lehins left a comment

Choose a reason for hiding this comment

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

This looks great, but I just realized we can do even better. Let me know if you have questions about this

eras/shelley/impl/src/Cardano/Ledger/Shelley/Genesis.hs Outdated Show resolved Hide resolved
JaredCorduan
JaredCorduan approved these changes Dec 13, 2022
View reviewed changes
Copy link
Contributor

@JaredCorduan JaredCorduan left a comment

Choose a reason for hiding this comment

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

looks great to me, thank you @aniketd !

@aniketd aniketd force-pushed the aniketd/nominaldifftimerounded branch from 06b5aa3 to 52d28e4 Compare December 13, 2022 14:49
@aniketd aniketd requested a review from lehins December 13, 2022 14:50
lehins
lehins reviewed Dec 13, 2022
View reviewed changes
CHANGELOG.md Outdated Show resolved Hide resolved
@aniketd aniketd force-pushed the aniketd/nominaldifftimerounded branch 3 times, most recently from 0a212f8 to f001adb Compare December 14, 2022 16:29
@aniketd aniketd requested a review from lehins December 14, 2022 16:35
lehins
lehins requested changes Dec 14, 2022
View reviewed changes
Copy link
Collaborator

@lehins lehins left a comment

Choose a reason for hiding this comment

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

Looks good. Deserializer is a bit borked, but there is a really easy fix for it

eras/shelley/impl/src/Cardano/Ledger/Shelley/Genesis.hs Outdated Show resolved Hide resolved
eras/shelley/impl/src/Cardano/Ledger/Shelley/Genesis.hs Outdated Show resolved Hide resolved
eras/shelley/impl/src/Cardano/Ledger/Shelley/Genesis.hs Outdated Show resolved Hide resolved
eras/shelley/impl/src/Cardano/Ledger/Shelley/Genesis.hs Outdated Show resolved Hide resolved
eras/shelley/impl/src/Cardano/Ledger/Shelley/Genesis.hs Outdated Show resolved Hide resolved
eras/shelley/test-suite/src/Test/Cardano/Ledger/Shelley/Serialisation/EraIndepGenerators.hs Outdated Show resolved Hide resolved
@aniketd aniketd force-pushed the aniketd/nominaldifftimerounded branch from eaf8447 to 8117479 Compare December 15, 2022 08:46
@aniketd aniketd requested a review from lehins December 15, 2022 08:46
@aniketd aniketd force-pushed the aniketd/nominaldifftimerounded branch from 8117479 to dc7a9d9 Compare December 15, 2022 08:46
lehins
lehins approved these changes Dec 15, 2022
View reviewed changes
Copy link
Collaborator

@lehins lehins left a comment

Choose a reason for hiding this comment

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

Looks great. Just a few final comments.

eras/shelley/impl/src/Cardano/Ledger/Shelley/Genesis.hs Outdated Show resolved Hide resolved
libs/cardano-ledger-core/src/Cardano/Ledger/Orphans.hs Outdated Show resolved Hide resolved
libs/cardano-ledger-core/src/Cardano/Ledger/Orphans.hs Outdated Show resolved Hide resolved
libs/cardano-ledger-core/src/Cardano/Ledger/Orphans.hs Outdated Show resolved Hide resolved
@aniketd aniketd force-pushed the aniketd/nominaldifftimerounded branch from 6b044ee to fbaddaf Compare December 15, 2022 14:25
@aniketd
Replace NominalDiffTime with a newtype wrapper.
bcc8759 
Remove CBOR instances of NominalDiffTime and only keep those for
the newtype wrapper. This is because we don't use full precision
during serialisation.
@aniketd
Address review comments
3ccec91
@aniketd
Address more review comments and learn some
a9a766e
@aniketd aniketd force-pushed the aniketd/nominaldifftimerounded branch from fbaddaf to a9a766e Compare December 15, 2022 16:54
@aniketd aniketd merged commit 40dab63 into master Dec 15, 2022
@iohk-bors iohk-bors bot deleted the aniketd/nominaldifftimerounded branch December 15, 2022 19:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JaredCorduan JaredCorduan JaredCorduan approved these changes

@lehins lehins lehins approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

CBOR serialization of NominalDiffTime is broken
3 participants
@aniketd @JaredCorduan @lehins