Skip to content

Conversation

danbaruka
Copy link

… cleanup Alonzo/Conway logic - 2025-07-01 07:33:54

Refactor transValidityInterval to use era-specific logic: implement Conway-specific translation (no protocol branching, always strict upper bound), update Conway instances, and clean up Alonzo to always use pre-Conway logic. Remove protocol-version-based branching and obsolete helpers.

… cleanup Alonzo/Conway logic - 2025-07-01 07:33:54
@danbaruka danbaruka requested a review from a team as a code owner July 1, 2025 04:38
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.

Your changes are all correct with respect to Alonzo and Babbage eras, but for Conway you are missing the new implementation of transValidityInterval that did the new style translation. In other words, you can't use the implementation of transValidityInterval from Alonzo in Conway, instead you need a new implementation of this function that has semantics of hardforkConwayTranslateUpperBoundForPlutusScripts
Other than that it looks good.

@lehins
Copy link
Collaborator

lehins commented Jul 15, 2025

Hey @danbaruka did you still want to finish this task. It's ok if you don't, I can pass this on to finish to one of my teammates, but if you do wanna finish it yourself that is ok too, just let me know. There is no rush.

@danbaruka
Copy link
Author

Hey thanks for your feedback, I'll definitely work on that and fix it as soon as possible.

I now understand.

…; remove outdated Alonzo comment - 2025-07-21 08:18:42
@danbaruka
Copy link
Author

Conway: Implement new transValidityInterval with open upper bound semantics; clean up Alonzo comment

  • Implemented transValidityInterval in Conway era to always use an open (strict) upper bound for Plutus scripts, matching hardforkConwayTranslateUpperBoundForPlutusScripts semantics.
  • Removed outdated Haddock comment regarding validity interval translation from Alonzo era.

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.

There are still some issues. Also compilation warnings need to be addressed

@danbaruka
Copy link
Author

All requested changes have been addressed:

  • transValidityInterval now uses proxy era and has the correct forall quantifier.
  • The implementation uses the correct open upper bound logic for Conway.
  • The redundant comment was removed.
  • transSlotToPOSIXTime is now shared and reused to avoid duplication.

Let me know if anything else needs to be updated!

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.

Please, at the very least make sure your PR builds without any warnings and passes all the tests before asking for review.

pure $ PV1.Interval (PV1.lowerBound PV1.NegInf) (PV1.strictUpperBound t)
| otherwise -> PV1.to <$> slotToTime i
ValidityInterval (SJust i) (SJust j)
| pvMajor pv >= natVersion @9 -> do
Copy link
Collaborator

Choose a reason for hiding this comment

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

What's up with this protocol version? The whole reason why this refactoring was suggested precisely to avoid branching on protocol version. Also this function is never gonna be applied to protocol version 9.

Copy link
Author

Choose a reason for hiding this comment

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

Well noted thank

…protocol version checks - 2025-07-23 13:53:00
@aniketd aniketd self-requested a review August 11, 2025 17:29
@aniketd aniketd self-assigned this Aug 11, 2025
@lehins
Copy link
Collaborator

lehins commented Aug 14, 2025

Closing in favor of a correctly implemented #5230

@lehins lehins closed this Aug 14, 2025
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.

3 participants