cardano-node icon indicating copy to clipboard operation
cardano-node copied to clipboard

Remove orphan ToJSON instances that were moved to ledger

Open neilmayhew opened this issue 1 year ago • 5 comments

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.md for affected package
  • [x] The version bounds in .cabal files are updated
  • [ ] CI passes. See note on CI. The following CI checks are required:
    • [ ] Code is linted with hlint. See .github/workflows/check-hlint.yml to get the hlint version
    • [ ] Code is formatted with stylish-haskell. See .github/workflows/stylish-haskell.yml to get the stylish-haskell version
    • [ ] Code builds on Linux, MacOS and Windows for ghc-8.10.7 and ghc-9.2.7
  • [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.

neilmayhew avatar Apr 09 '24 23:04 neilmayhew

cc @Jimbo4350

neilmayhew avatar Apr 10 '24 00:04 neilmayhew

@neilmayhew> since the tests were not launched on the original instances:

image

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?

smelc avatar Apr 10 '24 08:04 smelc

@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:

  1. The golden tests, which would be merged right away.
  2. Removing the instances, which would wait until integration.
  3. 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).

neilmayhew avatar Apr 10 '24 14:04 neilmayhew

However, perhaps the least confusing thing would be to split this PR into three:

  1. The golden tests, which would be merged right away.
  2. Removing the instances, which would wait until integration.
  3. 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:

smelc avatar Apr 11 '24 09:04 smelc

cc @Lucsanszky

neilmayhew avatar Apr 11 '24 13:04 neilmayhew

This PR is stale because it has been open 45 days with no activity.

github-actions[bot] avatar May 27 '24 01:05 github-actions[bot]

Superseded by recent integration

neilmayhew avatar May 28 '24 13:05 neilmayhew