chore: renamed plus and minus to add and subtract, added utility methods

[nemo83]

In recent days I had to do a lot of comparisons, addition and subtractions of ada and native assets. To support this I've created local utility classes for Value and Amount in CCL, Amt in yaci and I'm sure I'm missing something somewhere else.

While the different models in different libraries can be somehow justified, I think CCL currently lack a powerful and versatile Value object that allows basic operations for the most common types.

In this PR I have:

• renamed and deprecated plus and minus in favour to the more widely adopted and correct add and subtract
• added add and subtract for unit and the couple policyId assetName
• added a isZero method
• added a isPositive method
• added amountOf for unit and the couple policyId assetName

The one thing that I really don't like is the fact we have to remember to prepend "0x" to asset name if we are passing as hex, which should be really the only, default, case.

[satran004]

@nemo83 Thanks for the PR.

1. from(policyId, assetName, amount)
2. fromLovelace(amount)

If I am not wrong, in the case of lovelace, the policy ID is empty. But, I need to verify that. To avoid confusion, as these values are represented differently in the Value structure, we can keep #1 for native assets and #2 for lovelace. For #1, we can add a null check for policy id. Null check for asset name is not required as it could be empty.

Also, we can add an overloaded method
addLovelace(amount).

And in Value add(String unit, BigInteger amount) , check

• if unit=lovelace then addLovelace(amount)
• else add(from(...) (current code)

[nemo83]

Thanks for the review.

For lovelace values, both policy id and asset name should be empty byte array.

The class Amount in CCL has just unit attribute which is set to lovelace for ada amount, which confused me in the past coz when doing substring, it would then be policyId = lovelace. The Amt class in yaci has policyId empty and asset name equals to lovelace.
IMHO, both should be empty, setting either of them or unit to lovelace just complicates things. Hence the policy check and asset name check.

Will leverage AssetUtil to try to simplify and add the other add methods

[satran004]

You may want to use AssetUtil here.

[satran004]

Can we change the long >= 0 to compare BigInteger ?

[satran004]

@nemo83 Thanks.

A Few Comments:

1. Regarding the usage of AssetUtil, I was probably too quick to suggest its use. My bad :( . Let's avoid using AssetUtil in the Value class of the transaction-spec module. The transaction-spec module primarily deals with on-chain serialization/deserialization logic, while AssetUtil operates at a higher layer, handling concepts like fingerprints, units, etc.

   Additionally, in one of the recent releases, we moved AssetUtil to the core-api module and deprecated the older version. So, let's avoid another breaking change.

2. Based on the current structure and the intended purpose of the transaction-spec module, we shouldn't introduce concepts like unit in the Value object or any API within the transaction-spec module. This module should focus mainly on serialization/deserialization logic, mirroring the on-chain representation.

   As you mentioned, unit is a concept found in the Amount class, which is part of a higher API layer and is used in backend service APIs. We can revisit the Yaci's Amt class later, but I believe it has a similar purpose.

3. If we consider the above points, here is a proposal for handling the Value class:

   Keep the following methods in the Value class:

   • add, plus (deprecated)
   • addLovelace: We may consider renaming this to addCoin, as the field in the Value object is called coin.
   • add(policyId, assetName, amount) - Only for multi-assets
   • Value from(policyId, assetName, amount): This should handle only multi-asset scenarios, where the returned Value will have a coin value of 0.
   • fromCoin(BigInteger lovelace)
   • subtract, minus (deprecated)
   • subtractCoin(amount)
   • subtract(policyId, assetName, amount): This should only handle multi-assets, not lovelace.
   • isZero
   • isPositive

   Remove the following:

   • add(unit, amount)
   • amountOf(unit)

4. Regarding amountOf(policyId, assetName): We can retain this method in the Value class, but using filter(asset -> asset.getName().equals(assetName)) may return inconsistent results. The on-chain value of assetName is represented as bytes. Since there are both UTF-8 and non-UTF8 names on public networks, it's better to compare using hex or byte values when checking for assetName equality.

   Additionally, when deserializing an existing transaction, assetName is deserialized as a hex value (with a 0x prefix) to represent all supported values. You can verify this in deserialize method in the MultiAsset class.

   Given that this is internal logic in the Asset class, we could consider introducing a method like boolean hasName(String name) in the Asset class. This method would internally handle the hex value check and strings with the "0x" prefix. It would also reuse the logic in Asset.getNameAsBytes().

   This approach might help us simplify code and remove ambiguities in a few other areas down the line.

5. Finally, there is a ValueUtil class that currently contains only one method to convert a Value object into a list of Amounts. We could consider moving some of the unit-specific methods there. This class is part of the core-api module.

Hope it makes sense, and I didn't miss anything.

[satran004]

This change is removing forEach and using stream api to avoid extra iteration. right?

[sonarcloud]

Quality Gate failed

Failed conditions
33.1% Duplication on New Code (required ≤ 3%)

See analysis details on SonarCloud