Parameterize over what it means to be an Asset (Coin v.s. Value) #1815

TimSheard · 2020-08-27T15:34:03Z

This PR parameterizes over what kind of assets a transaction can produce. In the old style we used a Coin, and as an alternative to that we introduce the type Value (which is a kind of multi-asset). Both these types are instances of the Val class, so the code has been made parametric over objects in the Val class.

JaredCorduan · 2020-08-27T16:28:27Z

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

 sgActiveSlotCoeff =
 mkActiveSlotCoeff
 . unitIntervalFromRational
 . sgActiveSlotsCoeff

-instance Crypto crypto => ToJSON (ShelleyGenesis crypto) where
+-- TODO JSON for v?


I think we can remove this TODO

JaredCorduan · 2020-08-27T16:30:40Z

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

 txsize = fromIntegral . BSL.length . txFullBytes

 -- | Convenience Function to bound the txsize function.
 -- | It can be helpful for coin selection.
-txsizeBound :: forall crypto. (Crypto crypto) => Tx crypto -> Integer
+txsizeBound :: forall crypto v. (CV crypto v) => Tx crypto v -> Integer
 txsizeBound tx = numInputs * inputSize + numOutputs * outputSize + rest


I think we might be able to get rid of this function altogether. which would mean we could also clean up the transaction type (it stores the input and output bytes so that txsizeBound can remove them). we can do this in a separate PR.

JaredCorduan · 2020-08-27T16:31:29Z

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

 Coin
 keyRefunds pp tx = (_keyDeposit pp) * (fromIntegral $ length deregistrations)
 where
 deregistrations = filter isDeRegKey (toList $ _certs tx)

+-- TODO add forge


I don't think we need to add all these TODOs in the code

a github issue would be more helpful

JaredCorduan · 2020-08-27T16:35:10Z

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

@@ -159,20 +159,20 @@ instance
 PostCondition
 "Deposit pot must not be negative (post)"
 (\_ st' -> _deposited st' >= 0),
- let utxoBalance us = _deposited us + _fees us + balance (_utxo us)
+ let utxoBalance us = vplus (vinject $ _deposited us + _fees us) (balance (_utxo us))


can we use <> to make it a bit more like the original?

JaredCorduan · 2020-08-27T16:36:26Z

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

@@ -301,11 +304,18 @@ utxoInductive = do
 -- process Protocol Parameter Update Proposals
 ppup' <- trans @(PPUP crypto) $ TRC (PPUPEnv slot pp genDelegs, ppup, txup tx)

- let outputs = Set.toList (eval (rng (txouts txb)))
+ -- TODO check voper


can we remove this? or do we need to check something?

JaredCorduan · 2020-08-27T16:36:46Z

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

+ -- TODO add forge errors
+ -- let (Value vls) = _forge txb
+ -- let cids = Map.keys vls
+ -- all (adaID /=) cids ?! (ForgingAda (Value vls))


let's remove this and make an issue instead

This is more to do with actually adding forging functionality than just doing the parametrization. Can definitely be removed from here.

JaredCorduan · 2020-08-27T16:42:32Z

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

-
-deriving via
- AllowThunksIn
- '[ "txWitsBytes"


don't we still want the annotations to be thunks?

We couldnt get deriving via to work with AllowThunksIn because of lack of ability to derive Generic for everything in Val. Is this essential?

If the field is lazy but the instance doesn't mark it as allowed, this will trip thunk detection.

If the field is made strict, this will trigger serialization every time a witness set is modified.

One solution would be to figure out a way to get a generic instance for the val. Another would be to write a manual instance.

JaredCorduan · 2020-08-27T16:53:32Z

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

+ CV crypto v =>
+ {-# UNPACK #-} !BSS.ShortByteString ->
+ v ->
+ TxOut crypto v


it seems we can no longer unpack the Word64 from the coin if we want this to be parametric, so the UTxO memory size will be a bit bigger (one word per TxOut?). we can maybe make an issue to measure the difference.

JaredCorduan · 2020-08-27T16:55:09Z

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

-instance Crypto c => HashAnnotated (TxBody c) c
+instance CV c v => HashAnnotated (TxBody c v) c
+
+-- TODO no thunks in bodyBytes is ok?


that's a good point, I think we want the annotations to be thunks

JaredCorduan · 2020-08-27T16:57:00Z

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

- case decompactAddr bs of
- Just (_ :: Addr crypto) -> pure $ TxOutCompact bs coin
- Nothing -> cborError $ DecoderErrorCustom "TxOut" "invalid address"
+ addr <- fromCBOR


I think we still need to check that the address parses with decompactAddr

JaredCorduan · 2020-08-27T16:59:03Z

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

-balance :: UTxO crypto -> Coin
-balance (UTxO utxo) = fromIntegral $ Map.foldl' addCoins 0 utxo
+balance :: CV crypto v => UTxO crypto v -> v
+balance (UTxO utxo) = foldr addCoins vzero utxo


JaredCorduan · 2020-08-27T16:59:51Z

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

 where
- addCoins !b (TxOutCompact _ a) = a + b
+ addCoins (TxOut _ a) b = vplus a b


I think we still want the real constructor TxOutCompact. and maybe <> would be more clear

Yeah, please don't undo #1771.

(Maybe you started working on this with an out-of-date checkout, one that didn't include that PR, and this change got lost while merging/rebasing? If so, we should be careful that no other such changes are in this PR.)

JaredCorduan · 2020-08-27T17:00:13Z

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

+ [cert | cert@(DCertPool (RegPool pparams)) <- toList (_certs txb), rewardAcntUsesScript (_poolRAcnt pparams)]
+ mirCertsUsingScripts =
+ [cert | cert@(DCertMir mir) <- toList (_certs txb), any credentialUsesScript (Map.keys (mirRewards mir))]
+-}


JaredCorduan · 2020-08-27T17:09:48Z

...hain-and-ledger/shelley-spec-ledger-test/src/Test/Shelley/Spec/Ledger/Generator/Constants.hs

+ maxReserves = 10000000,
+ genTxRetries = 6,
+ genTxBudget = 10000
+ }


let's not indent so that we can see exactly what changed

JaredCorduan · 2020-08-27T17:10:42Z

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

@@ -58,7 +58,8 @@ import Cardano.Crypto.DSIGN.Class (DSIGNAlgorithm (..))
 import Cardano.Crypto.VRF (evalCertified)
 import qualified Cardano.Crypto.Hash as Hash
 import Control.Iterate.SetAlgebra (eval, (∪), (⋪))
-import Control.Monad (replicateM)
+


mystery line! :)

JaredCorduan · 2020-08-27T17:13:41Z

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

+ [ (frequencyTxWithMetaData, genMetaData'),
+ (100 - frequencyTxWithMetaData, pure (SNothing, SNothing))
+ ]
+ pure(a,b)


I think this doesn't actually change anything

JaredCorduan · 2020-08-27T17:14:33Z

...in-and-ledger/shelley-spec-ledger-test/src/Test/Shelley/Spec/Ledger/Generator/Trace/DCert.hs

@@ -150,7 +150,7 @@ genDCerts ::
 ([KeyPair 'Witness c], [(MultiSig c, MultiSig c)])
 )
 genDCerts
- ge@( GenEnv
+  ge@( GenEnv


we don't need this

JaredCorduan · 2020-08-27T17:15:06Z

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

+ pure (Nothing, [])
+ )
+ ]
+ pure(a,b)


also not needed

JaredCorduan · 2020-08-27T17:15:49Z

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

@@ -57,18 +62,18 @@ import Shelley.Spec.Ledger.LedgerState
 DState (..),
 KeyPairs,
 UTxOState (..),
- minfeeBound,
+ minfee,


JaredCorduan · 2020-08-27T17:16:05Z

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

@@ -82,6 +87,8 @@ import Shelley.Spec.Ledger.UTxO
 makeWitnessesFromScriptKeys,
 makeWitnessesVKey,
 )
+-- import Shelley.Spec.Ledger.Value (Val (..))


JaredCorduan · 2020-08-27T17:19:19Z

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

@@ -173,40 +179,219 @@ genTxRetry
 -- SpendingBalance, Output Addresses (including some Pointer addresses)
 -- and a Outputs builder that distributes the given balance over addresses.
 -------------------------------------------------------------------------
- let spendingBalance = spendingBalanceUtxo + (sum (snd <$> wdrls)) - deposits + refunds
+ let spendingBalance = vplus spendingBalanceUtxo (vinject $ (sum (snd <$> wdrls)) - deposits + refunds)
+ n = if (Map.size . unUTxO) utxo < 100 then 3 else 0


we could add this 100 to the constants or reuse another constant...

JaredCorduan · 2020-08-27T17:21:16Z

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

+ genWrdls (Map.toList wdrls)
+ )
+ ]
+ pure ( a, b)


another one that didn't change?

JaredCorduan · 2020-08-27T17:21:51Z

...and-ledger/shelley-spec-ledger-test/src/Test/Shelley/Spec/Ledger/Serialisation/Generators.hs

+ MD.I <$> arbitrary,
+ MD.B <$> arbitrary,
+ MD.S <$> (T.pack <$> arbitrary)
+ ]


just indendation?

JaredCorduan · 2020-08-27T17:22:13Z

...and-ledger/shelley-spec-ledger-test/src/Test/Shelley/Spec/Ledger/Serialisation/Generators.hs

+  RequireAllOf <$> resize maxMultiSigListLens (listOf (sizedMultiSig (n-1))),
+  RequireAnyOf <$> resize maxMultiSigListLens (listOf (sizedMultiSig (n-1))),
+  RequireMOf <$> arbitrary <*> resize maxMultiSigListLens (listOf (sizedMultiSig (n-1)))
+  ]


just indentation

JaredCorduan · 2020-08-27T17:23:19Z

...er/shelley-spec-ledger-test/src/Test/Shelley/Spec/Ledger/Serialisation/Generators/Genesis.hs

 import Test.Shelley.Spec.Ledger.Utils (mkHash)

-genShelleyGenesis :: Crypto c => Gen (ShelleyGenesis c)
+-- TODO this is temporary until I look through the Hedgehog docs


do we need this now?

JaredCorduan

Thank you for this massive undertaking!! All in all it looks good to me, but I left several comments, but it's all small stuff.

…not consistent between different parts of the generated code. Slight refactoring to make the changes widely useable.

This commit only adds the class and an instance for `Coin`. It differs from #1815 in the following ways: - Class methods are now intended for qualified import (as per Michael's comment in #1803) - Added some additional documentation. Some functions still need significantly more documentation. - Use the `Semigroup`, `Monoid` and `Group` superclasses. This removes the need for various functions defined in this class. - Removed the instance for `Integer` and define the instance for `Coin` directly.

Introduce `Val` class from #1815

This builds upon PR #1830, so the first commit should be reviewed in that PR. Adds the `Value` type for Shelley-MA lifted from PR #1815 (and adjusted following changes in the ts/eras branch). Some changes are needed: - The documentation is lifted from Plutus and is incorrect in some detail; it should be updated. - Size computation should be checked.

TimSheard requested review from JaredCorduan and uroboros as code owners August 27, 2020 15:34

Replacing coin with value.

9c272d8

TimSheard force-pushed the fixed-point-gentx branch from 004a462 to 9c272d8 Compare August 27, 2020 16:14

JaredCorduan reviewed Aug 27, 2020

View reviewed changes

TimSheard added 2 commits August 27, 2020 14:12

wip, trackdown missing key

93ef777

Fixed the breaking benchmarks. Break was caused because genState was …

c769d68

…not consistent between different parts of the generated code. Slight refactoring to make the changes widely useable.

nc6 mentioned this pull request Sep 2, 2020

Introduce Val class from #1815 #1829

Merged

polinavino added a commit that referenced this pull request Sep 2, 2020

Merge pull request #1829 from input-output-hk/nc/value

9dd938b

Introduce `Val` class from #1815

nc6 mentioned this pull request Sep 3, 2020

Shelley-MA value type #1831

Merged

JaredCorduan mentioned this pull request Sep 16, 2020

Optimise (dom (delegs ▷ Set.singleton hk) ◁ stake) #1857

Merged

JaredCorduan closed this Sep 18, 2020

redxaxder deleted the fixed-point-gentx branch April 20, 2021 17:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Parameterize over what it means to be an Asset (Coin v.s. Value) #1815

Parameterize over what it means to be an Asset (Coin v.s. Value) #1815

TimSheard commented Aug 27, 2020

JaredCorduan Aug 27, 2020

JaredCorduan Aug 27, 2020

JaredCorduan Aug 27, 2020

JaredCorduan Aug 27, 2020

JaredCorduan Aug 27, 2020

JaredCorduan Aug 27, 2020

JaredCorduan Aug 27, 2020

polinavino Aug 27, 2020

JaredCorduan Aug 27, 2020

polinavino Aug 27, 2020

redxaxder Aug 31, 2020

JaredCorduan Aug 27, 2020

JaredCorduan Aug 27, 2020

JaredCorduan Aug 27, 2020

JaredCorduan Aug 27, 2020

JaredCorduan Aug 27, 2020

mrBliss Aug 27, 2020

JaredCorduan Aug 27, 2020

JaredCorduan Aug 27, 2020

JaredCorduan Aug 27, 2020

JaredCorduan Aug 27, 2020

JaredCorduan Aug 27, 2020

JaredCorduan Aug 27, 2020

JaredCorduan Aug 27, 2020

JaredCorduan Aug 27, 2020

JaredCorduan Aug 27, 2020

JaredCorduan Aug 27, 2020

JaredCorduan Aug 27, 2020

JaredCorduan Aug 27, 2020

JaredCorduan Aug 27, 2020

JaredCorduan left a comment

Parameterize over what it means to be an Asset (Coin v.s. Value) #1815

Parameterize over what it means to be an Asset (Coin v.s. Value) #1815

Conversation

TimSheard commented Aug 27, 2020

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

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

JaredCorduan left a comment

Choose a reason for hiding this comment