CAD-2147: Support additional scripts in structured metadata. #1993

nc6 · 2020-11-17T14:32:28Z

Additional comments per commit.

uroboros

looks good! - also looks like a painful rebase in my near future :)

JaredCorduan · 2020-11-17T14:51:56Z

shelley-ma/impl/src/Cardano/Ledger/ShelleyMA/Metadata.hs

+-- | Raw, un-memoised metadata type
+data MetadataRaw era = MetadataRaw
+  { mdScriptPreimages :: !(StrictSeq (Core.Script era)),
+    -- | Unstructured metadata "blob"


so sorry to bike-shed a word in a comment, and a variable name. I myself had been using the word "unstructured" for this component, but @dcoutts let me know that actually we've been elsewhere been calling this "structured metadata" and this will confuse folks. mdScriptPreimages is great, though. :)

JaredCorduan · 2020-11-17T15:00:12Z

shelley-ma/impl/src/Cardano/Ledger/ShelleyMA/Metadata.hs

+      ( Ann (RecD MetadataRaw)
+          <*! D (sequence <$> decodeStrictSeq fromCBOR)
+          <*! Ann From
+      )


I thought that we wanted the deserialization to be backwards compatible.

Yes, I am an idiot and totally forgot about this. Will fix.

you are anything but that! and thanks again for taking this one on!

@redxaxder and I propose this: https://github.com/input-output-hk/cardano-ledger-specs/pull/1994/files#diff-03ecce724e5f30f7e4d885d88b81708a4bc1bc0451d2aa8b82137f7791f5c07bR214-R219

JaredCorduan

@nc6 this is really awesome, thank you. I made two comments, one about names 😱 and one about backwards compatibility.

nc6 · 2020-11-17T15:40:40Z

I should point out that the failing CI here is expected; I'm waiting for Ryan's PR on generalising the tests to fix things.

nc6 · 2020-11-20T12:40:15Z

@redxaxder Please note that in the last commit I changed the CDDL format to match the encoder, because it seemed completely non-obvious to me how to use Coders to represent the suggested format. But perhaps you or @TimSheard know how to do this easily, in which case I'd welcome a commit changing the representation (or just advice as to how to do this). This can either be part of this PR or a follow-up

redxaxder · 2020-11-20T13:25:38Z

shelley-ma/shelley-ma-test/cddl-files/shelley-ma.cddl

     ; other types of metadata...
-    }]
+    ]


If you're changing it to a dense encoding then the labels 0: and 1: are just informative rather than part of the encoding. IIRC some code generators emit these as names. So these could be more descriptive.

redxaxder · 2020-11-20T13:57:22Z

@redxaxder Please note that in the last commit I changed the CDDL format to match the encoder, because it seemed completely non-obvious to me how to use Coders to represent the suggested format. But perhaps you or @TimSheard know how to do this easily, in which case I'd welcome a commit changing the representation (or just advice as to how to do this). This can either be part of this PR or a follow-up

Your chosen encoding here is actually smaller than the one before. (In the case where there are two kinds of metadata, which is where we are). I think it might we worth talking about longer term plans for this.

Introduce a 'ValidateMetadata' class which operates across eras. Currently tests are still failing, since it may be easier to operate this atop of the generalised test generation framework.

This covers CAD-2147, and adds support for adding additional scripts to the metadata in a structured way.

- Round-tripping of MA-era Metadata - Check that Shelley-era metadata deserialises as MA-era.

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

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 <duncan@well-typed.com>

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 <duncan@well-typed.com>

2149: Update dependencies r=dcoutts a=mrBliss Visible changes: * IntersectMBO/cardano-ledger#1993 * IntersectMBO/cardano-ledger#2021 Co-authored-by: Thomas Winant <thomas@well-typed.com>

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 <duncan@well-typed.com>

2149: Update dependencies r=dcoutts a=mrBliss Visible changes: * IntersectMBO/cardano-ledger#1993 * IntersectMBO/cardano-ledger#2021 Co-authored-by: Thomas Winant <thomas@well-typed.com> Co-authored-by: Duncan Coutts <duncan@well-typed.com> Co-authored-by: Luke Nadur <19835357+intricate@users.noreply.github.com>

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 <duncan@well-typed.com>

2149: Update dependencies r=dcoutts a=mrBliss Visible changes: * IntersectMBO/cardano-ledger#1993 * IntersectMBO/cardano-ledger#2021 Co-authored-by: Thomas Winant <thomas@well-typed.com> Co-authored-by: Duncan Coutts <duncan@well-typed.com> Co-authored-by: Luke Nadur <19835357+intricate@users.noreply.github.com>

nc6 requested review from JaredCorduan and uroboros as code owners November 17, 2020 14:32

nc6 force-pushed the nc/cad-2147 branch from c81bfd8 to 348dc22 Compare November 17, 2020 14:36

uroboros approved these changes Nov 17, 2020

View reviewed changes

JaredCorduan reviewed Nov 17, 2020

View reviewed changes

nc6 force-pushed the nc/cad-2147 branch 2 times, most recently from d188e99 to 5621e17 Compare November 19, 2020 16:41

redxaxder force-pushed the nc/cad-2147 branch from 5621e17 to daa602d Compare November 20, 2020 00:05

nc6 force-pushed the nc/cad-2147 branch from daa602d to 4ffa559 Compare November 20, 2020 12:36

nc6 requested review from redxaxder and TimSheard November 20, 2020 12:36

redxaxder reviewed Nov 20, 2020

View reviewed changes

nc6 added 2 commits November 20, 2020 16:30

Generalise Metadata for different eras.

7db6d8c

Introduce a 'ValidateMetadata' class which operates across eras. Currently tests are still failing, since it may be easier to operate this atop of the generalised test generation framework.

Drop unneeded shelley-ma dependency in tests.

503122a

nc6 force-pushed the nc/cad-2147 branch from 4ffa559 to b6c75f7 Compare November 20, 2020 15:30

nc6 added 2 commits November 20, 2020 16:55

Implement Metadata for Shelley MA.

5a78877

This covers CAD-2147, and adds support for adding additional scripts to the metadata in a structured way.

Add tests for Metadata serialisation.

ac28c87

- Round-tripping of MA-era Metadata - Check that Shelley-era metadata deserialises as MA-era.

nc6 force-pushed the nc/cad-2147 branch from b6c75f7 to ac28c87 Compare November 20, 2020 15:55

nc6 merged commit c418c12 into master Nov 22, 2020

iohk-bors bot deleted the nc/cad-2147 branch November 22, 2020 20:20

mrBliss added a commit to IntersectMBO/cardano-node that referenced this pull request Dec 1, 2020

Update dependencies

d882f3a

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

mrBliss mentioned this pull request Dec 1, 2020

Update dependencies IntersectMBO/cardano-node#2149

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CAD-2147: Support additional scripts in structured metadata. #1993

CAD-2147: Support additional scripts in structured metadata. #1993

nc6 commented Nov 17, 2020

uroboros left a comment

JaredCorduan Nov 17, 2020

JaredCorduan Nov 17, 2020

nc6 Nov 17, 2020

JaredCorduan Nov 17, 2020

JaredCorduan Nov 17, 2020

JaredCorduan left a comment

nc6 commented Nov 17, 2020

nc6 commented Nov 20, 2020

redxaxder Nov 20, 2020

redxaxder commented Nov 20, 2020

CAD-2147: Support additional scripts in structured metadata. #1993

CAD-2147: Support additional scripts in structured metadata. #1993

Conversation

nc6 commented Nov 17, 2020

uroboros left a comment

Choose a reason for hiding this comment

JaredCorduan Nov 17, 2020

Choose a reason for hiding this comment

JaredCorduan Nov 17, 2020

Choose a reason for hiding this comment

nc6 Nov 17, 2020

Choose a reason for hiding this comment

JaredCorduan Nov 17, 2020

Choose a reason for hiding this comment

JaredCorduan Nov 17, 2020

Choose a reason for hiding this comment

JaredCorduan left a comment

Choose a reason for hiding this comment

nc6 commented Nov 17, 2020

nc6 commented Nov 20, 2020

redxaxder Nov 20, 2020

Choose a reason for hiding this comment

redxaxder commented Nov 20, 2020