This repository has been archived by the owner on Feb 9, 2021. It is now read-only.
Change representation of Lovelace from Word64 to Natural #693
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
dcoutts
requested review from
kantp,
nc6,
redxaxder,
kevinhammond and
JaredCorduan
dcoutts
force-pushed
the
dcoutts/lovelaceportion-simplification
branch
from
1563f93 to
9477c2a
Compare
dcoutts
force-pushed
the
dcoutts/lovelace-as-natural
branch
2 times, most recently
from
d0bb48f to
32cbe41
Compare
The use of double in lovelacePortionToDouble is highly dubious.
We will use it instead of applyLovelacePortionDown which will help us eliminate unnecessary uses of LovelacePortion.
Change uses of LovelacePortion to Rational. The GenesisInitializer was using LovelacePortion for calculations for what portion of all the money goes to the "richmen" vs everyone else. This is unnecessary. We can simply use Rational, and use a function for scaling a Lovelace by a Rational. LovelacePortion is a (fixed base) representation of a rational in the range [0..1] that we inherited for Byron compatability for the thresholds for (mostly unused) update system protocol paramaters. We should avoid using it for anything else.
Now unused.
With the intention of replacing the other constructors and conversions.
Replace with rationalToLovelacePortion
Use lovelacePortionToRational instead. It was being used in applying an update system threshold paramater to the number of genesis keys to determine the threshold required. Since this was being used with the number of genesis keys, rather than total system stake, then the use of Double was not in practice a problem, but it's much clearer to stick to our rule that we avoid floating point completely within the ledger and consensus rules.
Just in the pretty-printing. We can use lovelacePortionToRational and convert for display.
Same for the LovelacePortion constructor directly. All replaced with rationalToLovelacePortion.
Removed mkLovelacePortion, mkKnownLovelacePortion and lovelacePortionToDouble. Also stop exporting lovelacePortionDenominator and the LovelacePortion consructor. So now we only export the LovelacePortion, the conversions to/from Rational, and of course the various instances.
In the JSON and CBOR decoders.
dcoutts
force-pushed
the
dcoutts/lovelaceportion-simplification
branch
from
9477c2a to
5fb1f2a
Compare
We will be able to eliminate a lot of the complexity involved in manipulating Lovelace values by making most of the operations total, where currently they are partial. The first step is changing the internal representation, without changing anything else. So it still does the overflow checks at this point.
Since soon it will not be bounded above. Mainly changes in the genesis generator and tests.
The representation now cannot overflow since it uses Natural. Stop exporting the LovelaceError constructors for the errors that are no longer possible. Remove the tests that check that overflows are detected.
Monoid with + and 0.
These will be used to replace all the others, especially the partial ones.
Legacy from cardano-sl that is no longer used anywhere.
Replace with lovelaceToNatural and naturalToLovelace.
Replace with naturalToLovelace.
We can do this now that we're using value level representations.
The add, sum, scale, div and mod operations are now pure, but not sub. This allows a small cascade of other things to become pure. Also remove properties for partiality of the operations that are no longer pure, and lovelace failures that are no longer possible.
Adjust the types of a couple fields in the GenesisInitializer and FakeAvvmOptions to make the types in the genesis generator match up better with fewer conversions.
Including removing unsafeGetLovelace and mkLovelace (which were already not exported but were still being used internally). Also rename Lovelace newtype projection function from getLovelace to unLovelace.
There is only one error for subtraction, it doesn't need a special error enumeration. Adjust the few users. This leaves LovelaceError unused in the Lovelace API and only used in the reporting of tx validation errors.
And improve the error messages at the same time. LovelaceErrors are no longer generated by the lovelace operations, after previous refactoring steps they were being thrown locally. Replace the general use of LovelaceError with a local error that is more specific. The error is really that one quanity was expected to be bigger than another and it was not.
In the next couple steps we will eliminate the type entirely. This is just an intermediate step. Also adjust tests.
It is now only used in one case, for when the input and output balances of the transaction do not match (which corresponds with when doing the lovelace subtraction operation to calculate the fee would fail). The binary format is retained for compatability. We rename the constructor to reflect its use. We eliminate the unused field. The values fields are changed to Lovelace (but encoded as Word64).
The CompactTxOut type used to contain simply a Lovelace, which was fine when it was a newtype wrapper over Word64, but when we made Lovelace a Natural then it is no longer so small. So we add and use a CompactLovelace type. Why is this OK? Because the sum of the whole utxo will fit in a Word64.
For when we want just one component of the CompactTxOut without uncompacting the other (e.g. computing the utxo balance).
dcoutts
force-pushed
the
dcoutts/lovelace-as-natural
branch
from
32cbe41 to
4061b61
Compare
kantp
approved these changes
Jan 6, 2020
| (fromCompactLovelace lovelace) | ||
|
|
||
| toCompactLovelace :: Lovelace -> CompactLovelace | ||
| toCompactLovelace = CompactLovelace . fromIntegral . lovelaceToNatural |
There was a problem hiding this comment.
Shouldn't we have a check here for overflow? This is where we are converting from Natural to Word64, right?
JaredCorduan
approved these changes
Jan 6, 2020
nc6
force-pushed
the
dcoutts/lovelaceportion-simplification
branch
from
5fb1f2a to
700181d
Compare
|
Uh, dunno why bors closed this, but it should still be open. I cannot reopen because it's against a closed branch, but also cannot retarget it because it's closed. |
|
Hah, ok, undeleted the target branch, now I can reopen... |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The motivation is that by changing most of the arithmetic operations from being partial to total it simplifies
Lovelacecalculations in many places elsewhere in the code.Also do a bunch of other cleaning and tidying.
This is still a draft because having changed the representation, we need to change the representation of a "compact" utxo entry to still use
Word64, otherwise we'll inflate their size with the change toNatural(which cannot be unpacked since it's a two-constructor type). So that'll need another patch to adjust the compact utxo representation, and add a validation check that each tx output value is less than 2^64 so that the conversion is guaranteed (this should be a redundant check since the CBOR on-chain tx representation is already a word64).This PR builds on #692, and should probably be re-targeted to
masteronce that one is merged.