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

Combinators for TxBodyContent and related types #4941

Merged
merged 1 commit into from
Mar 28, 2023

Conversation

newhoggy
Copy link
Contributor

@newhoggy newhoggy commented Mar 5, 2023

TxBodyContent has a function that constructs one of two default values for BuiltTx and ViewTx.

foo :: TxBodyContent BuildTx BabbageEra
foo = defaultTxBodyContent InBuildTx
  & setTxValidityRange someValidityRange
  & setTxIns someTxIns
  & setTxOuts someTxOuts
  & setTxOuts someMoreTxOuts

This is an alternative to #4458.

We don't seem to be able to agree on what the Semigroup instance should do out of a number alternatives and if there is confusion with choosing the behaviour, there will likely be confusion using the instance.

The idea behind this alternative is that we shouldn't arbitrarily choose a behaviour for the Semigroup instance because there is a high probability people who use our instances might assume it does something else.

This works with both ViewTx and BuildTx.

@newhoggy newhoggy marked this pull request as ready for review March 5, 2023 23:06
@newhoggy newhoggy force-pushed the newhoggy/default-values branch 3 times, most recently from 4b3c24a to 7f64edf Compare March 7, 2023 04:26
@newhoggy newhoggy changed the title New DefaultValue type class. Make TxBodyContent an instance of DefaultValue Make TxBodyContent and related types an instance of Default Mar 7, 2023
@newhoggy newhoggy changed the title Make TxBodyContent and related types an instance of Default Default instance and combinators TxBodyContent and related types Mar 7, 2023
@newhoggy newhoggy changed the title Default instance and combinators TxBodyContent and related types Default instance and combinators for TxBodyContent and related types Mar 7, 2023
Copy link
Contributor

@ch1bo ch1bo left a comment

Choose a reason for hiding this comment

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

Left my 2 lovelaces as a user of this library.

cardano-api/src/Cardano/Api/TxBody.hs Show resolved Hide resolved
cardano-api/src/Cardano/Api/TxBody.hs Outdated Show resolved Hide resolved
cardano-api/src/Cardano/Api/TxBody.hs Outdated Show resolved Hide resolved
cardano-api/src/Cardano/Api/Script.hs Outdated Show resolved Hide resolved
@lehins
Copy link
Contributor

lehins commented Mar 7, 2023

I'd rather not introduce more classes/types

Just a minor note. No new types or classes are introduced in this PR, just new instances 😉

@newhoggy newhoggy force-pushed the newhoggy/default-values branch 3 times, most recently from 424ea6a to 16e60df Compare March 21, 2023 06:40
@newhoggy newhoggy dismissed Jimbo4350’s stale review March 21, 2023 08:14

Use GADTs instead

@newhoggy newhoggy changed the title Default instance and combinators for TxBodyContent and related types Combinators for TxBodyContent and related types Mar 21, 2023
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 👍 . We definitely aren't interested in building ViewTx transactions, so you can remove support for that.

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

@ch1bo ch1bo left a comment

Choose a reason for hiding this comment

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

I like it and it's very much what we currently do in the upstream cardano-api. So hydra will be using this 👍

cardano-api/src/Cardano/Api/TxBody.hs Outdated Show resolved Hide resolved
@newhoggy newhoggy force-pushed the newhoggy/default-values branch 3 times, most recently from 9c643dd to 2bef436 Compare March 27, 2023 13:29
@newhoggy newhoggy requested a review from Jimbo4350 March 27, 2023 13:29
@newhoggy newhoggy dismissed Jimbo4350’s stale review March 27, 2023 13:30

Comments addressed

@newhoggy newhoggy force-pushed the newhoggy/default-values branch 2 times, most recently from 3cf60ab to fadb264 Compare March 27, 2023 13: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.

LGTM! Just a couple naming suggestions.

cardano-api/src/Cardano/Api.hs Show resolved Hide resolved
cardano-api/src/Cardano/Api/TxBody.hs Outdated Show resolved Hide resolved
cardano-api/src/Cardano/Api/TxBody.hs Outdated Show resolved Hide resolved
@newhoggy newhoggy added this pull request to the merge queue Mar 28, 2023
Merged via the queue into master with commit 9ea49d3 Mar 28, 2023
@iohk-bors iohk-bors bot deleted the newhoggy/default-values branch March 28, 2023 18:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants