Event Log #2244
Event Log #2244
Conversation
|
I'm not familiar enough with the codebase yet to know where any more interesting events might be, but there's an example usage @ https://github.com/input-output-hk/cardano-ledger-specs/pull/2244/files#diff-a367824a11ce6839b2fb774cca01608b15ddeba59d6734fab737f10f0c409cf6R183 |
semantics/executable-spec/src/Control/State/Transition/Extended.hs
Outdated
Show resolved
Hide resolved
semantics/executable-spec/src/Control/State/Transition/Extended.hs
Outdated
Show resolved
Hide resolved
semantics/executable-spec/src/Control/State/Transition/Extended.hs
Outdated
Show resolved
Hide resolved
…ecs into event-logger
| @@ -43,6 +43,8 @@ instance STS PBFT where | |||
|
|
|||
| type PredicateFailure PBFT = PbftPredicateFailure | |||
|
|
|||
| data Event _ = SigCountEvent (Event SIGCNT) | |||
There was a problem hiding this comment.
Wait, what? You can just use an _ for associated data/type families? I never knew that!
| EventReturnType 'EventPolicyReturn sts a = (a, [Event sts]) | ||
| EventReturnType _ _ a = a | ||
|
|
||
| type family EventConstraintType e sts m :: Constraint where |
There was a problem hiding this comment.
I'm not sure the liberalism in the constraint here seems necessary given its current usage. When I suggested something like this, it was for the purpose of allowing an alternative "reporting" mechanism - rather than returning the events, yield them into the base monad in some way. At the moment, however, it's used to generalise a purely internal monad which doesn't escape the scope of runClause. Since this is already at least a State monad, I don't see any harm in just requiring that to have an "event" component that gets left empty if there are no events being collected.
There was a problem hiding this comment.
ah, i don't think it's necessary at all, then. it's always interpreted at https://github.com/input-output-hk/cardano-ledger-specs/pull/2244/files/0947eea86860dd4cbef1ed1e8a7f59587ae8c27c#diff-8d279cc8073de17e2b7c14f40f0bb530821125948ffa4d43c4712b9ae64d7261R514-R516 into the clause monad somehow. it would be trivial to extend this to modify some impure state, log to stdout, whatever
There was a problem hiding this comment.
Do you want to remove it, in that case?
There was a problem hiding this comment.
EventReturnType is gone, EventConstraintType is still useful
There was a problem hiding this comment.
Oh, wait, now I'm totally confused. I had thought the complete opposite way around...
There was a problem hiding this comment.
Namely: the point of the polymorphic return type is that consumers calling these methods (some outside of the ledger code) can continue to have exactly the same method signature, unless they choose to enable event logging. But the internal constraint seems to serve no useful purpose; that monad may just be fixed as the state monad, regardless of event logging. There's nothing the user can do to change it, so making it polymorphic seems unnecessary.
The initial idea behind the constraint polymorphism was to put the constraint on the base monad, thus allowing reporting into the base monad iff it supported such.
There was a problem hiding this comment.
ah ok, we do want to be polymorphic in the return type. nbd, I can just revert that change.
The initial idea behind the constraint polymorphism was to put the constraint on the base monad, thus allowing reporting into the base monad if it supported such.
this is still desirable in my opinion, I can imagine at some point in the future introducing a new Event policy that modifies some impure state. I expect it will really only ever be used in runClause
There was a problem hiding this comment.
tbh after thinking about it a bit, having EventReturnType seems a bit unnecessary. I think since we need to pass the event return policy, there's already an API change so we may as well not have this type family in the return type
…ecs into event-logger
This reverts commit ad0975a.
This reverts commit 58e3a65.
…ecs into event-logger
| @@ -104,6 +105,10 @@ instance | |||
| type BaseM (POOL era) = ShelleyBase | |||
| type PredicateFailure (POOL era) = PoolPredicateFailure era | |||
|
|
|||
| data Event (POOL era) | |||
| = NewPoolParam | |||
| | NewFuturePoolParam | |||
There was a problem hiding this comment.
Is the plan to eventually add more information to these? I think just the pool ID is all we really need.
There was a problem hiding this comment.
Looks good to me, thanks @goolord ! I think this is good to merge, and we can add more details into the tellEvents in follow up PRs.
JaredCorduan
dismissed
nc6’s stale review
I think it makes sense to merge this, we can always change it in follow-up PRs.
No description provided.