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

Adapt packages to use Protocol type class #2498

Merged
merged 1 commit into from
Apr 22, 2021
Merged

Conversation

jhartzell42
Copy link

@jhartzell42 jhartzell42 commented Mar 16, 2021

This PR depends on IntersectMBO/ouroboros-network#2978

In that PR, the Protocol type that the cardano-node packages use is removed. This is so that ouroboros-network does not need to enumerate all the consensus modes it implements. The majority of changes in this PR are in reaction to that, creating a type class to replace it.

We found that it would take a significant increase in code complexity to remove the explicit enumeration of supported consensus modes in two places:

  • cardano-node/src/Cardano/Node/Configuration/Logging.hs:nodeBasicInfo
  • cardano-node/src/Cardano/Node/Query.hs

For that purpose, we have written the GADT BlockType which provides the little bit of type discovery these places need.

In addition, we have removed the Enum instance for AnyCardanoEra. The motivation is that we anticipate the inclusion of experimental eras and consensus modes in the code base which cannot reasonably be said to be enumerable among the mainnet eras. This instance does not appear to be used anywhere. If it needs to be preserved then we will need an answer on how to handle eras we don't expect to be part of mainnet (at least when they are first implemented).

@jhartzell42 jhartzell42 force-pushed the os/open-protocol-pr branch 3 times, most recently from 0d28c0e to 068e593 Compare March 18, 2021 19:57
@jhartzell42
Copy link
Author

Per @nc6 , we don't have to support GHC 8.6, and those are the only tests that are failing, so we can go ahead with this.

@jhartzell42 jhartzell42 changed the title WIP: Adapt packages to use Protocol type class Adapt packages to use Protocol type class Mar 18, 2021
@jhartzell42 jhartzell42 marked this pull request as ready for review March 18, 2021 19:59
@jhartzell42 jhartzell42 changed the title Adapt packages to use Protocol type class WIP: Adapt packages to use Protocol type class Mar 29, 2021
@jhartzell42 jhartzell42 changed the title WIP: Adapt packages to use Protocol type class Adapt packages to use Protocol type class Mar 30, 2021
@jhartzell42 jhartzell42 force-pushed the os/open-protocol-pr branch 7 times, most recently from 4c9f837 to 9b57f62 Compare April 1, 2021 15:26
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.

I like the changes but I would get a sign off from @dcoutts

cardano-cli/src/Cardano/CLI/Shelley/Run/Query.hs Outdated Show resolved Hide resolved
@jhartzell42 jhartzell42 force-pushed the os/open-protocol-pr branch 2 times, most recently from a9013ff to d59426d Compare April 12, 2021 14:28
Copy link
Contributor

@dcoutts dcoutts 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 in case there are conflicts, this should go in after PR #2547. There probably won't be, but just to be safe.

@jhartzell42
Copy link
Author

bors merge

@Jimbo4350
Copy link
Contributor

bors r-

@iohk-bors
Copy link
Contributor

iohk-bors bot commented Apr 14, 2021

Canceled.

@jhartzell42
Copy link
Author

Sorry I didn't see the comment at first. Thanks!

iohk-bors bot added a commit that referenced this pull request Apr 19, 2021
2547: API refactoring to move scripts into the txbody r=Jimbo4350 a=dcoutts

REMOVE: Merge #2498 after this PR has been merged

This is the first step in a refactoring that we will need for Alonzo
support. The refactoring will put the scripts within the tx body at the
sites where they are used. For Shelley era scripts we can do it either
way round, but for Alonzo it makes much more sense to place the script
at the use sites. So to break the Alonzo changes into steps we do the
refactoring for the existing Shelley-based eras first.
    
The ideas here have been co-authored with Jordan Millar and most of the
code changes here appeared first in his Alonzo branch.

- [x] Update the API tests for these API changes
- [x] Update the CLI for these API changes

There are a couple breaking changes:
- You must specify auxiliary scripts with `--auxiliary-script-file` instead of `--script-file`
- Scripts  witnessing txins, certificates, withdrawals and minting must be paired with the thing they are witnessing. 
E.g:
```
--certificate-file  $certfile --certificate-script-file $scriptfile
--tx-out $txout --minting-script-file $scriptfile
--withdrawal $withdrawal --withdrawal-script-file $scriptfile
--tx-in $txin --txin-script-file $scriptfile
```
- Scripts are no longer specified at the tx signing stage. They are specified at the txbody stage.

Co-authored-by: Duncan Coutts <duncan@well-typed.com>
Co-authored-by: Jordan Millar <jordan.millar@iohk.io>
The way ouroboros-network exposes Protocol information to clients such
as cardano-api and cardano-node was changed from a Protocol GADT,
which lived in ouroboros-network, to a type class that lives
in this repo; similarly for ProtocolClient. These changes make the
universe of Protocols that can be run more open than before.

cardano-api, cardano-cli, cardano-node, and cardano-node-chairman are
here modified to use this type class as much as possible. At some point
they do have to close their assumptions on which modes are supported, but
this is pushed up to the top-level as much as possible.
@Jimbo4350
Copy link
Contributor

bors r+

@iohk-bors
Copy link
Contributor

iohk-bors bot commented Apr 22, 2021

@iohk-bors iohk-bors bot merged commit a593cec into master Apr 22, 2021
@iohk-bors iohk-bors bot deleted the os/open-protocol-pr branch April 22, 2021 07:06
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