IntersectMBO / cardano-api

Cardano API
Apache License 2.0
22 stars 20 forks source link

[FEATURE] - Challenging Breaking change `Use the ledger's Coin instead of Lovelace`in `8.40.0` #607

Closed nhenin closed 1 month ago

nhenin commented 1 month ago

Internal/External : Internal
Context : I am challenging this commit : https://github.com/IntersectMBO/cardano-api/commit/46c1e605244fb3d01deda3451a0b56763c7c9370 causing a breaking change into 8.40.0 Where

newtype Lovelace = Lovelace Integer
  deriving stock (Eq, Ord, Show)
  deriving newtype (Enum, Real, Integral, Num, ToJSON, FromJSON, ToCBOR, FromCBOR)

has been replaced by

-- | The amount of value held by a transaction output.
newtype Coin = Coin {unCoin :: Integer}
  deriving
    ( Eq
    , Ord
    , Enum
    , NoThunks
    , Generic
    , ToJSON
    , FromJSON
    , NFData
    )
  deriving (Show) via Quiet Coin
  deriving (Semigroup, Monoid, Group, Abelian) via Sum Integer
  deriving newtype (PartialOrd, ToCBOR, EncCBOR, HeapWords)

imo, It is misleading for following reasons :

  1. New Invariant : It is a Positive Quantity as it is -- | The amount of value held by a transaction output. Before you could think it was only Lovelace Quantities.
  2. Naming : Coin is ~ to Token so you would expect CurrencySymbol + Name + Quantity, here is a definition of Token vs Coin
    
    Coins vs Tokens: Key Differences
    The primary difference between coins and tokens lies in their structure and purpose. Coins operate on their own
    blockchain and primarily function as a medium of exchange. They are akin to digital forms of value exchange and are 
    often used to pay for transactions within their respective blockchain network.

Tokens, however, operate on existing blockchain networks and aim to offer a wider range of functionalities. They are often associated with a specific project or protocol within the blockchain ecosystem and are used to access certain features of that project.

In terms of creation, tokens are easier to create than coins. Creating a coin requires building a new blockchain, which requires time and expertise. Tokens, however, can be created on an existing blockchain, making the process simpler and more accessible.

If we use the term `Coin` it needs more explanation. I didn't find any mention of this breaking change in the change log.
(see https://github.com/IntersectMBO/cardano-api/blob/main/cardano-api/CHANGELOG.md#84400  + please it would be more efficient if you document what is a `Breaking Change`, an `Add` or a `Remove` moving forward as well - e.g : https://www.conventionalcommits.org/en/v1.0.0/)

3. `Structure/Typing` : We have lost semantics,  it is not Lovelace anymore ? it could be anything ? How do I know ?

5. Lack of consistency : 
We keep using `Lovelace` in the functions defined in `cardano-api` but with the type `Coin` now : 
```haskell
lovelaceToQuantity :: L.Coin -> Quantity
lovelaceToQuantity (L.Coin x) = Quantity x

quantityToLovelace :: Quantity -> L.Coin
quantityToLovelace (Quantity x) = L.Coin x

Either way if it is Lovelace or Coin, the naming should be consistent. it is either Coin all the way or Lovelace but not a mix like it is today. :

https://github.com/IntersectMBO/cardano-api/blob/a7af2f29dbc7ae923a7140dddbbbb0f89f760046/cardano-api/internal/Cardano/Api/Value.hs#L10-L58) Coin concept is not explain and these functions doesn't express an accurate semantic since Lovelace doesn't exist anymore : e.g :

   -- ** Ada \/ L.Coin specifically
  , quantityToLovelace
  , lovelaceToQuantity
  , selectLovelace
  , lovelaceToValue
  , valueToLovelace

-- VS 

   -- ** Ada \/ L.Coin specifically
  , quantityToCoin
  , coinToQuantity
  , selectCoin
  , coinToValue
  , valueToCoin
lehins commented 1 month ago

Coin is a ledger type that is backed by an Integer, but it is enforced to always fit into Word64, in other words it is always non-negative. It's been a type in ledger since Shelley, so it predates all of the current ledger developers. Therefore, I don't know the reasoning for choosing the name Coin, but its actual meaning in Ledger is "Lovelace". That being said, changing the name in the ledger codebase would take a whole lot of work, so I am not quite sure that it is worth it at this point, especially considering that all developers working on the code already do know the meaning. On the cardano-api side there was always a Lovelace type that is isomorphic to Coin, so there was always this unnecessary conversion back and forth between the two types. It only makes sense to get rid of this redundant conversion.

I personally think that this removal of extra Lovelace indirection is a very nice improvement, even if it comes at a cost of using an unusually named type instead.

it is either Coin all the way or Lovelace but not a mix like it is today.

I don't think it is that big of a deal. In the US we have pennies, cents and coins, which all refer conceptually to the same thing. So, I sincerely believe there is no need to make a big deal of it. As long as everyone is clear about the name, we should be fine.

lehins commented 1 month ago

Oh yeah, and it is definitely not a bug. I've changed the title of the ticket to reflect it.

nhenin commented 1 month ago

Responding to this comment from @lehins:

I personally think that this removal of extra Lovelace indirection is a very nice improvement, even if it comes at a cost of using an unusually named type instead.

I believe this situation arises because maintainers of cardano-api have responded to feedback from their users by adapting their own repositories rather than modifying the ledger. While Coin is a specialized concept for those working at the low level of the blockchain, Lovelace is more relevant to end users. An API should cater to its users' needs, and in this case, the change seems like a regression from that perspective.

As a heavy user of this API, particularly via the Marlowe project, I’ve observed the impact of this change firsthand. It has introduced additional complexity and reduced clarity in the codebase, which makes it harder to work with.

To clarify, I do not recommend modifying the Ledger repositories directly. The purpose of an API is to provide an interface that adapts to the needs of external users. cardano-api serves as a higher-level abstraction over the ledger/consensus and network, allowing it to interact with users without altering the underlying Ledger codebase when it is not relevant.

lehins commented 1 month ago

It has introduced additional complexity and reduced clarity in the codebase, which makes it harder to work with.

I don't quite understand what kind of complexity you can run into by switching from one isomorphic type to another. This change could be viewed as equivalent to renaming Lovelace to Coin in cardano-api, which cannot introduce any complexity, aside from requiring do perform a rename for downstream users.

it is either Coin all the way or Lovelace but not a mix like it is today.

One way or another, we are moving in a direction of Coin all the way, which doesn't seem to conflict with your suggestion. Am I missing something here?