Make tests polymorphic over the Value type #1913

polinavino · 2020-10-13T22:36:23Z

Remove the constraint Core.Value era ~ Coin from the bundle ShelleyTest
Add to ShelleyTest testing-related constraints on the Value type (Arbitrary, hedgehog, Split)
Change ShelleyBased from a constraint bundle to a type class
Remove UndecidableInstances where they're not needed
Update generators (genTxOut) of outputs to generate ones containing Value era
Rename Value type family to VALUE following some (apparently existing) convention for type family names, then wrap VALUE in Value newtype
Split out Compatible as a module
Leave testing files where ConcreteCryptoTypes (C) is imported as using the C era

nc6

Just did a partial review to leave some comments (now stuck in meetings)! Might be good to go through this in a meeting since there's probably some subtleties to it

nc6 · 2020-10-14T09:56:28Z

shelley-ma/impl/src/Cardano/Ledger/ShelleyMA.hs

@@ -33,4 +33,4 @@ type family MAValue (x :: MaryOrAllegra) era :: Type where
 MAValue Allegra era = Coin
 MAValue Mary era = Value era

-type instance Core.Value (ShelleyMAEra m c) = MAValue m (ShelleyMAEra m c)


Why is Value now in ALLCAPS?

ha! we (Alex, Tim and I) discussed type families being in all caps (there is already some attempt to make this a convention it looks like from before), then wrapped in a newtype.

leaving for this PR

nc6 · 2020-10-14T09:56:54Z

shelley/chain-and-ledger/executable-spec/shelley-spec-ledger.cabal

@@ -19,6 +19,7 @@ flag development
 library
 exposed-modules:
 Cardano.Ledger.Core
+ Cardano.Ledger.Compactible


nc6 · 2020-10-14T10:00:03Z

shelley/chain-and-ledger/executable-spec/src/Cardano/Ledger/Core.hs

+-- the genericShrink has something that
+-- detects that the immediate subterms of a type are the same as the parent type
+-- when there is a type family in that position, the instance resolution fails
+newtype Value era = Value {unVl :: VALUE era}


Ah, I see. Somehow I seem to have got around this issue in #1908, though in honesty I haven't the foggiest how. It seems an unfortunate hack, so I'd hope we could avoid it, but maybe that just causes more trouble...

I think it is because you have put ShelleyEra c where it was just era before, together with the ShelleyBased era constraint. ShelleyEra includes Core.Value~Coin and TxBody being the Shelley TxBody, so genericShrink was not confused any more

maybe rewrite the shrinker to a manual shrinker? to avoid this

leaving shrinker as is for now

shelley/chain-and-ledger/executable-spec/src/Cardano/Ledger/Shelley.hs

nc6 · 2020-10-14T10:01:59Z

shelley/chain-and-ledger/executable-spec/src/Cardano/Ledger/Core.hs

