Change watchEpochStateUpdate callback type by carbolymer · Pull Request #5855 · IntersectMBO/cardano-node

carbolymer · 2024-05-24T13:36:46Z

Description

This PR changes watchEpochStateUpdate callback type from a sum type Maybe a to a product (LedgerStateCondition, a), which allows to return from the function, when the condition is not satisfied, but the deadline epoch has been reached.

Checklist

Commit sequence broadly makes sense and commits have useful messages
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
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
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.

carbolymer · 2024-05-28T12:20:22Z

cardano-testnet/src/Testnet/Components/Query.hs

  -> EpochInterval -- ^ The maximum number of epochs to wait
-  -> ((AnyNewEpochState, SlotNo, BlockNo) -> m (Maybe a)) -- ^ The guard function (@Just@ if the condition is met, @Nothing@ otherwise)
-  -> m (Maybe a)
+  -> ((AnyNewEpochState, SlotNo, BlockNo) -> m (LedgerStateCondition, a)) -- ^ The guard function (@Just@ if the condition is met, @Nothing@ otherwise)


LedgerStateCondition needs to be renamed to a more general name. ConditionCheckResult perhaps? Will do in a follow-up PR to API.

carbolymer · 2024-05-28T12:21:14Z

cardano-testnet/test/cardano-testnet-test/Cardano/Testnet/Test/Gov/CommitteeAddNew.hs

-      pure $ maybeExtractGovernanceActionIndex (fromString governanceActionTxId) anyNewEpochState
+    H.nothingFailM . fmap snd . watchEpochStateUpdate epochStateView (L.EpochInterval 1) $ \(anyNewEpochState, _, _) -> do
+      let r = maybeExtractGovernanceActionIndex (fromString governanceActionTxId) anyNewEpochState
+      pure (if isJust r then ConditionMet else ConditionNotMet, r)


Writing ifs by hand is error-prone. This will be simplified in a follow-up pr to API. We need Bool <-> LedgerStateCondition conversion functions

smelc · 2024-05-29T12:45:16Z

cardano-testnet/src/Testnet/EpochStateProcessing.hs

-  mResult <- watchEpochStateUpdate epochStateView maxWait checkForVotes
-  when (isNothing mResult) $
+  (cond, ()) <- watchEpochStateUpdate epochStateView maxWait checkForVotes
+  when (cond == ConditionNotMet) $


I'm not a big fan of those == on a union type, for long term maintenance; but in this specific case I think it's fine. It's unlikely we'll add a third case to LedgerStateCondition.

smelc · 2024-05-29T12:47:28Z

cardano-testnet/test/cardano-testnet-test/Cardano/Testnet/Test/Gov/CommitteeAddNew.hs

+      if isCommitteePresent == Just True
+         then (ConditionMet, ())
+         else (ConditionNotMet, ())


Suggested change

if isCommitteePresent == Just True

then (ConditionMet, ())

else (ConditionNotMet, ())

case isCommitteePresent of

Just True -> (ConditionMet, ())

_ -> (ConditionNotMet, ())

Too bad Haskell doesn't have arm sharing here 🙃

palas · 2024-06-04T15:10:20Z

This adds considerable complexity and opportunity for error, and I am not sure the added complexity is worth it.
Do you have a use case in mind that couldn't be done by, for example, just querying again afterwards and returning Just immediately?

In any case, if we need the added flexibility, we could just have a restricted version of watchEpochStateUpdate on top of it that doesn't have it and use that one unless we need the generic one. That would be inline with the good practice of keeping types as tight as possible. Is like using fold when you can use map, even if map is implemented on top of fold (which probably isn't).

github-actions · 2024-08-02T01:50:29Z