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

Transaction build in any alonzo era when on babbage testnet #4135

Merged
merged 3 commits into from
Jul 26, 2022

Conversation

newhoggy
Copy link
Contributor

@newhoggy newhoggy commented Jul 5, 2022

Resolves #3909 for building Alonzo transactions from Babbage era

@newhoggy
Copy link
Contributor Author

newhoggy commented Jul 5, 2022

$ CARDANO_NODE_SOCKET_PATH=example/main.sock cardano-cli query utxo --testnet-magic 42 --address $(cardano-cli address build --testnet-magic 42 --payment-verification-key-file example/utxo-keys/utxo1.vkey)
                           TxHash                                 TxIx        Amount
--------------------------------------------------------------------------------------
4e11a1dee6c6acceb54471be4cc378d3f57fe803bcff405b7527b967c94210b6     0        300000000000 lovelace + TxOutDatumNone
$ cardano-cli address build --testnet-magic 42 --payment-verification-key-file example/utxo-keys/utxo1.vkey
addr_test1vrlwhj4x3e6lyfj5d4pz9rn7n0sshfdf53zvlydjagsp08g686tth
$ cardano-cli transaction build \
  --alonzo-era \
  --testnet-magic 42 \
  --change-address 'addr_test1vrlwhj4x3e6lyfj5d4pz9rn7n0sshfdf53zvlydjagsp08g686tth' \
  --tx-in '4e11a1dee6c6acceb54471be4cc378d3f57fe803bcff405b7527b967c94210b6#0' \
  --tx-out 'addr_test1vrlwhj4x3e6lyfj5d4pz9rn7n0sshfdf53zvlydjagsp08g686tth+1000000' \
  --out-file tx.body

Estimated transaction fee: Lovelace 165545
$ cat tx.body
{
    "type": "TxBodyAlonzo",
    "description": "",
    "cborHex": "86a300818258204e11a1dee6c6acceb54471be4cc378d3f57fe803bcff405b7527b967c94210b600018282581d60feebcaa68e75f226546d42228e7e9be10ba5a9a444cf91b2ea20179d1b00000045d952ef1782581d60feebcaa68e75f226546d42228e7e9be10ba5a9a444cf91b2ea20179d1a000f4240021a000286a99fff8080f5f6"
}
$ CARDANO_NODE_SOCKET_PATH=example/main.sock cardano-cli transaction sign --tx-body-file tx.body --signing-key-file example/utxo-keys/utxo1.skey --out
-file tx.body.signed
$ CARDANO_NODE_SOCKET_PATH=example/main.sock cardano-cli transaction submit --tx-file tx.body.signed --testnet-magic 42
Transaction successfully submitted.

@newhoggy newhoggy force-pushed the newhoggy/transaction-in-any-era branch from 3590002 to 2486484 Compare July 5, 2022 13:02
@newhoggy newhoggy marked this pull request as ready for review July 5, 2022 13:02
@newhoggy newhoggy force-pushed the newhoggy/transaction-in-any-era branch 3 times, most recently from 11da8e5 to df23de5 Compare July 5, 2022 13:18
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.

Great work 👍 . I do have a suggestion for an improvement because I believe the Summon type class is unnecessary. If you look closely, the summon type class is the same as as all of the supportedInEra functions:

3d8ff42

^ I have created a branch above. Feel free to cherry pick.

What are we trying to achieve with EraCast? We want to see if a particular type is supported in another era and if it is we can convert it. We have a slew of isSupportedInEra functions. If we add the era that we want our type to be converted to, we can simply reuse this functionality in the EraCast instances and we can do away with the Summon type class.

I would also like to add an error type to the EraCast. I have it defaulted to String but this is atrocious. We can do better however this is complicated by some of the supportedInEra functions returning a Maybe and some returning an Either. I would suggest a datatype that contains the offending type and the era we are translating to. Propagate this upwards to the cli and in the cli we can include the era we are translating from. We can then render "X is supported in eraA but not eraB" or something like that.

@newhoggy newhoggy force-pushed the newhoggy/transaction-in-any-era branch from df23de5 to 01c061e Compare July 9, 2022 01:43
@newhoggy newhoggy requested a review from Jimbo4350 July 9, 2022 09:24
@newhoggy newhoggy dismissed Jimbo4350’s stale review July 9, 2022 23:48

Review comments

@newhoggy newhoggy force-pushed the newhoggy/transaction-in-any-era branch from 4db7ef3 to 24866ae Compare July 10, 2022 03:50
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.

Almost there, one further suggestion

0a12e83

I think this is a better type for EraCastError:

data EraCastError = forall toEra thing.
  ( IsCardanoEra toEra
  , Show thing
  ) =>
    EraCastError
    { typeName :: thing
    , toEra :: CardanoEra toEra
    }

Why? You get more information for free. E.g 0a12e83#diff-d2926b61bd606dc6c5e96c7607110c554a255cf353852708e0ecbbcc3ccbc4f0L1442
Previously all you were given was that you were trying to convert a ReferenceScript script from one era to another. You have to perform some additional gymnastics to figure out which era you were converting from. However, if you propagate the data constructor and show the offending data, it tells you exactly which reference script you were trying to convert and gives you the ReferenceTxInsScriptsInlineDatumsSupportedInEra era type as well.

data ReferenceTxInsScriptsInlineDatumsSupportedInEra era where
    ReferenceTxInsScriptsInlineDatumsInBabbageEra :: ReferenceTxInsScriptsInlineDatumsSupportedInEra BabbageEra

You immediately know you which era you were trying to convert from as this is given via the ReferenceTxInsScriptsInlineDatumsInBabbageEra constructor.

If you want more precise control over how we render the type (instead of using Show), implementing a typeclass to do that would be fine. Although I think show is good enough for now.

cardano-api/src/Cardano/Api/Address.hs Outdated Show resolved Hide resolved
cardano-api/src/Cardano/Api/Query.hs Outdated Show resolved Hide resolved
cardano-api/src/Cardano/Api/Script.hs Outdated Show resolved Hide resolved
cardano-api/src/Cardano/Api/TxBody.hs Outdated Show resolved Hide resolved
@newhoggy newhoggy force-pushed the newhoggy/transaction-in-any-era branch from ac0a82c to 2a7befe Compare July 14, 2022 01:37
@newhoggy
Copy link
Contributor Author

EraCastError now stores the value instead of the type name.

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.

Minor fixes

@@ -45,6 +45,7 @@ import qualified Cardano.Binary as CBOR
import Cardano.Ledger.Shelley.Scripts ()
import Cardano.Ledger.ShelleyMA.TxBody ()

import Cardano.Api.EraCast
Copy link
Contributor

Choose a reason for hiding this comment

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

This should not be exposed to cardano-cli. Whatever you need should be exposed via Cardano.Api or Cardano.Api.Shelley.


import qualified Cardano.Api as Api
Copy link
Contributor

Choose a reason for hiding this comment

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

These modules are in the wrong place

@newhoggy newhoggy force-pushed the newhoggy/transaction-in-any-era branch from 5a84f94 to d463e01 Compare July 21, 2022 23:46
@newhoggy newhoggy dismissed Jimbo4350’s stale review July 22, 2022 00:14

addressed comments

@newhoggy newhoggy requested a review from Jimbo4350 July 22, 2022 00:15
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.

Great work @newhoggy

@newhoggy
Copy link
Contributor Author

bors r+

iohk-bors bot added a commit that referenced this pull request Jul 22, 2022
4135: Transaction build in any era r=newhoggy a=newhoggy

Resolves #3909

Co-authored-by: John Ky <john.ky@iohk.io>
Co-authored-by: Jordan Millar <jordan.millar@iohk.io>
@Jimbo4350
Copy link
Contributor

bors r-

@iohk-bors
Copy link
Contributor

iohk-bors bot commented Jul 22, 2022

Canceled.

@Jimbo4350 Jimbo4350 self-requested a review July 22, 2022 15:24
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.

newhoggy and others added 2 commits July 25, 2022 20:05
…than the current era.

This is achieved by adding a new EraCast type class which allows some
types such as the UTxO type to change era.
@newhoggy newhoggy force-pushed the newhoggy/transaction-in-any-era branch from d463e01 to 71072a6 Compare July 25, 2022 13:59
@newhoggy newhoggy changed the title Transaction build in any era Transaction build in any alonzo era when on babbage testnet Jul 26, 2022
@newhoggy newhoggy requested a review from Jimbo4350 July 26, 2022 09:32
@newhoggy newhoggy dismissed Jimbo4350’s stale review July 26, 2022 09:33

confirmed to work for buliding alonzo transactions

@newhoggy newhoggy force-pushed the newhoggy/transaction-in-any-era branch from 71072a6 to cd0d56b Compare July 26, 2022 11:12
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! Can you squash the last 4 commits before merging?

* Swap order of arguments to eraCast
* Improve error type for the eraCast operation
* Store the value instead of the type name in EraCastError
* Tidy up imports
@newhoggy newhoggy force-pushed the newhoggy/transaction-in-any-era branch from cd0d56b to af2e359 Compare July 26, 2022 22:53
@newhoggy
Copy link
Contributor Author

Squashed.

@newhoggy
Copy link
Contributor Author

bors r+

@iohk-bors
Copy link
Contributor

iohk-bors bot commented Jul 26, 2022

Build succeeded:

@iohk-bors iohk-bors bot merged commit 675c614 into master Jul 26, 2022
@iohk-bors iohk-bors bot deleted the newhoggy/transaction-in-any-era branch July 26, 2022 23:39
@CarlosLopezDeLara CarlosLopezDeLara mentioned this pull request Aug 2, 2022
12 tasks
mkoura added a commit to mkoura/cardano-node-tests that referenced this pull request Aug 8, 2022
mkoura added a commit to IntersectMBO/cardano-node-tests that referenced this pull request Aug 25, 2022
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.

[BUG] - transaction build command can build transactions only for current era
2 participants