Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[vesting] rewrite the private sale module #75

Merged
merged 17 commits into from
Jan 20, 2023

Conversation

devfull
Copy link
Contributor

@devfull devfull commented Aug 24, 2022

Closes #74

-- | Utxo entry size calculation
utxoEntrySize :: Value -> HasDatumHash -> Integer
utxoEntrySize value datum =
utxoEntrySizeWithoutVal + Value.size (toMaryValue value) + datumHashSize datum
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tu mets explicitement toMaryValue, y a-t-il une notion de rétrocompatibilité à prendre en compte également ? quel impact pour les futurs versions ? faudra-t-il changer cette fonction ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This calculation is specific to the Alonzo era, as described in : https://github.com/input-output-hk/cardano-ledger/blob/2c203ead68913d660389f04a705c734c155c2a35/doc/explanations/min-utxo-alonzo.rst

The drawback of patching fees or minUTxO calculations is that those pieces of code are very unstable and will require maintenance. The only intent is to avoid generating a valid Address to build a TxOut for doing tests without IO as explained in the issue #74. We should use the original functions when appropriate.

Comment on lines +50 to +55
datumHashSize :: HasDatumHash -> Integer
datumHashSize NoDatumHash = 0
datumHashSize WithDatumHash = 10

utxoEntrySizeWithoutVal :: Integer
utxoEntrySizeWithoutVal = 27
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cardano est très "paramétrisable" dans le sens où on peut jouer avec les protocol-parameters sans avoir à sortir une nouvelle version du noeud, ces utxo et datumhash sizes font-elles parti de ces fameux paramètres ? le cas échéant, faut-il regrouper dans un fichier de conf les paramètres ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The utxoEntrySizeWithoutVal and dataHashSize are not protocol parameters but lengths obtained experimentally. The documentation refers to them as "current constants" :

Note that the coinsPerUTxOWord is a protocol parameter and is subject to change. The values utxoEntrySizeWithoutVal and dataHashSize (dh) are fixed at least for the entire Alonzo era.

deriving stock (Show)

-- | Utxo entry size calculation
utxoEntrySize :: Value -> HasDatumHash -> Integer
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

la maj vasil introduit les inline datum qui peuvent impacter ce calcul
https://cips.cardano.org/cips/cip32/

Comment on lines +68 to +81
ShelleyBasedEraShelley -> protocolParamMinUTxOValue
ShelleyBasedEraAllegra -> calculateMinimumUTxOAllegraMary
ShelleyBasedEraMary -> calculateMinimumUTxOAllegraMary
ShelleyBasedEraAlonzo -> calculateMinimumUTxOAlonzo
where
calculateMinimumUTxOAllegraMary :: Maybe Lovelace
calculateMinimumUTxOAllegraMary =
calcMinimumDeposit value
<$> protocolParamMinUTxOValue

calculateMinimumUTxOAlonzo :: Maybe Lovelace
calculateMinimumUTxOAlonzo =
(Lovelace (utxoEntrySize value datumHash) *)
<$> protocolParamUTxOCostPerWord
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

si je comprends bien on devra mettre ça à jour à chaque fois qu'on entre dans une nouvelle era qui touche à ce calcul ?

<$> protocolParamUTxOCostPerWord

-- | Calculate minimumUTxO with default protocol parameters from a Value
calculateDefaultMinimumUTxOFromValue ::
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

quelle est la motivation derrière ces fonctions default ? ou plutôt pourquoi avons-nous besoin des fonctions "non default" ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PParams in each era have a default instance, basically setting all fields to empty values. The non-default calculate* functions take a ProtocolParameters as argument. Those parameters could be retrieved from disk with IO. But given that the calculation use only one parameter in each era that is not susceptible to change, we can provide another default updated with the hardcoded needed value, and avoid doing IO in the process. The default functions are based on the non-default ones to give a clear design and to allow more generic calculations.

defaultCalculateMinimumUTxOParams era

-- | Calculate minimumUTxO for a singleton Value from AssetId
calculateMinimumUTxOFromAssetId ::
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ce cas particulier est-il nécessaire ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the simplest way to call the calculation in the GenerateNative module for example, to encompass with a single call the same minUTxO for all singleton values based on the given AssetClass.

validatePrivateSale PrivateSale{..} =
    let proportions = TranchesProportions $ proportion <$> tranchesProperties
    in
        liftEither $ do
            ε <- calculateDefaultMinimumUTxOFromAssetClass assetClass

)

