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

Extend Cardano.Api.ProtocolParameters with Alonzo params #2699

Merged
merged 3 commits into from
May 21, 2021
Merged

Extend Cardano.Api.ProtocolParameters with Alonzo params #2699

merged 3 commits into from
May 21, 2021

Conversation

dcoutts
Copy link
Contributor

@dcoutts dcoutts commented May 14, 2021

  • Add PlutusScriptV1 constructor in the existing PlutusScriptVersion type which previously had no constructors at all.
  • Add API types for execution units, prices and cost models
  • Extend the ProtocolParameters with Alonzo era ones, both ProtocolParameters and ProtocolParametersUpdate.

@dcoutts dcoutts requested review from cblp and Jimbo4350 May 14, 2021 00:08
@dcoutts
Copy link
Contributor Author

dcoutts commented May 14, 2021

Note that this extends the branch for PR #2657 so ignore the first 3 commits, and this will need to be rebased after that goes in.

This PR is a rebased and tidied up version of PR #2634 by @Jimbo4350. If we're happy with this one we can close the original. (I opened it as a new one entirely since it's actually substantially different so I didn't want to just force push over the top.)

jonathanknowles
jonathanknowles reviewed May 14, 2021
View reviewed changes
cardano-api/src/Cardano/Api/ProtocolParameters.hs
-- | Cost in ada per word of UTxO storage.
--
-- /Introduced in Alonzo/
protocolParamUTxOCostPerWord :: Maybe Lovelace,
Copy link

Choose a reason for hiding this comment

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

Hi @dcoutts

Does "word" in this context refer to the size of a Data.Word?

(I notice that we still use "byte" elsewhere, for example, in txFeePerByte, so I'm guessing the differentiation is deliberate in some way.)

Apologies for the drive-by comment, the PR title piqued my interest and decided to have a look. 😄

Copy link
Contributor

Choose a reason for hiding this comment

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

In Alonzo, the protocol parameter minUTxOValue is deprecated, and replaced by adaPerUTxOWord.
This specifies directly the deposit required for storing bytes of data on the ledger in the form of
UTxO entries

It's not clear to me that word means byte, but I think so.

Jimbo4350
Jimbo4350 reviewed May 14, 2021
View reviewed changes
cardano-node/src/Cardano/Node/Protocol/Cardano.hs Outdated
maryProtVer =
ProtVer 4 0
if npcTestEnableUnstableEras
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this to enable testing of a hardfork to Alonzo before consensus has merged their changes?

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. Though I've now moved this patch into this PR instead: #2400

cblp
cblp reviewed May 14, 2021
View reviewed changes
Copy link
Contributor

@cblp cblp left a comment

Choose a reason for hiding this comment

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

LGTM, but somebody else should approve

@dcoutts dcoutts force-pushed the dcoutts/alonzo-protocol-parameters-integration branch 3 times, most recently from e7e87e0 to 3f94692 Compare May 17, 2021 00:58
@dcoutts
Copy link
Contributor Author

dcoutts commented May 17, 2021

Rebased on top of #2649 since this order makes more sense. Only the last three commits are part of this PR proper. All the previous commits are from #2657 and #2649 that this builds on.

@Jimbo4350 Jimbo4350 mentioned this pull request May 17, 2021
Closed
@Jimbo4350 Jimbo4350 force-pushed the dcoutts/alonzo-protocol-parameters-integration branch from 3f94692 to 941bca7 Compare May 18, 2021 13:18
@dcoutts dcoutts force-pushed the dcoutts/alonzo-protocol-parameters-integration branch from 941bca7 to fe13a75 Compare May 18, 2021 23:11
@Jimbo4350 Jimbo4350 mentioned this pull request May 19, 2021
Merged
@Jimbo4350 Jimbo4350 force-pushed the dcoutts/alonzo-protocol-parameters-integration branch from fe13a75 to 9d89996 Compare May 20, 2021 15:09
dcoutts and others added 2 commits May 21, 2021 09:57
@dcoutts @Jimbo4350 @newhoggy
Add API types for execution units, prices and cost models
0c658ef 
Needed for using Plutus script witnesses and for protocol parameters.

Co-authored-by: Jordan Millar <jordan.millar@iohk.io>
@dcoutts @newhoggy
Add AnyPlutusScriptVersion type
919fae5 
And make it an instance of To/FromJSONKey. We will use this type in the
protocol params to index the per-language cost models and prices.
newhoggy
newhoggy reviewed May 21, 2021
View reviewed changes
cardano-cli/src/Cardano/CLI/Shelley/Script.hs Outdated
displayError (ScriptDecodeTextEnvelopeError err) =
"Error decoding script: " ++ displayError err
displayError (ScriptDecodeSimpleScriptError err) =
"Syncax error in script: " ++ displayError err
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo Syncax

@dcoutts @Jimbo4350 @newhoggy
Extend the API ProtocolParameters with Alonzo era ones
91b3d6c 
Both ProtocolParameters and ProtocolParametersUpdate.

We use AnyPlutusScriptVersion as the map key for the per-language cost
models and prices.

Conversion functions not yet extended.

Co-authored-by: Jordan Millar <jordan.millar@iohk.io>
@newhoggy newhoggy force-pushed the dcoutts/alonzo-protocol-parameters-integration branch from 9d89996 to 91b3d6c Compare May 21, 2021 00:11
@newhoggy newhoggy self-requested a review May 21, 2021 00:56
newhoggy
newhoggy approved these changes May 21, 2021
View reviewed changes
Copy link
Contributor

@newhoggy newhoggy left a comment

Choose a reason for hiding this comment

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

LGTM

@newhoggy
Copy link
Contributor

bors merge

@iohk-bors
Copy link
Contributor

iohk-bors bot commented May 21, 2021

Build succeeded:

@iohk-bors iohk-bors bot merged commit 996e409 into master May 21, 2021
@iohk-bors iohk-bors bot deleted the dcoutts/alonzo-protocol-parameters-integration branch May 21, 2021 01:08
iohk-bors bot added a commit that referenced this pull request May 24, 2021
@iohk-bors @newhoggy
@dcoutts @Jimbo4350
Merge #2642
73ab3e8 
2642: Update Cardano.Api.Tx in preparation for Alonzo r=dcoutts a=Jimbo4350

Depends on: 
1. #2657
2. #2400 
3. #2699
4. #2649

Co-authored-by: John Ky <john.ky@iohk.io>
Co-authored-by: Duncan Coutts <duncan@well-typed.com>
Co-authored-by: Jordan Millar <jordan.millar@iohk.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cblp cblp cblp left review comments

@jonathanknowles jonathanknowles jonathanknowles left review comments

@Jimbo4350 Jimbo4350 Jimbo4350 left review comments

@newhoggy newhoggy newhoggy approved these changes

@DavidEichmann DavidEichmann Awaiting requested review from DavidEichmann

@erikd erikd Awaiting requested review from erikd

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

Successfully merging this pull request may close these issues.
5 participants
@dcoutts @newhoggy @cblp @jonathanknowles @Jimbo4350