Skip to content

Conversation

@lehins
Copy link
Contributor

@lehins lehins commented Apr 25, 2024

Description

Please include a meaningful description of the PR and link the relevant issues
this PR might resolve.

Also note that:

  • New code should be properly tested (even if it does not add new features).
  • The fix for a regression should include a test that reproduces said regression.

@lehins lehins force-pushed the lehins/hardfork-initiation branch from 0f58889 to 7b15748 Compare April 25, 2024 22:06
@Lucsanszky Lucsanszky force-pushed the lucsanszky/integration-8.11 branch from ad3720a to 45e379c Compare April 26, 2024 01:24
Copy link
Member

@amesgen amesgen left a comment

Choose a reason for hiding this comment

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

Great to see the hard fork detection code being moved to Ledger, looks very clean! 🎉

pparamsUpdate st =
let nes = shelleyLedgerState st
in ShelleyUpdatedPParams
(nes ^. SL.newEpochStateGovStateL . SL.futurePParamsGovStateG)
Copy link
Member

Choose a reason for hiding this comment

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

If want to further minimize the API surface: It would suffice to just store the protocol version here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

EpochNo is used in shelleyTransition, but my worry was that guard $ updatesBefore /= updatesAfter is affected by removal of this epochNo, so if they are guaranteed to be the same, then we can remove it from ShelleyLedgerUpdate

Copy link
Member

Choose a reason for hiding this comment

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

Right, I was imprecise; I think it makes sense to keep EpochNo here; but the Maybe (Core.PParams era) could be changed to Maybe ProtVer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So this suggestion would be fine if inspectLedger was only used for predicting protocol version. I am just really not sure that this is indeed the case. I would suspect that updates to other protocol parameters could be useful for inspection as well?

In other words if I thin this to only ProtVer then inspecLedger will return Nothing on any other parameter update, while previously it would return a Just value, due to some parameter change and this guard $ updatesBefore /= updatesAfter

@lehins lehins force-pushed the lehins/hardfork-initiation branch from 18f13c8 to c3ae431 Compare May 8, 2024 03:12
lehins added 7 commits May 7, 2024 21:30
Use the updated behavior

Fixup haddock to reflect generality of protocol version update
Change in serialization is due to the addition of future
`PParams` to the `LedgerState`
@lehins lehins force-pushed the lehins/hardfork-initiation branch from c3ae431 to 4f4296f Compare May 8, 2024 03:42
@lehins lehins marked this pull request as ready for review May 9, 2024 00:21
@lehins lehins requested a review from a team as a code owner May 9, 2024 00:21
@lehins lehins merged commit 1cf5777 into lucsanszky/integration-8.11 May 9, 2024
@lehins lehins deleted the lehins/hardfork-initiation branch May 9, 2024 00:22
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