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.

Sign up for GitHub

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

Jump to bottom

Updated to the most recent Alonzo specification #2095

Merged
merged 1 commit into from Jan 18, 2021
Merged

Updated to the most recent Alonzo specification #2095

merged 1 commit into from Jan 18, 2021

Conversation

TimSheard
Copy link
Contributor

Completed Figures 1, 5, 6, 7, 8 and 12. Ormolise.

This PR implements all the functions in the most up todate specification in the above Figures.

@nc6 nc6 self-requested a review January 13, 2021 12:09
JaredCorduan
JaredCorduan approved these changes Jan 13, 2021
View reviewed changes
Copy link
Contributor

@JaredCorduan JaredCorduan left a comment

Choose a reason for hiding this comment

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

This is a bunch of the alonzo figures, all lining up so nicely with the spec, it's looking really great! I made a few comments about mostly trivial things.

@JaredCorduan JaredCorduan self-requested a review January 13, 2021 15:01
nc6
nc6 requested changes Jan 13, 2021
View reviewed changes
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.

A bunch of minor things

alonzo/impl/src/Cardano/Ledger/Alonzo/PParams.hs Show resolved Hide resolved
alonzo/impl/src/Cardano/Ledger/Alonzo/PParams.hs Show resolved Hide resolved
alonzo/impl/src/Cardano/Ledger/Alonzo/PParams.hs Outdated Show resolved Hide resolved
alonzo/impl/src/Cardano/Ledger/Alonzo/PParams.hs Outdated Show resolved Hide resolved
alonzo/impl/src/Cardano/Ledger/Alonzo/Tx.hs Outdated Show resolved Hide resolved
alonzo/impl/src/Cardano/Ledger/Alonzo/Tx.hs Outdated Show resolved Hide resolved
alonzo/impl/src/Cardano/Ledger/Alonzo/Tx.hs Show resolved Hide resolved
alonzo/impl/src/Cardano/Ledger/Alonzo/Tx.hs Show resolved Hide resolved
alonzo/impl/src/Cardano/Ledger/Alonzo/TxBody.hs Outdated Show resolved Hide resolved
semantics/small-steps-test/small-steps-test.cabal
@@ -33,6 +33,7 @@ library
, microlens-th
, mtl
, nothunks
, prettyprinter
Copy link
Contributor

Choose a reason for hiding this comment

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

How is this related to this PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems the tests won't build without it. See Issue 2093, and the transcript of the build here
#2093

@TimSheard
Updated to the most recent Alonzo specification
78990a4 
Completed Figures 1, 2, 3, 4, 5, 6, 7, 8 and 12.
@TimSheard
Copy link
Contributor Author

This latest push does a few things

  1. It cleans up TxWitness so that the subpart that must be hashed is a public Memobytes. I also added a new pattern FlatWitness that lets users access it in the old style where there we 5 fields. So one can use either view.

  2. I tried to make everything that needed to be hashed a Memobytes

  3. I did some renaming of fields to be consistent and added HasField instances identedical to earlier errors.

  4. There are still two places where hashing need to be further addressed. in PParams and the function hashScriptData. I have stubbed out these parts. I suggest we get this PR merged and go back to address those issues.

nc6
nc6 approved these changes Jan 17, 2021
View reviewed changes
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!

@nc6 nc6 merged commit 3b27f2d into master Jan 18, 2021
@iohk-bors iohk-bors bot deleted the ts-alonzo-txmethods branch January 18, 2021 12:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@WhatisRT WhatisRT WhatisRT left review comments

@nc6 nc6 nc6 approved these changes

@uroboros uroboros Awaiting requested review from uroboros

@JaredCorduan JaredCorduan Awaiting requested review from JaredCorduan

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@TimSheard @WhatisRT @JaredCorduan @nc6