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

More API updates for the Allegra and Mary eras #2111

Merged
merged 31 commits into from
Nov 25, 2020

Conversation

dcoutts
Copy link
Contributor

@dcoutts dcoutts commented Nov 22, 2020

Generalise most of the API tx construction functions over all the Shelley-based eras.

Will be needed for constructing txs in later ledger eras.
Move them into the TxMetadata module and generalise
toShelleyMetadataHash to work over all ledger eras.

Will be needed for creating txs in later ledger eras.
Will be needed for making txs for later ledger eras.
Not yet generalised over ledger eras.
We would like the ProtocolParametersUpdate type to be able to be
converted both to and from the equivalent underlying ledger type. We
would like this for a couple reasons:
1. so that we can use this same type later for programs that analyse
   the blockchain and want to see the content of update proposals;
2. so we can switch UpdateProposal to use the representation of the
   surface API types, while still using the on-chain format for the
   binary serialisation. This requires converting in both directions.

We want to do 2. now because we want a single era-independent
representation for update proposals, and to convert into the
era-dependent ledger types on-demand when we make a transaction.
So we now have conversions in both directions.
Switch the UpdateProposal to use the representation of the surface API
types.

We stick to the existing on-chain binary serialisation format.

We want to do this now to add support for more ledger eras. We want a
single era-independent representation for update proposals, and to
convert into the era-dependent ledger types on-demand when we make a
transaction for a specific era.
The protocol params should always have been exported since it is a API
surface type. The UpdateProposal repreentation can now be public since
it now uses the API surface types too.
Use Text instead of URI. We will want to allow conversion in both
directions which means we cannot be more restrictive than the ledger's
validity rules.
We'll use this next to change the representation of certificates
We will switch the representation shortly and so these conversion
functions will become non-trivial.
This will make it easier to convert from a common era-independent type
into ledger era-dependent types.

We still use the same on-chain binary format.
Needed both directly and to be able to generalise other such
conversion functions.
@dcoutts
Copy link
Contributor Author

dcoutts commented Nov 22, 2020

For some functions the Allegra and Mary cases are still TODOs but the types are now more general. This should allow CLI code to type check at the new eras. We'll fill in the remaining TODO cases.

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.

So far 👍

toCBOR = toCBOR . toShelleyUpdate
toCBOR = toCBOR . toShelleyUpdate @StandardShelley
-- We have to pick a monomorphic era type for the serialisation. We use the
-- Shelley era. This makes no difference since era type is phantom.
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we define a type synonym to make this clear. Something like:

type ShelleyOnwardsCrypto = StandardShelley or something with a better name 😁

Replace per-era instances with a single era-paramaterised instance.
This will be simpler as we add more eras.
Replace per-era instances with a single era-paramaterised instance.
This will be simpler as we add more eras.
Generalise over the IsShelleyBasedEra class.

The Allegra and Mary cases remain to do.
Switch from the Shelley.TxBody concrete type, to using the Ledger.TxBody
type family. The type family has different representations for different
eras.

For this step the specific era remains the same, so there are no other
changes. The next step will be to cover multiple eras, at which point we
will take advantage of the different tx body representations for each
era.
The 'ShelleyBasedEra' GADT tells us what era we are in.

The 'ShelleyLedgerEra' type family maps that to the era type from the
ledger lib.

The 'Ledger.TxBody' type family maps that to a specific tx body type,
which is different for each Shelley-based era.

Due to this use of GADTs We now need custom Eq and Show instances.

Do the minimal changes elsewhere, inserting error cases to fill in next.
This is partial in that we generalise the type, but the Allegra and Mary
cases are error TODOs for now. This is still helpful to allow downstream
code to type check at other eras.
All the Shelley-based eras are supported with one overloaded
implementation used at different types.
Still TODOs for validity intervals and minting.
We sort-of already had implementations of these in the impl of
to/fromShelleyAddress functions. Pull them out to top level functions
and put them next to the definition of the ScriptHash type.

We'll reuse these new functions in the next patch.
To/from the equivalent ledger library type. The ledger representation is
a pair of the lovelace value and a nested map of the non-ada assets.
Now that we have toMaryValue
Order the ada case first.
Copy link
Contributor

@intricate intricate left a comment

Choose a reason for hiding this comment

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

LGTM overall.

Comment on lines +518 to +521
-- The era parameter in these types is a phantom type so it is safe to cast.
-- We choose to cast because we need to use an era-independent address
-- representation, but have to produce an era-dependent format used by the
-- Shelley ledger lib.
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice 👍.

I'm slightly concerned that someone reading through the types will be confused, but doc like this should help.

Comment on lines -186 to +161
stakePoolMetadataURL :: URI.URI,
stakePoolMetadataURL :: Text,
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't it safer to use a URI to ensure that we've got a valid URI?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, but the on-chain format does not. And given that we want to use this type to read the chain as well as write it, then we cannot impose stronger constraints in the types.

We can do optional helpful validation of course.


makeByronKeyWitness :: NetworkId
-> TxBody ByronEra
-> SigningKey ByronKey
-> Witness ByronEra
makeByronKeyWitness _ (ShelleyTxBody era _ _) = case era of {}
Copy link
Contributor

Choose a reason for hiding this comment

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

This is my first time encountering EmptyCase, but is there a benefit here as opposed to using error?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes! The benefit is that the compiler can tell you that it is correct!

With error you're promising the compiler that this can never happen. With empty case the compiler check for us that it can never happen. We have pattern match warnings turned on (and as errors in CI) and so the compiler can check that we're right that there are no cases here that we have missed. There are simply no possible cases at all.

Comment on lines +7 to 8
{-# LANGUAGE PatternSynonyms #-}
{-# LANGUAGE NamedFieldPuns #-}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
{-# LANGUAGE PatternSynonyms #-}
{-# LANGUAGE NamedFieldPuns #-}
{-# LANGUAGE NamedFieldPuns #-}
{-# LANGUAGE PatternSynonyms #-}

Comment on lines 3 to +4
{-# LANGUAGE FlexibleInstances #-}
{-# LANGUAGE FlexibleContexts #-}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
{-# LANGUAGE FlexibleInstances #-}
{-# LANGUAGE FlexibleContexts #-}
{-# LANGUAGE FlexibleContexts #-}
{-# LANGUAGE FlexibleInstances #-}

Comment on lines +247 to +250
&& case era of
ShelleyBasedEraShelley -> txbodyA == txbodyB
ShelleyBasedEraAllegra -> txbodyA == txbodyB
ShelleyBasedEraMary -> txbodyA == txbodyB
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this be simplified?

Suggested change
&& case era of
ShelleyBasedEraShelley -> txbodyA == txbodyB
ShelleyBasedEraAllegra -> txbodyA == txbodyB
ShelleyBasedEraMary -> txbodyA == txbodyB
&& txbodyA == txbodyB

or is there a reason for doing it this way?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No. Can you see why?

Comment on lines +366 to +369
ByronEra -> "TxUnsignedByron"
ShelleyEra -> "TxUnsignedShelley"
AllegraEra -> "TxBodyAllegra"
MaryEra -> "TxBodyMary"
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want to maintain consistent naming here?

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 was thinking that we change it to be consistent with the type names (ie TxBody), but keep existing ones for compatibility. I'm happy to be talked out of it.

(Seq.fromList (map toShelleyCertificate txCertificates))
(toShelleyWithdrawal txWithdrawals)
(toShelleyLovelace fee)
(error "TODO: support validity interval")
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
(error "TODO: support validity interval")
(error "TODO: makeShelleyTransaction support validity interval")

=> Mary.Value ledgerera -> Value
fromMaryValue (Mary.Value lovelace other) =
Value $
--TODO: write QC tests to show it's ok to use Map.fromAscList here
Copy link
Contributor

Choose a reason for hiding this comment

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

And this would be okay because we're deriving an Ord instance for AssetId in which the earliest constructor declared is AdaAssetId, right?

So an appropriate test would basically just verify that AdaAssetId < AssetId x y.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Both order of ada vs other assets, and order amongst non-ada assets. This should be the case because converting to/from the ledger lib PolicyId and AssetName should not affect the order.

So an appropriate test would basically just verify that AdaAssetId < AssetId x y

No, it'd be the whole order of converting the value to a list, including all assets (including cases of assets with a common policy id).

Move toByronLovelace to the Value module, and split the TxBody module
section on TxIn and TxOut so they're in separate sections.

We're about to add a bunch more sections for types used in the body.
Move the TxOutValue, MintValue and related functions from the Value
module to the TxBody module.

We're going to end up with a whole bunch of these era-dependent field
types for the tx body and it makes most sense to keep them all together
in one module.
@dcoutts dcoutts force-pushed the dcoutts/api-allegra-mary branch 2 times, most recently from d0e1015 to dde3664 Compare November 24, 2020 16:12
@dcoutts
Copy link
Contributor Author

dcoutts commented Nov 24, 2020

bors merge

@iohk-bors
Copy link
Contributor

iohk-bors bot commented Nov 25, 2020

@iohk-bors iohk-bors bot merged commit 58e7a04 into master Nov 25, 2020
@iohk-bors iohk-bors bot deleted the dcoutts/api-allegra-mary branch November 25, 2020 01:06
iohk-bors bot added a commit that referenced this pull request Nov 26, 2020
2121: Completing API support for Allegra and Mary eras r=dcoutts a=dcoutts

Following on from #2111 with what is hopefully the last remaining API
changes needed to support all the Shelley-based eras.

All the representations and functions should now work for all
appropriate eras.

2126: Updated help text for query ledger/protocol-state r=intricate a=kevinhammond

Clarified that these commands are not intended for general use, and pointed to the underlying data structures for more information about content

Co-authored-by: Duncan Coutts <duncan@well-typed.com>
Co-authored-by: Luke Nadur <19835357+intricate@users.noreply.github.com>
Co-authored-by: Kevin Hammond <12563287+kevinhammond@users.noreply.github.com>
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