Remove orphan ToJSON instances that were moved to ledger
Description
These instances were added to ledger in IntersectMBO/cardano-ledger#4250. This PR shouldn't be merged until node is updated to use the corresponding versions of the ledger packages.
The golden tests pass with the current instances. They can be used to validate the new instances and should then be discarded (in #5776) because they will no longer be testing code that's in node.
Checklist
- [x] Commit sequence broadly makes sense and commits have useful messages
- [x] New tests are added if needed and existing tests are updated. These may include:
- golden tests
- property tests
- roundtrip tests
- integration tests See Runnings tests for more details
- [ ] Any changes are noted in the
CHANGELOG.mdfor affected package - [x] The version bounds in
.cabalfiles are updated - [ ] CI passes. See note on CI. The following CI checks are required:
- [ ] Code is linted with
hlint. See.github/workflows/check-hlint.ymlto get thehlintversion - [ ] Code is formatted with
stylish-haskell. See.github/workflows/stylish-haskell.ymlto get thestylish-haskellversion - [ ] Code builds on Linux, MacOS and Windows for
ghc-8.10.7andghc-9.2.7
- [ ] Code is linted with
- [x] Self-reviewed the diff
Note on CI
If your PR is from a fork, the necessary CI jobs won't trigger automatically for security reasons. You will need to get someone with write privileges. Please contact IOG node developers to do this for you.
cc @Jimbo4350
@neilmayhew> since the tests were not launched on the original instances:
We don't really know if the instances from the ledger behave the same :upside_down_face: Maybe remove the commit removing the local instances until the time when we'll be able to merge this PR?
@neilmayhew> since the tests were not launched on the original instances:
We don't really know if the instances from the ledger behave the same 🙃
We do, because I ran them locally on a detached head. :smile:
Also, my PR for ledger uses identical golden files (copied from this PR).
Maybe remove the commit removing the local instances until the time when we'll be able to merge this PR?
That would be unfortunate, because the point of this PR was to reduce the work needed when the time comes to integrate with a newer ledger, by making it easier to find the instances that need to be removed.
I was assuming that whoever does the integration would do something similar to what I did above, by running the tests manually before and after the integration.
However, perhaps the least confusing thing would be to split this PR into three:
- The golden tests, which would be merged right away.
- Removing the instances, which would wait until integration.
- Removing the golden tests, which would be merged right after the preceding PR (2).
If this is more acceptable to you, I'll create new PRs for (1) and (3) and update this one to become (2).
However, perhaps the least confusing thing would be to split this PR into three:
- The golden tests, which would be merged right away.
- Removing the instances, which would wait until integration.
- Removing the golden tests, which would be merged right after the preceding PR (2).
Yes I think this is the best way, to make sure nothing can go wrong during the integration. This procedures avoids the integrator to have to understand in details what's happening, he will anyway be safe!
So I've approved https://github.com/IntersectMBO/cardano-node/pull/5775 and this one will serve as reference for the integration.
Thanks @neilmayhew :heart:
cc @Lucsanszky
This PR is stale because it has been open 45 days with no activity.
Superseded by recent integration