partially monomorphise & de-generify Env #4521

NadiaYvette · 2022-10-11T17:25:58Z

Store's User case, Action's Set case & branching through Name's cases within Store's Named case are all unfolded. The entire Setters file along with its Tag, Sum etc. data types are removed.

Get some more privacy around the environment passed around in Action. This wraps the Env type alias in a discriminated union to generate type errors on usage, then moves direct accesses to Env and wraps the original usage in an API or moves the code to the Env module.

Env DMap via a Generic in the Set case of Action. So this limits usage of the SetKeyVal type alias to the Types module momentarily to limit exposure of the Generic that packages key-value pairs for insertion into the DMap.

This moves the access to the Set case of action exposing SetKeyVal into Types.hs; the wrapper may move again, but the caller will be insulated.

This splits it up into SNetworkId and SSocketPath and removes the old User constructor. A sweep is done and accessors in Env likewise changed.

This monomorphises Action's Set case in order to remove the last dependencies on Setters.Tag via SetKeyVal. The Setters file is removed from the tree and the cabal file as well.

There are no callers outside Env.

This trades in the Named case of Store for the various cases of Name. getName & setName are bypassed in favour of get/set of the Store directly.

They're not abstracting much as the approach is from a different direction. They don't save much typing, either. It saves a few lines & saves some module dependencies.

Searching revealed no use cases of the LoggingLayer case. This reduces the number of distinct cases needed to remove from the Env DMap.

It largely represents a sort of individual state variable, so the corresponding dmap entry in Env is replaced with an additional record field. Accessors replace the usage of the case of Store.

deepfire · 2022-10-11T22:01:07Z

bench/tx-generator/src/Cardano/Benchmarking/Script/Env.hs


 emptyEnv :: Env
-emptyEnv = Env { dmap = DMap.empty }
+emptyEnv = Env { dmap = DMap.empty, protoParams = Nothing }


Just a very minor nitpick about formatting -- I'd:

break the line, between = and Env aligning the latter naturally.

break the line, just before the , aligning the latter with the {

make the trailing } on its own line

...which is how multi-line is done frequently.

deepfire · 2022-10-11T22:03:23Z

bench/tx-generator/src/Cardano/Benchmarking/Script/Env.hs

@@ -73,12 +77,21 @@ askIOManager = lift RWS.ask
 set :: Store v -> v -> ActionM ()
 set key val = lift $ RWS.modify $ (\e -> e { dmap = DMap.insert key (pure val) (dmap e)})

+setProtoParamMode :: ProtocolParameterMode -> ActionM ()
+setProtoParamMode val = lift $ RWS.modify $ (\e -> e { protoParams = pure val })


Given that you'll have more of those cases, I'd split this into a combination of:

the generic lift . RWS.modify helper and have it accept the:

the Env modifier for specific cases

deepfire · 2022-10-11T22:04:46Z

bench/tx-generator/src/Cardano/Benchmarking/Script/Env.hs

 get :: Store v -> ActionM v
 get key = do
  lift (RWS.gets $ (\e -> DMap.lookup key $ dmap e)) >>= \case
    Just (Identity v) -> return v
    Nothing -> throwE $ LookupError key

+getProtoParamMode :: ActionM ProtocolParameterMode
+getProtoParamMode = do
+  lift (RWS.gets $ (\e -> protoParams e)) >>= \case


Here, we can similarly to the setter, extract a generic helper that calls a supplied reader on top of the common lift . RWS.gets part.

I'll mop up the read accessors in a sweep after finishing off all the Store cases. The set accessors might still be worthwhile vs. open-coding, though lift $ modify lambda isn't too big a deal either.

A pair of get & set accessors is coupled with a record field in Env to replace the usage of the Store case in the Env DMap.

This adds an Env record field to replace the Genesis DMap entry and sweeps uses of the Store case/constructor replacing with accessors.

The Env accessors were duplicating a lot of code, so this just does some lambda lifting across the minor differences. The subroutines for the shared code are likely worth using to replace the accessors outright given an audit of record field names for name clashes & renaming for more brevity.

It's replaced with a record field in Env and accessors to get & set it.

I inadvertently introduced hlint warnings in this branch by dint of some cut & paste of unnecessary $'s. This should hopefully dispose of them, but I'll have to find out how to run hlint in the build system to be sure.

This replaces the SNetworkId case of Store with a monomorphic NetworkId record field of Env.

This folds the socket's FilePath into a record field of Env.

This replaces the KeyName case with a monomorphic Map in Env. Helpers are set up to help subsequent wallet & thread patches. Some testing will be needed to ensure there's no impact on Aeson's JSON formatting.

This removes the ThreadName constructor from Store and replaces it with a monomorphic Map in the Env structure and accessors.

This removes the use of the WalletName type alias, WalletName constructor, and the entire Store module, moving its remainder to Script/Types. At the end of this, the use of the Generic DMap is also removed altogether in Env. The cabal file is updated to reflect the removal of the Store.hs source file.

mgmeier · 2022-10-12T15:27:42Z

Thank you a lot, Nadia! :)

* hide-env Get some more privacy around the environment passed around in Action. This wraps the Env type alias in a discriminated union to generate type errors on usage, then moves direct accesses to Env and wraps the original usage in an API or moves the code to the Env module. * SetKeyVal exposes an inextricable precursor to the Generic usage in the Env DMap via a Generic in the Set case of Action. So this limits usage of the SetKeyVal type alias to the Types module momentarily to limit exposure of the Generic that packages key-value pairs for insertion into the DMap. * Hide usage of Action Set case in setConst. This moves the access to the Set case of action exposing SetKeyVal into Types.hs; the wrapper may move again, but the caller will be insulated. * Monomorphise the User case of the Store type. This splits it up into SNetworkId and SSocketPath and removes the old User constructor. A sweep is done and accessors in Env likewise changed. * Remove Tag, Setters This monomorphises Action's Set case in order to remove the last dependencies on Setters.Tag via SetKeyVal. The Setters file is removed from the tree and the cabal file as well. * Remove unSet & consumeName There are no callers outside Env. * Fold Name into Store This trades in the Named case of Store for the various cases of Name. getName & setName are bypassed in favour of get/set of the Store directly. * Remove [gs]et(NetworkId|SocketPath) accessors. They're not abstracting much as the approach is from a different direction. They don't save much typing, either. It saves a few lines & saves some module dependencies. * Remove LoggingLayer case of Store. Searching revealed no use cases of the LoggingLayer case. This reduces the number of distinct cases needed to remove from the Env DMap. * Remove ProtocolParameterMode case from Store It largely represents a sort of individual state variable, so the corresponding dmap entry in Env is replaced with an additional record field. Accessors replace the usage of the case of Store. * Remove BenchTracers case from Store A pair of get & set accessors is coupled with a record field in Env to replace the usage of the Store case in the Env DMap. * Remove Genesis case of Store This adds an Env record field to replace the Genesis DMap entry and sweeps uses of the Store case/constructor replacing with accessors. * Share Env accessor code The Env accessors were duplicating a lot of code, so this just does some lambda lifting across the minor differences. The subroutines for the shared code are likely worth using to replace the accessors outright given an audit of record field names for name clashes & renaming for more brevity. * Remove Protocol case of Store It's replaced with a record field in Env and accessors to get & set it. * Env hlint warning cleanup I inadvertently introduced hlint warnings in this branch by dint of some cut & paste of unnecessary $'s. This should hopefully dispose of them, but I'll have to find out how to run hlint in the build system to be sure. * Fix extraneous parentheses flagged by hlint * Remove SNetworkId case of Store This replaces the SNetworkId case of Store with a monomorphic NetworkId record field of Env. * Remove SSocketPath from Store This folds the socket's FilePath into a record field of Env. * Remove KeyName case from Store This replaces the KeyName case with a monomorphic Map in Env. Helpers are set up to help subsequent wallet & thread patches. Some testing will be needed to ensure there's no impact on Aeson's JSON formatting. * Remove ThreadName from Store This removes the ThreadName constructor from Store and replaces it with a monomorphic Map in the Env structure and accessors. * Remove Store altogether This removes the use of the WalletName type alias, WalletName constructor, and the entire Store module, moving its remainder to Script/Types. At the end of this, the use of the Generic DMap is also removed altogether in Env. The cabal file is updated to reflect the removal of the Store.hs source file. Co-authored-by: Michael Karg <[email protected]>

NadiaYvette added 8 commits October 10, 2022 23:51

hide-env

6f9786c

Get some more privacy around the environment passed around in Action. This wraps the Env type alias in a discriminated union to generate type errors on usage, then moves direct accesses to Env and wraps the original usage in an API or moves the code to the Env module.

Hide usage of Action Set case in setConst.

b1c5479

This moves the access to the Set case of action exposing SetKeyVal into Types.hs; the wrapper may move again, but the caller will be insulated.

Monomorphise the User case of the Store type.

aab193f

This splits it up into SNetworkId and SSocketPath and removes the old User constructor. A sweep is done and accessors in Env likewise changed.

Remove Tag, Setters

8ddfaed

This monomorphises Action's Set case in order to remove the last dependencies on Setters.Tag via SetKeyVal. The Setters file is removed from the tree and the cabal file as well.

Remove unSet & consumeName

64ccd8d

There are no callers outside Env.

Fold Name into Store

d707426

This trades in the Named case of Store for the various cases of Name. getName & setName are bypassed in favour of get/set of the Store directly.

Remove [gs]et(NetworkId|SocketPath) accessors.

4d22bb9

They're not abstracting much as the approach is from a different direction. They don't save much typing, either. It saves a few lines & saves some module dependencies.

NadiaYvette requested review from deepfire, denisshevchenko, jutaro, MarcFontaine, mgmeier, fmaste and cleverca22 as code owners October 11, 2022 17:25

NadiaYvette added 2 commits October 11, 2022 16:59

Remove LoggingLayer case of Store.

80feb2a

Searching revealed no use cases of the LoggingLayer case. This reduces the number of distinct cases needed to remove from the Env DMap.

Remove ProtocolParameterMode case from Store

42c40a5

It largely represents a sort of individual state variable, so the corresponding dmap entry in Env is replaced with an additional record field. Accessors replace the usage of the case of Store.

deepfire reviewed Oct 11, 2022

View reviewed changes

NadiaYvette added 10 commits October 11, 2022 18:18

Remove BenchTracers case from Store

359874c

A pair of get & set accessors is coupled with a record field in Env to replace the usage of the Store case in the Env DMap.

Remove Genesis case of Store

2d49382

This adds an Env record field to replace the Genesis DMap entry and sweeps uses of the Store case/constructor replacing with accessors.

Remove Protocol case of Store

29db9a0

It's replaced with a record field in Env and accessors to get & set it.

Env hlint warning cleanup

4a7eb72

I inadvertently introduced hlint warnings in this branch by dint of some cut & paste of unnecessary $'s. This should hopefully dispose of them, but I'll have to find out how to run hlint in the build system to be sure.

Fix extraneous parentheses flagged by hlint

8f8991a

Remove SNetworkId case of Store

2fd0d23

This replaces the SNetworkId case of Store with a monomorphic NetworkId record field of Env.

Remove SSocketPath from Store

12447bc

This folds the socket's FilePath into a record field of Env.

Remove KeyName case from Store

499e245

This replaces the KeyName case with a monomorphic Map in Env. Helpers are set up to help subsequent wallet & thread patches. Some testing will be needed to ensure there's no impact on Aeson's JSON formatting.

Remove ThreadName from Store

c883487

This removes the ThreadName constructor from Store and replaces it with a monomorphic Map in the Env structure and accessors.

NadiaYvette and others added 2 commits October 12, 2022 10:43

Merge branch 'tx-generator/4174-reusable-api-and-library' into nyc-api-2

a848918

mgmeier approved these changes Oct 12, 2022

View reviewed changes

mgmeier merged commit e589d22 into IntersectMBO:tx-generator/4174-reusable-api-and-library Oct 12, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

partially monomorphise & de-generify Env #4521

partially monomorphise & de-generify Env #4521

NadiaYvette commented Oct 11, 2022

deepfire Oct 11, 2022

deepfire Oct 11, 2022

deepfire Oct 11, 2022

NadiaYvette Oct 11, 2022

mgmeier commented Oct 12, 2022

partially monomorphise & de-generify Env #4521

partially monomorphise & de-generify Env #4521

Conversation

NadiaYvette commented Oct 11, 2022

deepfire Oct 11, 2022

Choose a reason for hiding this comment

deepfire Oct 11, 2022

Choose a reason for hiding this comment

deepfire Oct 11, 2022

Choose a reason for hiding this comment

NadiaYvette Oct 11, 2022

Choose a reason for hiding this comment

mgmeier commented Oct 12, 2022