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

Fixup time related instances #373

Merged
merged 2 commits into from
Mar 13, 2023
Merged

Conversation

lehins
Copy link
Collaborator

@lehins lehins commented Feb 24, 2023

To/FromCBOR instance for NominalDiffTime is pretty broken, since there is loss of precision on roundtripping. This PR fixes it by removing the instance altogether in favor of helper functions that can do the same job, but they are more explicit.

Also moving orphan To/FromJSON instances for SystemStart from cardano-node

Adding a couple of other potentially useful instances

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.

thank you @lehins , looks great!

lehins added 2 commits March 13, 2023 21:31
* Remove `To/FromCBOR` instances for `NominalDiffTime`, since they did rounding.  Newly
  added functions `encodeNominalDiffTimeMicro`/`decodedNominalDiffTimeMicro` can be used
  to recover previous behavior. Correct instances that do not perform any rounding will
  be added in some future version, for now `decodeNominalDiffTime` and
  `encodeNominalDiffTime` can be used.
* Add `decodeNominalDiffTime` and `encodeNominalDiffTime`
* Add `To/FromCBOR` for all `Fixed a`, not just `Nano` and `Pico`
* Bump up the version to 1.7.0.0
* Addition of `ToJSON`/`FromJSON` instances for `SystemStart`
* Addition of `ToCBOR`/`FromCBOR` instances for `RelativeTime` and `SlotLength`
@lehins lehins force-pushed the lehins/fixup-time-related-instances branch from a185a0a to 2032bae Compare March 13, 2023 18:31
@lehins lehins merged commit dd15fa6 into master Mar 13, 2023
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.

2 participants