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

Start assembling Alonzo transactions and witnesses. #2022

Merged
merged 4 commits into from
Dec 9, 2020
Merged

Start assembling Alonzo transactions and witnesses. #2022

merged 4 commits into from
Dec 9, 2020

Conversation

nc6
Copy link
Contributor

@nc6 nc6 commented Nov 26, 2020

Early work setting up Alonzo types:

  • transaction inputs and outputs
  • witnesses
  • scripts and data

@nc6 nc6 force-pushed the nc/alonzo branch 8 times, most recently from f1c02f2 to d9c9324 Compare November 30, 2020 13:59
@nc6 nc6 requested a review from polinavino November 30, 2020 21:08
import Shelley.Spec.Ledger.Hashing (HashAnnotated (..))

-- | TODO this should be isomorphic to the plutus type
data Data era
Copy link
Contributor

Choose a reason for hiding this comment

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

Should it be imported from the Plutus library? Did we ever conclude the discussion on how close this is going to be to plain CBOR?

Copy link
Contributor

@polinavino polinavino Nov 30, 2020

Choose a reason for hiding this comment

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

I think Plutus will have or already has a particular single module that (re?)-exports everything that should be accessible to the ledger (this type, the interpreter calling function, etc.).

I guess things there will also have to become era-parametrized? Or do we maybe want to make this a type family so that only the family is parametrized by era, but not the specific types to which the family is instantiated for each era

exUnitsSteps :: !Word64
}
deriving (Eq, Show, Ord)

Copy link
Contributor

Choose a reason for hiding this comment

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

we need to have a discussion about how we actually handle this execution units tracking. What is in the spec now (ie this structure) is kind of middle ground between two approaches not finalized and needs to change. @WhatisRT we should schedule a meeting for some time in the next week or two


data ScriptDataRaw era = ScriptDataRaw
{ _scriptDataScripts :: Map (ScriptHash era) (Core.Script era),
_scriptDataData :: Set (Data era),
Copy link
Contributor

Choose a reason for hiding this comment

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

Using the correct plural of Datum here seems slightly more confusing than using the wrong one to make it clear that this is a collection of Datum objects (Datums) heeehee

Copy link
Contributor

Choose a reason for hiding this comment

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

also, should we make the scriptDataData and scriptDataScripts consistent with each other - as in, either have them both be a map from hash to the preimage, or have them both be just the preimage? And if so, which way do you prefer?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm fine with either. The spec currently has them just being the preimage, but the existing Shelley code has them as the map. As yet I haven't grokked the usage patterns well enough to proffer an opinion on what would be best for Data. If we want them consistent, then I can happily change this to a Map.

@nc6 nc6 force-pushed the nc/alonzo branch 4 times, most recently from 779ed35 to c6efcaa Compare December 9, 2020 13:17
@nc6 nc6 requested a review from JaredCorduan December 9, 2020 14:11
Copy link
Contributor

@TimSheard TimSheard left a comment

Choose a reason for hiding this comment

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

Things look good, but I have some questions about serialization of the Scripts and Witnesses.
perhaps you could talk me through it.

Comment on lines +16 to +30

-- | Marker indicating the part of a transaction for which this script is acting
-- as a validator.
data Tag
= -- | Validates spending a script-locked UTxO
Input
| -- | Validates minting new tokens
Mint
| -- | Validates certificate transactions
Cert
| -- | Validates withdrawl from a reward account
Wdrl
deriving (Eq, Generic, Ord, Show)
Copy link
Contributor

Choose a reason for hiding this comment

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

Lets put a comment here about how we intend to use this. Is it as a index to some type constructor (using Data.Kinds) or as a field to some record type.

Comment on lines +101 to +119
TxWitnessConstr
( Memo
( TxWitnessRaw
witsVKey
witsBoot
( ScriptDataConstr
( Memo
( ScriptDataRaw
witsScript
witsData
witsRdmr
)
_
)
)
)
_
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't we use the constructor patterns for ScriptData, rather than building it from whole cloth here?

) =>
FromCBOR (TxBodyRaw era)
where
fromCBOR = decode $ SparseKeyed "TxBodyRaw" init bodyFields []
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The final argument to SparseKeyed here is the set of required fields, such that an error will be raised if they're not present. So should set it here to the same set as in shelley-ma

Copy link
Contributor

@TimSheard TimSheard left a comment

Choose a reason for hiding this comment

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

After our discussion clarifying how TxWitness and TxScript are used this looks good.

@nc6 nc6 force-pushed the nc/alonzo branch 2 times, most recently from 2e7512e to 2d99233 Compare December 9, 2020 15:59
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.

I didn't realize I hadn't approved this already, sorry. Thanks @nc6 !

nc6 added 3 commits December 9, 2020 17:48
- transaction inputs and outputs
- witnesses
- scripts and data
Start establishing the framework for Alonzo tests, so far just including
roundtrip tests. We include a roundtrip CBOR test for TxWitness.
This is needed to work around a bug with cabal-to-nix. See this thread
(https://input-output-rnd.slack.com/archives/CG386Q1NE/p1607468183215300)
for further details.
@nc6 nc6 merged commit deeff4d into master Dec 9, 2020
@iohk-bors iohk-bors bot deleted the nc/alonzo branch December 9, 2020 17:25
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.

4 participants