-
Notifications
You must be signed in to change notification settings - Fork 158
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
[DO NOT MERGE] Temporary Multiasset parametrization PR #1803
Conversation
… added vcoin, vinject and vsize to the type class Val 3. defined these for Value
…rdano-ledger-specs into polina-nonbreaking-ma
…Combinators.hs Examples.hs Init.hs
Examples.hs Examples/PoolLifetime.hs Examples/Updates.hs Fees.hs MultiSigExamples.hs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Size calculation seems somewhat plausible but it's hard to work out what's going on at the moment.
vcoin (Value c1 _) = c1 | ||
vinject c1 = Value c1 vzero | ||
vsize (Value _ v) = foldr accum uint v where -- add uint for the Coin portion in this size calculation | ||
accum u ans = foldr accumIns (ans + addrHashLen) u where -- add addrHashLen for each Policy ID |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: it always takes me ages to remember the arguments to fold functions. I highly recommend type signatures for all local functions, tbh.
shelley/chain-and-ledger/executable-spec/src/Shelley/Spec/Ledger/Value.hs
Show resolved
Hide resolved
visZero (Value c1 v1) = (visZero c1) && (visZero v1) | ||
vcoin (Value c1 _) = c1 | ||
vinject c1 = Value c1 vzero | ||
vsize (Value _ v) = foldr accum uint v where -- add uint for the Coin portion in this size calculation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd find this whole thing much easier to read if it was written with functions computing the sizes for the individual bits, even those are mostly const
. As it is, it's hard to tell whether everything is covered.
e.g. vsize (Value c v) = foldr accum (coinSize c) v where coinSize _ = uint
.
deriving (Show, Eq, ToCBOR, FromCBOR, Ord, NoUnexpectedThunks, NFData) | ||
|
||
-- | The Value representing MultiAssets | ||
data Value crypto = Value Coin (Map (PolicyID crypto) (Map AssetID Quantity)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this normalized so that all Ada must be in the Coin
part? Is the size calculation defined on the abstract type where we just have the map, or on this optimized version?
(Also, I thought we were only going to use the optimized one in specific places?)
utxoEntrySizeWithoutVal = inputSize + outputSizeWithoutVal | ||
|
||
-- This scaling function is right for UTxO, not EUTxO | ||
scaleVl :: (Val v) => v -> Coin -> Coin |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is about the deposit? It's quite hard to work out whether this is doing the right thing since it's fairly obscure what it's supposed to do.
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.
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.
Closing this, as it's been superseded at least twice. We can still reference the code review suggestions made. |
Temporary Multiasset parametrization PR -- to see if CI tests give same output as locally