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

Reorganize Cardano.Ledger.Shelley and ShelleyMA era modules #2021

Merged
merged 1 commit into from
Nov 26, 2020

Conversation

dnadales
Copy link
Member

  • Move ShelleyEra and associated definitions to Cardano.Ledger.Shelley.

  • Define Core instances in the ShelleMA era module.

Copy link
Contributor

@nc6 nc6 left a comment

Choose a reason for hiding this comment

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

Looks great, thanks Damian!

I'm a little uncertain about pulling e.g. ValidateScript to the top, but this is my own fault for making these things era-based, rather than using the specific Script type. So this is perhaps something to address in the future, but I think that this PR takes the best approach to these things as they are defined now.

Copy link
Contributor

@mrBliss mrBliss left a comment

Choose a reason for hiding this comment

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

LGTM

Comment on lines 18 to 20
import Shelley.Spec.Ledger.MetaData (MetaData (MetaData), MetaDataHash (MetaDataHash), ValidateMetadata (hashMetadata, validateMetadata), validMetaDatum)
import Shelley.Spec.Ledger.Scripts (MultiSig)
import Shelley.Spec.Ledger.Tx (TxBody, ValidateScript (hashScript, validateScript), hashMultiSigScript, validateNativeMultiSigScript)
Copy link
Contributor

Choose a reason for hiding this comment

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

There are a few lines that I would wrap.


instance CryptoClass.Crypto c => ValidateMetadata (ShelleyEra c) where
hashMetadata = MetaDataHash . hashWithSerialiser toCBOR
validateMetadata (MetaData m) = all validMetaDatum m
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably unrelated to this PR, but I saw a deepseq here in some other implementation. Is that intentional?

Copy link
Contributor

Choose a reason for hiding this comment

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

That was in the Shelley-MA package, related to the new optional scripts. The idea there is that we want to verify that scripts included in the metadata are valid. Scripts are valid by construction, so this check comes down to making sure they're fully deserialised.

module Cardano.Ledger.Shelley.Constraints where

import Cardano.Ledger.Compactible (Compactible (..))
import Cardano.Ledger.Core (AnnotatedData, ChainData, Metadata, Script, SerialisableData, TxBody, Value)
Copy link
Contributor

Choose a reason for hiding this comment

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

Rather long 🙂

…lley`.

- Define `Core` instances in the `ShelleMA` era module.
@dnadales dnadales force-pushed the dnadales/move-shelley-era-to-its-own-module branch from 69568ba to a260d53 Compare November 26, 2020 14:15
@dnadales
Copy link
Member Author

bors r+

@iohk-bors
Copy link
Contributor

iohk-bors bot commented Nov 26, 2020

@iohk-bors iohk-bors bot merged commit 261a62e into master Nov 26, 2020
@iohk-bors iohk-bors bot deleted the dnadales/move-shelley-era-to-its-own-module branch November 26, 2020 18:59
mrBliss added a commit to IntersectMBO/cardano-node that referenced this pull request Dec 1, 2020
dcoutts pushed a commit to IntersectMBO/cardano-node that referenced this pull request Dec 1, 2020
Visible changes:
* IntersectMBO/cardano-ledger#1993
* IntersectMBO/cardano-ledger#2021

One of the changes in Allegra over Shelley is the notion of transaction
auxiliary data. We retrospectively define tx metadata to be part of the
auxiliary data where in the Shelley era this consists only of the
existing tx metadata.

But from the Allegra era the auxiliary data contains both the tx
metadata and also auxiliary scripts. So where we previously hashed the
tx metadata, we now hash the auxiliary data.

Take the opportunity to refactor the tx metadata representation and
conversion functions to fit better with the new scheme.

Also take the opportunity to tidy up the imports in the Query module
since we have some very similarly-named type classes.

Co-Authored by: Duncan Coutts <[email protected]>
dcoutts added a commit to IntersectMBO/cardano-node that referenced this pull request Dec 1, 2020
Visible changes:
* IntersectMBO/cardano-ledger#1993
* IntersectMBO/cardano-ledger#2021

One of the changes in Allegra over Shelley is the notion of transaction
auxiliary data. We retrospectively define tx metadata to be part of the
auxiliary data where in the Shelley era this consists only of the
existing tx metadata.

But from the Allegra era the auxiliary data contains both the tx
metadata and also auxiliary scripts. So where we previously hashed the
tx metadata, we now hash the auxiliary data.

Take the opportunity to refactor the tx metadata representation and
conversion functions to fit better with the new scheme.

Also take the opportunity to tidy up the imports in the Query module
since we have some very similarly-named type classes.

Co-authored-by: Duncan Coutts <[email protected]>
iohk-bors bot added a commit to IntersectMBO/cardano-node that referenced this pull request Dec 1, 2020
2149: Update dependencies r=dcoutts a=mrBliss

Visible changes:
* IntersectMBO/cardano-ledger#1993
* IntersectMBO/cardano-ledger#2021

Co-authored-by: Thomas Winant <[email protected]>
intricate pushed a commit to IntersectMBO/cardano-node that referenced this pull request Dec 1, 2020
Visible changes:
* IntersectMBO/cardano-ledger#1993
* IntersectMBO/cardano-ledger#2021

One of the changes in Allegra over Shelley is the notion of transaction
auxiliary data. We retrospectively define tx metadata to be part of the
auxiliary data where in the Shelley era this consists only of the
existing tx metadata.

But from the Allegra era the auxiliary data contains both the tx
metadata and also auxiliary scripts. So where we previously hashed the
tx metadata, we now hash the auxiliary data.

Take the opportunity to refactor the tx metadata representation and
conversion functions to fit better with the new scheme.

Also take the opportunity to tidy up the imports in the Query module
since we have some very similarly-named type classes.

Co-authored-by: Duncan Coutts <[email protected]>
dcoutts added a commit to IntersectMBO/cardano-node that referenced this pull request Dec 1, 2020
Visible changes:
* IntersectMBO/cardano-ledger#1993
* IntersectMBO/cardano-ledger#2021

One of the changes in Allegra over Shelley is the notion of transaction
auxiliary data. We retrospectively define tx metadata to be part of the
auxiliary data where in the Shelley era this consists only of the
existing tx metadata.

But from the Allegra era the auxiliary data contains both the tx
metadata and also auxiliary scripts. So where we previously hashed the
tx metadata, we now hash the auxiliary data.

Take the opportunity to refactor the tx metadata representation and
conversion functions to fit better with the new scheme.

Also take the opportunity to tidy up the imports in the Query module
since we have some very similarly-named type classes.

Co-authored-by: Duncan Coutts <[email protected]>
iohk-bors bot added a commit to IntersectMBO/cardano-node that referenced this pull request Dec 1, 2020
2149: Update dependencies r=dcoutts a=mrBliss

Visible changes:
* IntersectMBO/cardano-ledger#1993
* IntersectMBO/cardano-ledger#2021

Co-authored-by: Thomas Winant <[email protected]>
Co-authored-by: Duncan Coutts <[email protected]>
Co-authored-by: Luke Nadur <[email protected]>
newhoggy pushed a commit to IntersectMBO/cardano-api that referenced this pull request May 23, 2023
Visible changes:
* IntersectMBO/cardano-ledger#1993
* IntersectMBO/cardano-ledger#2021

One of the changes in Allegra over Shelley is the notion of transaction
auxiliary data. We retrospectively define tx metadata to be part of the
auxiliary data where in the Shelley era this consists only of the
existing tx metadata.

But from the Allegra era the auxiliary data contains both the tx
metadata and also auxiliary scripts. So where we previously hashed the
tx metadata, we now hash the auxiliary data.

Take the opportunity to refactor the tx metadata representation and
conversion functions to fit better with the new scheme.

Also take the opportunity to tidy up the imports in the Query module
since we have some very similarly-named type classes.

Co-authored-by: Duncan Coutts <[email protected]>
newhoggy pushed a commit to IntersectMBO/cardano-cli that referenced this pull request May 24, 2023
Visible changes:
* IntersectMBO/cardano-ledger#1993
* IntersectMBO/cardano-ledger#2021

One of the changes in Allegra over Shelley is the notion of transaction
auxiliary data. We retrospectively define tx metadata to be part of the
auxiliary data where in the Shelley era this consists only of the
existing tx metadata.

But from the Allegra era the auxiliary data contains both the tx
metadata and also auxiliary scripts. So where we previously hashed the
tx metadata, we now hash the auxiliary data.

Take the opportunity to refactor the tx metadata representation and
conversion functions to fit better with the new scheme.

Also take the opportunity to tidy up the imports in the Query module
since we have some very similarly-named type classes.

Co-authored-by: Duncan Coutts <[email protected]>
newhoggy pushed a commit to IntersectMBO/cardano-cli that referenced this pull request May 24, 2023
2149: Update dependencies r=dcoutts a=mrBliss

Visible changes:
* IntersectMBO/cardano-ledger#1993
* IntersectMBO/cardano-ledger#2021

Co-authored-by: Thomas Winant <[email protected]>
Co-authored-by: Duncan Coutts <[email protected]>
Co-authored-by: Luke Nadur <[email protected]>
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.

3 participants