Provide property tests for changeUTxO

[jonathanknowles]

Issue Number

ADP-1084

Comments

This PR provides property tests for the changeUTxO function.

Since this function is re-used by totalUTxO (which we want to modify and then verify), it's useful to first verify that changeUTxO is correct.

[rvl]

Suggested change

	prop_changeUTxO :: Property
	prop_changeUTxO =
	forAllShrink (listOf genTxOutputs) (shrinkList shrinkTxOutputs) inner
	where
	inner :: [TxOutputs] -> Property
	inner txOutputs =
	newtype Classify a = Classify (a -> Bool)
	prop_changeUTxO :: Classify Address -> [TxOut] -> Property
	prop_changeUTxO (Classify condition) txOutputs =

[jonathanknowles]

@rvl Nice suggestion!

I've actually made a similar change in 0ffa43d, which replaces the various types with a single IsOursIf {condition :: a -> Bool} newtype (with a single instance).

[rvl]

Any other Address -> Bool function would be just as good, as long it splits the set of generated addresses into roughly equal parts.

[jonathanknowles]

Yes, definitely.

It's an arbitrary choice (excuse the pun!) to use this particular function. It has two nice advantages:

1. Addresses are "even" or "odd" with equal frequency (meaning we don't have to do anything special with generation).
2. Address parity can be determined with a pure function (meaning we don't have to keep any state around in our IsOurs instance).

I've added a comment to prop_changeUTxO with these reasons.

[rvl]

Really? The only place this is used it's with (not (UTxO.null x). But x /= mempty would be fine.

[jonathanknowles]

Really? The only place this is used it's with (not (UTxO.null x). But x /= mempty would be fine.

I agree with you that x /= mempty would work fine!

Though I find that UTxO.null is a little more self-documenting (in this case), as:

• A reader doesn't have to navigate to the definition to figure out the type of thing being tested, because it's stated right at the point of use; and
• The null function is idiomatic across different container types, so it's familiar.

[sevanspowell]

I really love the way you've set out the comments, makes it easy to understand your logic and reasoning 👍 I'm trying to emulate your style in my own work.

My first thought was that changeUTxO is such a simple function and I wasn't sure how your new tests actually tested it, but now I understand that though the function appears simple, the existence of State s, and the way the behaviour of the function could arbitrarily change based on s and the IsOurs instance, makes more involved testing necessary.

I find this interesting because I'm working in a very similar place at the moment and I've made some modifications, let me know what you think:

• Using a type class is equivalent to implicitly passing the typeclass members to the function.

• Therefore we can re-write changeUTxO as:

  changeUTxO
  :: (Address -> s -> (Maybe (NonEmpty DerivationIndex), s))
  -> Set Tx
  -> s
  -> UTxO
  changeUTxO isOurs pending = evalState $
      mconcat <$> mapM (state . utxoOurs isOurs) (Set.toList pending)
  

  (also making the relevant change to utxoOurs and so on...)

• But utxoOurs, and by extension changeUTxO uses isOurs :: Address -> s -> (Maybe (NonEmpty DerivationIndex), s) purely as a isOurs :: Address -> s -> (Bool, s) function. You confirm this with your isOursIf function, which essentially lifts Address -> Bool to a "proper" isOurs function (in a non-total manner). i.e. we don't care about the result

• A function can easily be written to demote isOurs :: Address -> s -> (Maybe DerivationIndex, s) to isOurs :: Address -> s -> (Bool, s).

• and entity -> s -> (result, s) is a special case of entity -> f result, where f is s -> (s,) (a functor)

• So we can further re-write changeUTxO as:

  changeUTxO
  :: (Address -> f Bool)
  -> Set Tx
  -> f UTxO
  changeUTxO isOurs pending = ...
  

  while retaining the original behaviour, although how we call changeUTxO will change (i.e. evalState $ changeUTxO isOursBool)

• For the purposes of testing, we can now do a couple of things:

  1. Offload the burden-of-proof that the Address -> f Bool, and the f functor, are working correctly off to the writer of those functions.
     • That's still "us", but maybe testing our demoted isOursBool function in isolation is a little bit easier.
  2. Re-write the tests of this function to not use State at all (e.g. use Identity functor for f). We're making it more explicit that this function can only rely on specific properties of f (e.g. Functor, Applicative, Monad).

• Maybe we can even write a property that asserts the equivalence between changeUTxO and some other similar exisiting function like filterM:
  ∀f set. toList (changeUTxO f set) = filterM f (toList set)

I haven't tested this out so your mileage may vary. One thing to note is that we can "not use typeclasses" in changeUTxO, but continue using typeclasses everywhere else, it's an incremental change rather than a "don't ever use IsOurs", although "some" people "out there" do have an aversion to using typeclases that don't have laws (i.e. Functor typeclass has laws, but not IsOurs). That's a bit irrelevant to all this though.

TLDR; the type signature of changeUTxO can be changed to more accurately reflect it's usage, and so shift responsibility of some of the more complicated parts off to another module's tests, which /might/ be worth it.

Otherwise looks really good! This is just food for thought and I'm happy to approve and merge as-is.

[jonathanknowles]

bors r+

[iohk-bors]

Build failed:

• buildkite/cardano-wallet

Looks like a failure of the "Lint bash shell scripts" stage:
https://buildkite.com/input-output-hk/cardano-wallet/builds/16305#da2c1f53-cc01-4dfd-93b8-8b4d689cad8c

I'm marking as
#expected
since it was due to a manual merge rather than due to flakiness.

[Anviking]

bors r+

[iohk-bors]

Build succeeded:

• buildkite/cardano-wallet
• ci/hydra-build:required