import PlutusTx.Builtins ( fromBuiltin )
import Plutus.V1.Ledger.Ada ( adaSymbol, adaToken )
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

on a pas mal de références à Plutus.V1, je ne me rends pas compte de ce que va apporter Plutus.V2 comme modifications. Je crois que V2 arrive avec Vasil si je ne me trompe pas, à moins que ce soit déjà déployé.

@@ -0,0 +1,42 @@
{-# LANGUAGE ImportQualifiedPost #-}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

besoin de faire un point sur la gestion de ces paramètres de manière générale dans tokenomia

Comment on lines +12 to +16
unLovelace :: Lovelace -> Integer
unLovelace (Lovelace l) = l

unQuantity :: Quantity -> Integer
unQuantity (Quantity l) = l
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

je ne sais pas pour unQuantity mais il me semble que unLovelace existe déjà

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

, assetClassToJSON
) where

import Control.Arrow ( (***) )
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

c'est quoi ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I should have used bimap instead.

Comment on lines +18 to +32
assetClassFromJSON :: Value -> Parser AssetClass
assetClassFromJSON = withObject "AssetClass" $ \o ->
assetClass
<$> (fromString <$> o .: "currencySymbol")
<*> (fromString <$> o .: "tokenName")

-- | Alternative AssetClass Value to the ToJSON instance
assetClassToJSON :: AssetClass -> Value
assetClassToJSON x =
let (currencySymbol, tokenName) = (show *** toString) $ unAssetClass x
in
object
[ "currencySymbol" .= currencySymbol
, "tokenName" .= tokenName
]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

les toJSON avec les lenses faisaient pas le taf ?

@@ -0,0 +1,86 @@
{-# LANGUAGE DerivingStrategies #-}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tu ne mets pas le repertoire Arbitrary avec ce qui est lié aux tests ?

@@ -0,0 +1,86 @@
{-# LANGUAGE DerivingStrategies #-}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tu ne mets pas le repertoire Arbitrary avec ce qui est lié aux tests ?

Comment on lines +34 to +36
instance Arbitrary CurrencySymbol where
arbitrary = CurrencySymbol <$> arbitrary
shrink x = CurrencySymbol <$> shrink (unCurrencySymbol x)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tu me conseilles de regarder la doc de quickcheck pour comprendre un peu les arbitrary et les shrinks ?

) where


newtype Restricted a
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

comment ça marche ? tu lui passe genre Restricted Positive et ça dit à quickcheck que tu veux que des valeurs positives ?

Comment on lines +30 to +39
instance Arbitrary (Restricted Value) where
arbitrary =
Restricted . mconcat
<$> uncurry assetClassValue <$$> gen
where
gen :: Gen [(AssetClass, Integer)]
gen =
zip
<$> (getRestricted <$$> Set.toList <$> arbitrary)
<*> (getPositive <$$> arbitrary)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

curieux de comprendre un peu plus en détail comment ça marche :)

@@ -0,0 +1,205 @@
{-# LANGUAGE DerivingStrategies #-}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

merde j'y pense que maintenant mais j'espère que t'as pas eu à refaire tout ça alors que ça se trouve peut être là dedans https://github.com/input-output-hk/cardano-wallet

@@ -0,0 +1,38 @@
#!/bin/bash
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

potentiellement remplacé complètement le binaire par l'intégration directe de la lib haskell https://github.com/input-output-hk/cardano-addresses

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@@ -0,0 +1,205 @@
{-# LANGUAGE DerivingStrategies #-}
{-# LANGUAGE ImportQualifiedPost #-}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

faudrait qu'on se mette au clair tous les formats utilisés dans cardano avec les key, les hash, le bech32, cbor,...
je me suis pas mal cassé les dents sur les conversions en jouant avec la lib typescript

Comment on lines +18 to +22
para :: (a -> [a] -> b -> b) -> b -> [a] -> b
para f z = go
where
go [] = z
go (x:xs) = f x xs (go xs)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

haha toujours plus compliqué :D
qu'est ce que c'est ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A paramorphism is a generalization of a catamorphism or a fold. A catamorphism takes a operator and a starting value, and reduces a list by applying this operator on each element and an accumulator. A paramorphism is more powerful in that the combining step function not only see the next element, but also the remaining elements as well.

This trick allows to implement mapLastWith effectively, by distinguishing whether the list to map on has more than on element, and apply a different function depending on the case.

@augucharles augucharles merged commit cbc3930 into mlabs/private-sale-staging Jan 20, 2023
@augucharles augucharles deleted the redesign-vesting branch January 20, 2023 15:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants