IntersectMBO / cardano-node

The core component that is used to participate in a Cardano decentralised blockchain.
https://cardano.org
Apache License 2.0
3.06k stars 720 forks source link

[FR] - Support latest era datatypes in cardano-api #4634

Open koslambrou opened 1 year ago

koslambrou commented 1 year ago

Internal/External Internal

Area Other

Describe the feature you'd like Working with the creation of transactions implies a lot of code boilerplate for the simple reason that transactions differ from era to era. However, this can be mitigated by providing modules which hardcode the era of the types. The Hydra team has done some significant effort in that aspect. See the Hydra.Cardano.API module in hydra-cardano-api). The README gives additional information on what the package contains.

For brievety, the module allows a user to go from:

changeOutput :: Address ShelleyAddr -> TxOut CtxUTxO BabbageEra
changeOutput =
  TxOut
    (AddressInEra (ShelleyAddressInEra ShelleyBasedEraBabbage) addr)
    (TxOutValue MultiAssetInBabbageEra (lovelaceToValue $ initialAmount - amount - fee))
    (TxOutDatumHash ScriptDataInBabbageEra (hashScriptData $ fromPlutusData someDatum))

to

changeOutput :: Address ShelleyAddr -> TxOut CtxUTxO
changeOutput =
  TxOut
    (AddressInEra addr)
    (lovelaceToValue $ initialAmount - amount - fee)
    (TxOutDatumHash $ hashScriptData $ fromPlutusData someDatum)

This feature request is about creating a PR for integrating the Hydra.Cardano.Api module, but renaming it to Cardano.Api.LatestEra. Note that the package also has some extra utilities to work with other stuff like Plutus scripts. In this ticket, we won't include these additional functionality and they should part of a separate ticket/discussion.

cc @abailly-iohk @ch1bo

Jimbo4350 commented 1 year ago

Thanks for this @koslambrou. I agree we need to expand the api to encompass a module like Cardano.Api.LatestEra.

I've had a look at Hydra.Cardano.Api and it was along the lines of what I was thinking (using pattern synonyms). What do you think of a slight tweak on the naming as follows:

type LatestEra = BabbageEra

latestEra :: ShelleyBasedEra BabbageEra
latestEra = ShelleyBasedEraBabbage

pattern ShelleyAddressType :: AddressTypeInEra ShelleyAddr LatestEra
pattern ShelleyAddressType <- ShelleyAddressInEra _
  where
    ShelleyAddressType = ShelleyAddressInEra latestEra

pattern ShelleyAddress :: Address ShelleyAddr -> AddressInEra LatestEra
pattern ShelleyAddress{address} <-
  Cardano.Api.AddressInEra Cardano.Api.ShelleyAddressInEra{} address
  where
    ShelleyAddress =
      Cardano.Api.AddressInEra ShelleyAddressType

I'm hesitant to implement type synonyms (as it will be more overhead IMO). Also I have renamed the patterns above because LatestEra indicates that indeed we are in the latest era. I didn't like the name ShelleyAddressInAnyEra because the address is not in any era, its in the latest era. ~Likewise for ShelleyAddressInEra~ This will result in a conflict with the constructor ShelleyAddress so we probably have to think carefully about naming when pulling in the hydra api types. Let me know your thoughts.

Jimbo4350 commented 1 year ago

I disagree with exposing ledger types via cardano-api e.g pattern ShelleyBootstrapWitness :: Ledger.BootstrapWitness StandardCrypto -> KeyWitness

So we should work together to expose a suitable interface to avoid users having to also pull in cardano-ledger related modules. I think we both agree that we don't want users to do this.

koslambrou commented 1 year ago

What do you think of a slight tweak on the naming as follows

@ch1bo

I disagree with exposing ledger types via cardano-api

Yes, and as a first pass, we should only merge the parts of Hydra.Cardano.Api which directly refer to existing cardano-api types. So it does not include this pattern. Thoughts @ch1bo

Also, I notice that the Hydra.Cardano.Api module hardcodes the type of a PlutusScript to V2. I'll argue that we don't want to do that because I can imagine DApp users staying with a specific Plutus language (like V1) for a long time, and the LatestEra module should only be about ledger eras, not Plutus language versions IMO. Thoughts?

Jimbo4350 commented 1 year ago

the LatestEra module should only be about ledger eras, not Plutus language versions IMO. Thoughts?

I agree. I think the design decision to only be concerned with the latest era makes sense (because that's what users are interested in). However we should not hardcode things like script versions. We should be backwards compatible wrt features.

ch1bo commented 1 year ago

I'm hesitant to implement type synonyms (as it will be more overhead IMO).

How would you realize this "type erasure" without pattern synonyms?

I disagree with exposing ledger types via cardano-api e.g pattern ShelleyBootstrapWitness :: Ledger.BootstrapWitness StandardCrypto -> KeyWitness

How is this different than the conversion functions which are available in Cardano.Api right now? (Or was it Cardano.Api.Shelley?) Some users (including Hydra) will need to get hold of the ledger types as there is no way to modify transactions after construction via the cardano-api (today?).


I do understand the responses so far as rather affirmative of the general idea? If yes, I would start work on a first PR and we can continue these discussions more concretely, e.g. what to include and what not?

github-actions[bot] commented 1 year ago

This issue is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 120 days.

ch1bo commented 1 year ago

Thanks Github, I should be looking into this :eyes:

github-actions[bot] commented 1 year ago

This issue is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 120 days.

github-actions[bot] commented 1 year ago

This issue is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 120 days.