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

Represent fungible asset values as a distinct type and support minor units #954

Merged
merged 5 commits into from
Aug 18, 2020

Conversation

dahlia
Copy link
Contributor

@dahlia dahlia commented Aug 12, 2020

This pull request adds Currency.DecimalPlaces property and FungibleAssetValue type to address the issue #944.

From this changes, fungible asset values are no more represented as BigInteger, which does not hold any decimal places, but FungibleAssetValue instead. The introduced FungibleAssetValue type is essentially a pair of (Currency, BigInteger). It cannot mix heterogeneous currencies. FungibleAssetValues can be made like below:

var gold = new Currency(ticker: "GOLD", decimalPlaces: 2, minter: null);
FungibleAssetValue value;

value = new FungibleAssetValue(gold, majorUnit: 123, minorUnit: 0);
// Or alternatively:
value = 123 * gold;
// Or:
value = gold * 123;

This patch also introduces a new property named DecimalPlaces to Currency. This defines how precise can currency values be, like 2 decimal places for cents of US Dollar. Continued from the above example, you can represent 123.45 GOLD like below:

var v = new FungibleAssetValue(gold, 123, 45);

On the other hand, it does not allow more decimal places than it's specified in DecimalPlaces. For example, the following code causes ArgumentException:

new FungibleAssetValue(gold, 123, 456);
// ArgumentException: Since the currency GOLD (dfc2eebfd2bfb1fc31d921d665d9210b9ea8ebb3) allows upto 2 decimal places, the given minor unit 456 is too big.

FungibleAssetValue type mimics other integral types like BigInteger; it provides +, -, *, %, .DivRem(), .Abs(), .Sign and so on, except for division operator (/) which is bug-prone IMHO. For the rationale, see also the XML comment on FungibleAssetValue struct.

In order to print the value, simply use .ToString() method or if you want only quantity without currency use .GetQuantityString() method.

Besides, this patch adds a new namespace Libplanet.Assets and Currency is also moved into that.

@dahlia dahlia added suggestion Suggestion or feature request storage Related to storage (Libplanet.Store) labels Aug 12, 2020
@dahlia dahlia self-assigned this Aug 12, 2020
@dahlia dahlia force-pushed the fungible-asset-value branch 3 times, most recently from afa4a9f to 2341ba9 Compare August 14, 2020 13:10
@codecov
Copy link

codecov bot commented Aug 14, 2020

Codecov Report

Merging #954 into main will increase coverage by 0.12%.
The diff coverage is 91.52%.

@@            Coverage Diff             @@
##             main     #954      +/-   ##
==========================================
+ Coverage   88.21%   88.34%   +0.12%     
==========================================
  Files         282      284       +2     
  Lines       25524    26055     +531     
==========================================
+ Hits        22516    23018     +502     
+ Misses       1535     1528       -7     
- Partials     1473     1509      +36     
Impacted Files Coverage Δ
Libplanet.Tests/Action/ActionContextTest.cs 88.37% <0.00%> (ø)
Libplanet/Action/CurrencyPermissionException.cs 71.42% <ø> (ø)
...bplanet/Blockchain/FungibleAssetStateCompleters.cs 35.71% <0.00%> (-9.75%) ⬇️
Libplanet/Blockchain/BlockChain.cs 90.48% <16.66%> (-0.30%) ⬇️
Libplanet.Tests/Action/ActionEvaluationTest.cs 91.66% <66.66%> (+1.04%) ⬆️
Libplanet.Tests/Blockchain/BlockChainTest.cs 98.30% <66.66%> (-0.06%) ⬇️
...ibplanet.Tests/Action/AccountStateDeltaImplTest.cs 94.79% <83.60%> (+0.11%) ⬆️
Libplanet.Tests/Tx/TransactionTest.cs 92.77% <89.47%> (-0.01%) ⬇️
Libplanet/Assets/Currency.cs 98.41% <90.90%> (ø)
Libplanet.Tests/Assets/FungibleAssetValueTest.cs 91.25% <91.25%> (ø)
... and 18 more

@dahlia dahlia force-pushed the fungible-asset-value branch 2 times, most recently from 99e9b43 to 0da1afc Compare August 14, 2020 16:43
@dahlia dahlia linked an issue Aug 14, 2020 that may be closed by this pull request
@dahlia dahlia marked this pull request as ready for review August 14, 2020 16:53
@dahlia dahlia changed the title WIP: Represent fungible asset values as a distinct type and support minor units Represent fungible asset values as a distinct type and support minor units Aug 14, 2020
longfin
longfin previously approved these changes Aug 16, 2020
/// name="sign"/>, <paramref name="majorUnit"/> and <paramref name="minorUnit"/>.
/// </summary>
/// <param name="currency">The currency to create a value.</param>
/// <param name="sign">Indicates the sign (negative, positive, or zero) of the value.
Copy link
Member

Choose a reason for hiding this comment

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

We may need new enum for 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.

Avoiding a new enum type was intended, because:

  • BigInteger.Sign property does the same thing.
  • It is much handier than enum values when adding a sign to values, i.e., you can simply do that by v.Sign * anotherValue instead of v.Sign == Sign.Negative ? -anotherValue : anotherValue.

/// <exception cref="DivideByZeroException">Thrown when the <paramref name="divisor"/> is
/// zero.</exception>
[Pure]
public BigInteger DivRem(FungibleAssetValue divisor, out FungibleAssetValue remainder)
Copy link
Member

Choose a reason for hiding this comment

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

Is this operation(someFAV.DivRem(someOtherFAV, out FAV rem)) really needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's intended to be parallel with BigInteger… a overloaded method which returns a remainder through a tuple instead out parameter does not exist in BigInteger but I added here for convenience' sake.

Copy link
Member

Choose a reason for hiding this comment

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

I was wondering there are situations in which one FAV needs to be divided by another FAV (not a number). I guess we can cover most of cases by .DivRem(BigInteger, out FungibleAssetValue).

IEquatable<FungibleAssetValue>,
IComparable<FungibleAssetValue>,
IComparable,
ISerializable
Copy link
Member

Choose a reason for hiding this comment

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

It would be better if we also adding IForamttable. (even next PR).

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 also wanted to implement IFormattable in this patch, but its specification seemed quite complex… I am going to implement it next time

(Bencodex.Types.Integer)stateCompleter(blockChain, hash, address, currency);
{
FungibleAssetValue balance = stateCompleter(blockChain, hash, address, currency);
return (Bencodex.Types.Integer)balance.RawValue;
Copy link
Member

Choose a reason for hiding this comment

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

Is it okay returning only .RawValue without currency?

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, raw state completer (Func<BlockChain<T>, HashDigest<SHA256>, IValue>) used by only BlockChain<T>.GetRawState() method, which is internal, takes IValue instead of FungibleAssetValue.

CHANGES.md Outdated Show resolved Hide resolved
CHANGES.md Outdated Show resolved Hide resolved
CHANGES.md Outdated Show resolved Hide resolved
Libplanet/Blockchain/BlockChain.cs Outdated Show resolved Hide resolved
Libplanet/Action/IAccountStateDelta.cs Outdated Show resolved Hide resolved
/// <see cref="FungibleAssetValue.MajorUnit"/>. For more precision, directly use <see
/// cref="FungibleAssetValue"/>'s constructors instead.</remarks>
[Pure]
public static FungibleAssetValue operator *(BigInteger quantity, Currency currency) =>
Copy link
Member

Choose a reason for hiding this comment

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

Can you please explain why this returns FungibleAssetValue instead of Currency?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Even though this diverts the multiplying operator (*), it's not an actual multiplication, but a shortcut for creating a new FungibleAssetValue: 123 * foo is equivalent to new FungibleAssetValue(currency: foo, major: 1, minor: 0).

@longfin longfin merged commit 815fa8d into planetarium:main Aug 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
storage Related to storage (Libplanet.Store) suggestion Suggestion or feature request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make fungible asset values to support exponent (minor units)
5 participants