@@ -14,46 +16,61 @@ module Cardano.Ledger.Core
 ( -- * Compactible
 Compactible (..),


I'd suggest now that Compactible is in its own module (which is great!) we avoid re-exporting it from here?

nc6 · 2020-10-14T10:05:02Z

shelley/chain-and-ledger/executable-spec/src/Shelley/Spec/Ledger/STS/Utxo.hs

@@ -146,7 +146,7 @@ deriving stock instance
 ShelleyBased era =>
 Eq (UtxoPredicateFailure era)

-instance NoThunks (Core.Value era) => NoThunks (UtxoPredicateFailure era)
+instance ShelleyBased era => NoThunks (UtxoPredicateFailure era)


I'd argue that cases like this (where there's only one constraint, so we don't save space by using the bundle) have a strong argument for remaining with the more specific constraint. It's better as documentation (we see exactly what we're relying on) and we have more chance of being able to re-use this in the future.

to remove UndecidableInstances. So I should put it back?

...ley/chain-and-ledger/shelley-spec-ledger-test/src/Test/Shelley/Spec/Ledger/Generator/Core.hs

polinavino · 2020-10-14T20:36:05Z

...n-and-ledger/shelley-spec-ledger-test/test/Test/Shelley/Spec/Ledger/Examples/PoolLifetime.hs

 deltaT8 :: Coin
 deltaT8 = Coin 317333333333

+-- TODO make DeltaCoin?
 deltaR8 :: Coin
 deltaR8 = Coin (-330026666665)



I hope this not being a delta coin is the reason for the test failure! Coins must always be positive or there will be ghosts..

changed this to +ve value and subtraction at use site for now!

polinavino · 2020-10-15T15:06:22Z

shelley/chain-and-ledger/executable-spec/src/Cardano/Ledger/Shelley.hs


-type TxBodyConstraints era =
+class


put back to type

polinavino · 2020-10-15T15:37:52Z

shelley/chain-and-ledger/shelley-spec-ledger-test/bench/Main.hs

@@ -88,6 +90,7 @@ instance CryptoClass.Crypto BenchCrypto where
 type ADDRHASH BenchCrypto = Blake2b_224

 type BenchEra = ShelleyEra BenchCrypto
+type instance VALUE BenchEra = Coin



consider changing convention for type families (or get rid of newtype)

always use qualified imports for type families

polinavino · 2020-10-15T15:43:07Z

...ley/chain-and-ledger/shelley-spec-ledger-test/src/Test/Shelley/Spec/Ledger/Generator/Core.hs

+genValList :: (ShelleyTest era) => Integer -> Integer -> Int -> Gen [Core.Value era]
+genValList minCoin maxCoin len = do
+ let addWOCoin c v = c <+> (v <-> (Val.inject $ Val.coin v))
+ replicateM len $ liftM2 addWOCoin (Val.inject <$> genCoin minCoin maxCoin) QC.arbitrary



pass generator for Val instead of using arbitrary

look at valprop.hs

leaving for now

polinavino · 2020-10-15T15:51:15Z

shelley/chain-and-ledger/shelley-spec-ledger-test/src/Test/Shelley/Spec/Ledger/Orphans.hs

+instance Split Coin where
+ vsplit (Coin n) 0 = ([], Coin n)
+ vsplit (Coin n) m -- TODO fix this?
+ | m Prelude.<= 0 = error "must split coins into positive parts"


polinavino · 2020-10-15T15:58:47Z

shelley/chain-and-ledger/shelley-spec-ledger-test/src/Test/Shelley/Spec/Ledger/Utils.hs

- Core.Value era ~ Coin,
+type ShelleyTest era = (ShelleyBased era, Arbitrary (Core.Value era),
+  HedGen (Core.Value era), Split (Core.Value era), P.PartialOrd (Core.VALUE era),
+ Arbitrary (Core.VALUE era), HedGen (Core.VALUE era), Split (Core.VALUE era) )


remove partial order and add a a constraint that requires to have pointwise comparison

polinavino · 2020-10-15T16:02:00Z

shelley/chain-and-ledger/shelley-spec-ledger-test/src/Test/Shelley/Spec/Ledger/Orphans.hs

+instance Arbitrary Coin where
+ -- Cannot be negative even though it is an 'Integer'
+ arbitrary = Coin <$> choose (0, 1000)
+


plumbing in our own generators should make us able to remove these

leaving for now

polinavino · 2020-10-15T16:05:49Z

shelley/chain-and-ledger/shelley-spec-ledger-test/src/Test/Shelley/Spec/Ledger/Shrinkers.hs

+ extraCoin sr = Val.coin $ extraTokens sr
+ -- put all the non-ada tokens in the head of the outputs, append shrunk list
+ mvExtraTksnToOut1 Empty = empty
+ mvExtraTksnToOut1 sr = (<|) (TxOut a (vs <+> (extraTokens sr) <-> (Val.inject $ extraCoin sr))) sr


polinavino · 2020-10-15T16:06:48Z

shelley/chain-and-ledger/shelley-spec-ledger-test/src/Test/Shelley/Spec/Ledger/Shrinkers.hs

-shrinkTxOut (TxOut addr coin) =
- TxOut addr <$> shrinkCoin coin
+shrinkTxOut (TxOut addr vl) =
+ TxOut addr <$> shrink vl



pass in our own shrinker here

TODO comment that we dont shrink the value here

polinavino · 2020-10-15T16:09:30Z

shelley/chain-and-ledger/shelley-spec-ledger-test/src/Test/Shelley/Spec/Ledger/Utils.hs

@@ -36,9 +36,12 @@ module Test.Shelley.Spec.Ledger.Utils
 MultiSigPairs,


paused review here

nc6 · 2020-10-19T07:51:23Z

PR #1922 may also now be helpful here!

JaredCorduan

LGTM

polinavino requested review from JaredCorduan and uroboros as code owners October 13, 2020 22:36

nc6 reviewed Oct 14, 2020

View reviewed changes

polinavino force-pushed the polina/paramtests branch from b4f9778 to 79afad8 Compare October 14, 2020 20:33

polinavino commented Oct 14, 2020

View reviewed changes

polinavino commented Oct 15, 2020

View reviewed changes

polinavino force-pushed the polina/paramtests branch from 79afad8 to 7119510 Compare October 16, 2020 20:44

polymorphic tests

fad5cfe

JaredCorduan force-pushed the polina/paramtests branch from 50dc43a to fad5cfe Compare October 20, 2020 01:11

Merge branch 'master' into polina/paramtests

303f20f

JaredCorduan approved these changes Oct 20, 2020

View reviewed changes

JaredCorduan merged commit 43bde81 into master Oct 20, 2020

iohk-bors bot deleted the polina/paramtests branch October 20, 2020 14:01

JaredCorduan mentioned this pull request Oct 20, 2020

Revert "Make tests polymorphic over the Value type" #1928

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make tests polymorphic over the Value type #1913

Make tests polymorphic over the Value type #1913

polinavino commented Oct 13, 2020 •

edited

Loading

nc6 left a comment

nc6 Oct 14, 2020

polinavino Oct 14, 2020

polinavino Oct 16, 2020

nc6 Oct 14, 2020

nc6 Oct 14, 2020

polinavino Oct 14, 2020 •

edited

Loading

polinavino Oct 15, 2020

polinavino Oct 16, 2020

nc6 Oct 14, 2020

polinavino Oct 14, 2020

nc6 Oct 14, 2020

polinavino Oct 14, 2020

polinavino Oct 15, 2020

polinavino Oct 14, 2020

polinavino Oct 16, 2020

polinavino Oct 15, 2020

polinavino Oct 15, 2020

polinavino Oct 15, 2020

polinavino Oct 15, 2020

polinavino Oct 15, 2020

polinavino Oct 16, 2020

polinavino Oct 15, 2020

polinavino Oct 15, 2020

polinavino Oct 16, 2020

polinavino Oct 15, 2020

polinavino Oct 16, 2020

polinavino Oct 15, 2020

polinavino Oct 15, 2020

polinavino Oct 15, 2020

polinavino Oct 15, 2020

nc6 commented Oct 19, 2020

JaredCorduan left a comment

		@@ -14,46 +16,61 @@ module Cardano.Ledger.Core
		( -- * Compactible
		Compactible (..),

		@@ -36,9 +36,12 @@ module Test.Shelley.Spec.Ledger.Utils
		MultiSigPairs,

Make tests polymorphic over the Value type #1913

Make tests polymorphic over the Value type #1913

Conversation

polinavino commented Oct 13, 2020 • edited Loading

nc6 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

polinavino Oct 14, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nc6 commented Oct 19, 2020

JaredCorduan left a comment

Choose a reason for hiding this comment

polinavino commented Oct 13, 2020 •

edited

Loading

polinavino Oct 14, 2020 •

edited

Loading