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

Added examples of transaction creation to Haddock #698

Open
wants to merge 15 commits into
base: master
Choose a base branch
from

Conversation

palas
Copy link
Contributor

@palas palas commented Dec 3, 2024

Changelog

- description: |
    Added example of transaction creation with traditional and experimental APIs to Haddock.
  type:
  - test
  - documentation

Context

Prompted by this slack discussion

How to trust this PR

Have a read of the haddock.

Checklist

  • Commit sequence broadly makes sense and commits have useful messages
  • New tests are added if needed and existing tests are updated. See Running tests for more details
  • Self-reviewed the diff

cardano-api/src/Cardano/Api/Experimental.hs Outdated Show resolved Hide resolved
cardano-api/src/Cardano/Api/Experimental.hs Outdated Show resolved Hide resolved
cardano-api/src/Cardano/Api/Experimental.hs Outdated Show resolved Hide resolved
cardano-api/src/Cardano/Api/Experimental.hs Outdated Show resolved Hide resolved
Copy link
Contributor

@Jimbo4350 Jimbo4350 left a comment

Choose a reason for hiding this comment

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

Nice work 👍 . I would move the transaction creation haddocks to Cardano.Api.Experimental.Tx and point to it from here.

cardano-api/src/Cardano/Api/Experimental.hs Outdated Show resolved Hide resolved
cardano-api/src/Cardano/Api/Experimental.hs Outdated Show resolved Hide resolved
-- let sbe = Api.shelleyBasedEra :: Api.ShelleyBasedEra Api.ConwayEra
-- @
--
-- We can also derive it from 'ConwayEra' from 'Cardano.Api.Experimental' by using the 'convert' function:
Copy link
Contributor

Choose a reason for hiding this comment

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

So I think this is a bad UI. We should have a defaultTxBodyContent defined in an experimental module:

defaultTxBodyContent
  :: ()
  => Era era
  -> TxBodyContent BuildTx era

We want to avoid mixing the old api and the new api (but obviously allow for migration e.g via convert).

These haddocks are excellent but what we should do is have haddocks strictly describing the old api, the new api and how to migrate from the old to the new.

cardano-api/src/Cardano/Api/Experimental.hs Outdated Show resolved Hide resolved
cardano-api/src/Cardano/Api/Experimental.hs Outdated Show resolved Hide resolved
@palas palas force-pushed the haddock-for-experimental-api branch from ec017ee to bd4f6ba Compare December 11, 2024 11:33
@palas palas added documentation Improvements or additions to documentation test Adding or reworking tests labels Dec 11, 2024
@palas palas force-pushed the haddock-for-experimental-api branch from 0ddace3 to c9d390f Compare December 13, 2024 14:36
Copy link
Contributor

@Jimbo4350 Jimbo4350 left a comment

Choose a reason for hiding this comment

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

Getting there 👍

( parseTxId

-- * Transaction bodies
( -- * Creating a 'TxBodyContent'
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's leave out mention of the experimental api in this section. You can add a separate section at the bottom mentioning the small tweak:

We can also derive it from [ConwayEra](https://github.com/IntersectMBO/cardano-api/pull/698/Cardano-Api-Eras-Core.html#t:ConwayEra) from [Experimental](https://github.com/IntersectMBO/cardano-api/pull/698/Cardano-Api.html#v:Experimental) by using the [convert](https://github.com/IntersectMBO/cardano-api/pull/698/Cardano-Api-Eon-Convert.html#v:convert) function:

let era = Exp.ConwayEra
let sbe = Api.convert era

Currently how the imports listed make it seem like you need:

import qualified Cardano.Api.Experimental as Exp   -- the experimental API

-- ** Balancing a transaction

-- |
-- If we have a UTxO with exactly 12 ADA, and 2 ADA is enough for the fees, we could just construct
Copy link
Contributor

Choose a reason for hiding this comment

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

We can word this better and we should think of this in terms of consumption and production (see preservation of value in the Shelley spec) if we want to be thorough. This is in the Shelley spec:
image

You would need to look through the other specs to confirm if these functions have been modified (likely with the introduction of multi-assets). We should say something like:

"A transaction is considered balanced if the value consumed is equivalent to the value produced. In the simplest terms a transaction that simply spends lovelace needs to be concerned with the total lovelace that exists at the inputs, the output(s) and the transaction fee. In other words:

inputs = outputs + fee"

You can then walk through the simple example of manually balancing a tx and point out that although the transaction is valid, you are overpaying for fees.

Overpaying for fees is not an invalid transaction.

--
-- We could do it analogously if we wanted to use an extended key, for example, using 'AsPaymentExtendedKey' and 'WitnessPaymentExtendedKey'.

-- ** Creating a transaction using the old API
Copy link
Contributor

Choose a reason for hiding this comment

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

I would put this section in the old API.

-- let oldStyleTx :: Api.Tx Api.ConwayEra = ShelleyTx sbe newApiSignedTx
-- @

-- ** Inspecting transactions
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here. If it doesn't have to do with the new api, move it to the old api modules. The only thing we should be talking about with respect to the old api is how to convert between transactions from old and new.

-- |
-- The documentation of the "Cardano.Api.Tx.Body" module shows how to create a 'TxBodyContent' for a
-- transaction that takes 12 ADA and sends 10 ADA to an address, and spends 2 ADA on fees. If we have a
-- UTxO with exactly 12 ADA, and 2 ADA is enough for the fees, this could be a valid transaction. But still,
Copy link
Contributor

Choose a reason for hiding this comment

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

Not could be, it is a valid transaction. This working will be confusing to newcomers. 2 ADA is more than enough for fees on mainnet so I would talk about it in that context, say the transaction is valid but you are overspending on fees. You also need to talk about tx outputs to make the explanation clearer.

-- finding this balance can be quite tricky. But fortunately, we have functions that help achieve this.
--
-- This module provides several different ways of calculating the fees for a transaction or altogether
-- balancing a draft transaciton, which require different amounts of information, and provide different levels
Copy link
Contributor

Choose a reason for hiding this comment

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

"transaction"

-- * The map of all deposits for drep credentials that are being unregistered in this transaction.
-- * Plutus script execution units for all script witness we are used in the transaction.
--
-- For this example, we are assuming we are doing nothing like that and using not scripts,
Copy link
Contributor

Choose a reason for hiding this comment

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

"we are assuming we are only spending key locked UTxOs and so we can ignore the above"

--
-- 5. Counts for:
--
-- * The collateral amount we are depositing. This is only required for transactions that
Copy link
Contributor

Choose a reason for hiding this comment

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

that have plutus scripts.

-- exampleChangeAddress
-- @
--
-- This will give us a balanced transaction, with the fees calculated, and the change output.
Copy link
Contributor

Choose a reason for hiding this comment

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

A balanced transaction body, that still needs to be signed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation test Adding or reworking tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants