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

Redefine Coin in terms of Natural and remove the Bounded instance. #3034

Merged
merged 20 commits into from
Nov 24, 2021

Conversation

jonathanknowles
Copy link
Contributor

@jonathanknowles jonathanknowles commented Nov 23, 2021

Issue Number

ADP-1283

Summary

This PR:

  • Redefines Coin in terms of Natural (rather than Word64).
  • Removes the Bounded instance for Coin.
  • Adds txOut{Min,Max}Coin constants to Primitive.Types.Tx, in the same style as the existing txOut{Min,Max}TokenQuantity constants.
  • Pushes validation checks for coins that must be within a certain range to the places where those checks are actually required.

Implementation Notes

Where possible, this PR uses intCast and intCastMaybe (instead of fromIntegral) to perform statically-checked integral conversations.

Motivation

The wallet uses the Coin type to represent an amount of lovelace.

It's currently defined in terms of Word64, and has a Bounded instance that limits the range to [0, 45_000_000_000_000_000].

However, this approach is problematic, for several reasons:

  • Boundary checks for Coin values make sense in contexts where there are boundaries. However, for pure Coin values in the absence of any particular context, there isn't one particular upper bound that makes sense.
  • The current choice of Bounded specifically defines the limits of what can be included in a transaction output. However, encoding values that appear in transaction outputs is not the only use of the Coin type.
  • For example, it's quite reasonable to use Coin for other purposes, like finding the total volume of ada transacted over some period of time. For such usages, there is no obvious choice of upper limit that we could consider “ideal”.
  • Right now, we often use awkward workarounds such as casting Coin values to Natural values before computing summations, thus losing the descriptive value of using the Coin type to mark that “this is a quantity of lovelace”.
  • Given that the default data constructor for Coin is exported, it’s possible to directly construct “invalid” Coin values from outside the Coin module.
  • Using the existing Semigroup instance, we can combine valid Coin values into values that are invalid, which can lead to unexpected run-time errors when those “invalid” values are passed to functions that we believe to be safe. 💣

For an example of the last problem, consider this:

> :set -XNumericUnderscores
> :set -XOverloadedLists
> import qualified Cardano.Wallet.Primitive.Types.Coin as Coin
> coinA = Coin 45_000_000_000_000_000
> coinB = coinA <> coinA
> coinB
Coin 90000000000000000
> Coin.equipartition coinB [()]
*** Exception: unsafeNaturalToCoin: overflow
CallStack (from HasCallStack):
  error, called at /home/jsk/projects/input-output-hk/cardano-wallet-master/lib/core/src/Cardano/Wallet/Primitive/Types/Coin.hs:131:37 in main:Cardano.Wallet.Primitive.Types.Coin

@jonathanknowles jonathanknowles self-assigned this Nov 23, 2021
@jonathanknowles jonathanknowles force-pushed the jonathanknowles/make-coin-great-again branch 2 times, most recently from a183849 to 25434b2 Compare November 23, 2021 13:51
Copy link
Member

@Anviking Anviking left a comment

Choose a reason for hiding this comment

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

LGTM, I think the direction makes sense. (And thanks for tackling this!) I left a few comments from going through the commits start to finish, once.

computeUtxoStatistics btype =
computeStatistics (pure . unCoin . txOutCoin) btype . Map.elems . unUTxO
computeUtxoStatistics btype
= computeStatistics (pure . fromIntegral . unCoin . txOutCoin) btype
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
= computeStatistics (pure . fromIntegral . unCoin . txOutCoin) btype
= computeStatistics (pure . Coin.unsafeToWord64 . txOutCoin) btype

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, well spotted! I'll fix this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 0b95b17.

, minimumCoins = coinQuantity <$> minCoins
, deposit = coinQuantity $ fromMaybe (Coin 0) mDeposit
, minimumCoins = Coin.unsafeToQuantity <$> minCoins
, deposit = Coin.unsafeToQuantity $ fromMaybe (Coin 0) mDeposit
Copy link
Member

Choose a reason for hiding this comment

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

We're converting to a Natural, so this ends up being safe. Can't we use Coin.toNatural instead?

Same for minimumCoins.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point!

I've fixed this in 1937bf7.

lib/core/src/Cardano/Wallet/Primitive/Types/Coin.hs Outdated Show resolved Hide resolved
return x
| otherwise =
Left $ TextDecodingError "Coin value is out of bounds"
fromText = fmap Coin . fromText @Natural
Copy link
Member

Choose a reason for hiding this comment

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

This seems like a slight change of behaviour in the CLI https://github.com/input-output-hk/cardano-wallet/blob/a1838496ba697208daf2889ffcd420bea4d1b0a5/lib/cli/src/Cardano/CLI.hs#L1408-L1409

but I guess it makes sense — stake is not related to a single TxOut ada value.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. I agree this should be acceptable. As you mention, this is unrelated to a transaction output.

If stake needs to be constrained further, then we could perhaps make a wrapper type that constrains the value further. Thoughts?

coinIsValidForTxOut :: Coin -> Bool
coinIsValidForTxOut c = (&&)
(c >= txOutMinCoin)
(c <= txOutMaxCoin)
Copy link
Member

Choose a reason for hiding this comment

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

Are we sure we couldn't have e.g. a 46B ada TxOut on a custom testnet?

I find this hard-coded value in the ledger specs
https://github.com/input-output-hk/cardano-ledger/blob/4347e54b2827090e898702ccfb93cc731b4a1909/eras/byron/ledger/impl/src/Cardano/Chain/Common/Lovelace.hs#L176, but it is for Byron.

So I guess you could setup a single-era cluster post-Byron with a different max supply. I presume this is what would calculate it: https://github.com/input-output-hk/cardano-ledger/blob/4347e54b2827090e898702ccfb93cc731b4a1909/eras/shelley/impl/src/Cardano/Ledger/Shelley/Genesis.hs#L434

For the proper Cardano mode (all eras), I suspect 45B still holds, though. So this might not be the most important thing to care about even if true.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree that we may want to avoid hard-coding the minimum and maximum value for Coin — they are actually configured in the Shelley genesis. (In fact, our local cluster has a higher maximum Ada quantity than 45B.)

In fact, I believe that we can remove txOutMaxCoin entirely — checking the maximum bound will never catch any user mistakes that will not manifest in a different way (e.g. inability to find inputs with sufficient funds).

When discussing this, Jonathan suggested to make a second PR in order to keep the scope of this PR to a refactoring (as opposed to a change in functionality).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Anviking I completely agree with your observation. This value could be different in custom testnets, so hard-coding it within the wallet primitive layer is of questionable value.

Though as @HeinrichApfelmus mentioned, it was my aim to avoid changing any behaviour in this PR.

I think we could/should consider making a ticket to capture this issue. I'll try to do that today.

@@ -531,7 +531,7 @@ prefilterBlock b u0 = runState $ do

(Nothing, Outgoing) -> -- Byron
let
totalOut = sumCoins (txOutCoin <$> outputs tx)
totalOut = F.fold (txOutCoin <$> outputs tx)
Copy link
Member

Choose a reason for hiding this comment

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

I appreciate the clarity of the name sumCoins over F.fold, but I won't per-se object with this change either.

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 appreciate the clarity of the name sumCoins over F.fold, but I won't per-se object with this change either.

I think my (not very strongly felt) objection to this stems from the fact that sumCoins seems specialized, but in reality it folds over the coins in a very similar way to how F.fold adds with the Semigroup and Monoid instances, so it seems a bit superfluous.

But my objection is not a strong one! I would prefer the name sum though, if we were to retain it, since the current API of Coin is intended to be imported in qualified fashion.

Comment on lines +276 to +282
[ (1, pure txOutMinCoin)
, (1, pure txOutMaxCoin)
, (8, Coin.fromNatural <$> chooseNatural
( Coin.toNatural txOutMinCoin + 1
, Coin.toNatural txOutMaxCoin - 1
)
)
Copy link
Member

Choose a reason for hiding this comment

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

Would it be an idea to do something like this instead?

n <- fromIntegral . getNonNegative <$> arbitrary @Int :: Natural
frequency
  [ (1, Coin.fromNatural $ txOutMinCoin + n)
  , (1, Coin.fromNatural $ txOutMaxCoin - n)
  , (8, Coin.fromNatural <$> chooseNatural (min, max))
  ] 

Motivation:

  • More coverage near boundaries
  • Manually excluding the boundary cases from the full-range chooseNatural seems pointless

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 think we could definitely consider that!

Though perhaps in a separate PR?

(My aim with this PR was to keep it as a pure refactoring, as much as possible.)

newtype Coin = Coin
{ unCoin :: Word64
{ unCoin :: Natural
}
deriving stock (Ord, Eq, Generic)
deriving (Read, Show) via (Quiet Coin)

instance Semigroup Coin where
Copy link
Contributor

Choose a reason for hiding this comment

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

One of the most important things about the Coin abstraction is that it has a Semigroup and a Monoid instance which correspond to summation. Could you add corresponding comments in the Haddock documentation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point!

I've added some comments in 535d0dd.

coinIsValidForTxOut :: Coin -> Bool
coinIsValidForTxOut c = (&&)
(c >= txOutMinCoin)
(c <= txOutMaxCoin)
Copy link
Contributor

Choose a reason for hiding this comment

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

I agree that we may want to avoid hard-coding the minimum and maximum value for Coin — they are actually configured in the Shelley genesis. (In fact, our local cluster has a higher maximum Ada quantity than 45B.)

In fact, I believe that we can remove txOutMaxCoin entirely — checking the maximum bound will never catch any user mistakes that will not manifest in a different way (e.g. inability to find inputs with sufficient funds).

When discussing this, Jonathan suggested to make a second PR in order to keep the scope of this PR to a refactoring (as opposed to a change in functionality).

@@ -61,6 +61,7 @@ library
, filepath
, fmt
, generic-lens
, int-cast
Copy link
Contributor

Choose a reason for hiding this comment

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

We use a bewildering variety of packages. At some point, we may want to document and consolidate our choices, so that writing mundane code becomes more streamlined (e.g. Text vs ByteString vs lazy ByteString; converting to/from hex). 😅 But I do not believe that the variety really hurts readability, so the status quo is fine as well.

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 do agree we use a very large variety of packages!

Though to some extent, perhaps this reflects the fragmentation and/or modularity within the Haskell package ecosystem?

In this case, I believe the int-cast package is rather well-designed and very useful for catching errors. We actually already do use it within cardano-wallet: this just adds it to some of the other build targets.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ByteString vs lazy ByteString

To some extent, I think our hand is forced here by the packages we rely on.

Historically, Aeson has used lazy ByteString by default. Though I've noticed that strict variants have recently been added for the most common functions.

jonathanknowles added a commit that referenced this pull request Nov 24, 2021
jonathanknowles added a commit that referenced this pull request Nov 24, 2021
jonathanknowles added a commit that referenced this pull request Nov 24, 2021
@jonathanknowles
Copy link
Contributor Author

bors r+

iohk-bors bot added a commit that referenced this pull request Nov 24, 2021
3032: Remove redundant imports and constraints. r=jonathanknowles a=jonathanknowles

## Issue Number

None

## Summary

This PR removes some redundant imports and constraints.

I collected these patches while working on other things. It seemed sensible to put them in their own PR.

3034: Redefine `Coin` in terms of `Natural` and remove the `Bounded` instance. r=jonathanknowles a=jonathanknowles

## Issue Number

ADP-1283

## Summary

This PR:
- Redefines `Coin` in terms of `Natural` (rather than `Word64`).
- Removes the `Bounded` instance for `Coin`.
- Adds `txOut{Min,Max}Coin` constants to `Primitive.Types.Tx`, in the same style as the existing `txOut{Min,Max}TokenQuantity` constants.
- Pushes validation checks for coins that must be within a certain range to the places where those checks are actually required.

## Implementation Notes

Where possible, this PR uses `intCast` and `intCastMaybe` (instead of `fromIntegral`) to perform statically-checked integral conversations.

## Motivation

The wallet uses the `Coin` type to represent an amount of _lovelace_.

It's currently defined in terms of `Word64`, and has a `Bounded` instance that limits the range to `[0, 45_000_000_000_000_000]`.

However, this approach is problematic, for several reasons:

- Boundary checks for `Coin` values make sense in contexts where there are boundaries.  However, for pure `Coin` values in the absence of any particular context, there isn't one particular upper bound that makes sense.
- The current choice of `Bounded` specifically defines the limits of what can be included in a _transaction output_. However, encoding values that appear in transaction outputs is not the only use of the `Coin` type.
- For example, it's quite reasonable to use `Coin` for other purposes, like finding the total volume of ada transacted over some period of time. For such usages, there is no obvious choice of upper limit that we could consider “ideal”.
- Right now, we often use awkward workarounds such as casting `Coin` values to `Natural` values before computing summations, thus losing the descriptive value of using the `Coin` type to mark that “this is a quantity of lovelace”.
- Using the existing `Semigroup` instance, we can combine valid `Coin` values into values that are **_invalid_**.  💣 

Co-authored-by: Jonathan Knowles <[email protected]>
Co-authored-by: IOHK <[email protected]>
@iohk-bors
Copy link
Contributor

iohk-bors bot commented Nov 24, 2021

Build failed (retrying...):

Looks like a few integration test failures relating to this PR. I'll investigate and fix.

#expected

@jonathanknowles
Copy link
Contributor Author

bors r-

@iohk-bors
Copy link
Contributor

iohk-bors bot commented Nov 24, 2021

Canceled.

jonathanknowles added a commit that referenced this pull request Nov 24, 2021
It turns out that on our integration testnet, the minimum quantity of lovelace
allowed in a transaction output is 0.

Hence we lower the `txOutMinCoin` constant to `Coin 0`.

In response to:

#3034 (comment)
@jonathanknowles
Copy link
Contributor Author

bors r+

iohk-bors bot added a commit that referenced this pull request Nov 24, 2021
3034: Redefine `Coin` in terms of `Natural` and remove the `Bounded` instance. r=jonathanknowles a=jonathanknowles

## Issue Number

ADP-1283

## Summary

This PR:
- Redefines `Coin` in terms of `Natural` (rather than `Word64`).
- Removes the `Bounded` instance for `Coin`.
- Adds `txOut{Min,Max}Coin` constants to `Primitive.Types.Tx`, in the same style as the existing `txOut{Min,Max}TokenQuantity` constants.
- Pushes validation checks for coins that must be within a certain range to the places where those checks are actually required.

## Implementation Notes

Where possible, this PR uses `intCast` and `intCastMaybe` (instead of `fromIntegral`) to perform statically-checked integral conversations.

## Motivation

The wallet uses the `Coin` type to represent an amount of _lovelace_.

It's currently defined in terms of `Word64`, and has a `Bounded` instance that limits the range to `[0, 45_000_000_000_000_000]`.

However, this approach is problematic, for several reasons:

- Boundary checks for `Coin` values make sense in contexts where there are boundaries.  However, for pure `Coin` values in the absence of any particular context, there isn't one particular upper bound that makes sense.
- The current choice of `Bounded` specifically defines the limits of what can be included in a _transaction output_. However, encoding values that appear in transaction outputs is not the only use of the `Coin` type.
- For example, it's quite reasonable to use `Coin` for other purposes, like finding the total volume of ada transacted over some period of time. For such usages, there is no obvious choice of upper limit that we could consider “ideal”.
- Right now, we often use awkward workarounds such as casting `Coin` values to `Natural` values before computing summations, thus losing the descriptive value of using the `Coin` type to mark that “this is a quantity of lovelace”.
- Using the existing `Semigroup` instance, we can combine valid `Coin` values into values that are **_invalid_**.  💣 

Co-authored-by: Jonathan Knowles <[email protected]>
Co-authored-by: IOHK <[email protected]>
@jonathanknowles
Copy link
Contributor Author

bors r-

@iohk-bors
Copy link
Contributor

iohk-bors bot commented Nov 24, 2021

Canceled.

The maximum size of a `Coin` value is context dependent.

This commit adds constants that define the limits of a `Coin` value in
the context of a transaction output.
This function is unsafe because `Coin` is now defined in terms of `Natural`.

We replace this function with a pair of functions:

- Coin.toQuantity        :: Integral i => Coin -> Maybe (Quantity n i)
- Coin.unsafeToQuantity  :: Integral i => Coin ->        Quantity n i
We use `intCast` instead of `fromIntegral`, which statically checks
that the conversion is safe.

We also use the same naming convention as the existing safe conversion
functions.
This conversion is no longer unsafe. So we create a safe conversion
function and place it within the "safe conversions" section.
We can now just rely on the `FromText` instance for `Natural`.
Boundary checks for `Coin` values make sense in contexts where there
are boundaries.

However, for pure `Coin` values in the absence of any particular
context, there isn't one particular upper bound that makes sense.

In the particular case of a transaction output, `Coin` values must be
within the inclusive range `[Coin 1, Coin 45_000_000_000_000_000]`.
jonathanknowles and others added 10 commits November 24, 2021 07:31
The `Coin` type has both `Semigroup` and `Monoid` instances.

Therefore we can just use `Data.Foldable.fold` to sum coins.
…dle}`.

These generators are specifically designed to generate values across the
full range of what's permitted to appear in a transaction output.

Therefore, we rename them to have the `genTxOut` prefix, and we move
them to the `Tx.Gen` module.
Instead, we use `txOutMinCoin` and `txOutMaxCoin` where appropriate.

These usages are similar to `txOut{Min,Max}TokenQuantity`.
Since `Coin` is typically imported in a qualified fashion, we can write
`Coin.add` instead of `Coin.addCoin`.

This change also redefines `<>` for `Coin` in terms of `Coin.add`.
Since `Coin` is typically imported in a qualified fashion, we can write
`Coin.subtract` instead of `Coin.subtractCoin`.
It turns out that on our integration testnet, the minimum quantity of lovelace
allowed in a transaction output is 0.

Hence we lower the `txOutMinCoin` constant to `Coin 0`.

In response to:

#3034 (comment)
@jonathanknowles jonathanknowles force-pushed the jonathanknowles/make-coin-great-again branch from 73d8c42 to 7f44139 Compare November 24, 2021 07:31
Due to changes in the definitions of generators, it's become necessary
to call `QC.resize` with a higher value in order to generate a range of
token bundle sizes that straddles the boundary between being "too small"
and "too large".
@jonathanknowles
Copy link
Contributor Author

Due to problems with hydra, I plan to merge this PR manually. 🔧

I have confirmed that the following test suites pass when run locally (for commit a8d68a3):

  • cardano-wallet-core:test:unit
  • cardano-wallet:test:unit
  • cardano-wallet:test:integration

I have run each test suite twice to guard against spurious test failures.

@jonathanknowles jonathanknowles merged commit 428df13 into master Nov 24, 2021
@jonathanknowles jonathanknowles deleted the jonathanknowles/make-coin-great-again branch November 24, 2021 10:33
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.

3 participants