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

#5052 Remove reading protocol parameters from Shelley genesis file #5053

Merged
merged 4 commits into from
Apr 14, 2023

Conversation

carbolymer
Copy link
Contributor

@carbolymer carbolymer commented Apr 4, 2023

As a goal of #5052 we should transition to reading protocol parameters by querying a node instead of using a Shelley genesis file.

This PR removes CLI option to read parameters from Shelley genesis file.

Command line arguments diff for example for cardano-cli transaction build:

--- before.out  2023-04-13 12:51:08.911896348 +0200
+++ after.out   2023-04-13 12:54:23.558643977 +0200
@@ -96,21 +96,21 @@
               | --withdrawal-tx-in-reference TX-IN
                 --withdrawal-plutus-script-v2
                 ( --withdrawal-reference-tx-in-redeemer-cbor-file CBOR FILE
                 | --withdrawal-reference-tx-in-redeemer-file JSON FILE
                 | --withdrawal-reference-tx-in-redeemer-value JSON VALUE
                 )
               ]]
             [--json-metadata-no-schema | --json-metadata-detailed-schema]
             [--auxiliary-script-file FILE]
             [--metadata-json-file FILE | --metadata-cbor-file FILE]
-            [--genesis FILE | --protocol-params-file FILE]
+            [--protocol-params-file FILE]
             [--update-proposal-file FILE]
             (--out-file FILE | --calculate-plutus-script-cost FILE)
 
   Build a balanced transaction (automatically calculates fees)
 
   Please note the order of some cmd options is crucial. If used incorrectly may produce undesired tx body. See nested [] notation above for details.
 
 Available options:
   --byron-era              Specify the Byron era
   --shelley-era            Specify the Shelley era
@@ -365,22 +365,18 @@
                            metadata.
   --json-metadata-detailed-schema
                            Use the "detailed schema" conversion from JSON to tx
                            metadata.
   --auxiliary-script-file FILE
                            Filepath of auxiliary script(s)
   --metadata-json-file FILE
                            Filepath of the metadata file, in JSON format.
   --metadata-cbor-file FILE
                            Filepath of the metadata, in raw CBOR format.
-  --genesis FILE           [TESTING] The genesis file to take initial protocol
-                           parameters from. For test clusters only, since the
-                           parameters are going to be obsolete for production
-                           clusters.
   --protocol-params-file FILE
                            Filepath of the JSON-encoded protocol parameters file
   --update-proposal-file FILE
                            Filepath of the update proposal.
   --out-file FILE          Output filepath of the JSON TxBody.
   --calculate-plutus-script-cost FILE
                            Output filepath of the script cost information.
   -h,--help                Show this help text

@carbolymer carbolymer force-pushed the mgalazyn/feature/deprecate-protocol-params-file branch from 9529de1 to 175db9d Compare April 4, 2023 15:47
carbolymer added a commit that referenced this pull request Apr 6, 2023
@carbolymer carbolymer changed the title #5052 Deprecate reading protocol parameters from a file #5052 Remove reading protocol parameters from Shelley genesis file Apr 7, 2023
@newhoggy
Copy link
Contributor

newhoggy commented Apr 7, 2023

LGTM, but I'll wait for @Jimbo4350 to approve in case I misunderstand something.

@newhoggy newhoggy requested a review from Jimbo4350 April 7, 2023 16:00
@carbolymer carbolymer force-pushed the mgalazyn/feature/deprecate-protocol-params-file branch from c340eb2 to 35346b0 Compare April 11, 2023 16:59
@newhoggy
Copy link
Contributor

LGTM

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.

LGTM, just update the changelog as this is a breaking change. Also reach out to @gitmachtl to make sure we aren't unknowingly breaking any of their tests.

@gitmachtl
Copy link
Contributor

gitmachtl commented Apr 13, 2023

@Jimbo4350 @carbolymer @newhoggy
If i understand the PR correctly, it would remove the possibility to specify a protocol-params-file for the calculations?

Thats a no go IMO, because we use this method all the time to do proper offline-transactions! The steps are basically:
1.) Do a query on a synced node in online mode, write out the latest protocol-parameters to a file
2.) Make and sign the transaction in offline mode using the values in the protocol-parameters file for cardano-cli
3.) Submitting the signed transaction again in online mode.

Or did i misunderstood something?

Or is this still possible but cardano-cli can only read in the JSON protocol params file that was queried before and written out?

@carbolymer
Copy link
Contributor Author

carbolymer commented Apr 13, 2023

If i understand the PR correctly, it would remove the possibility to specify a protocol-params-file for the calculations?

@gitmachtl Thanks for the input. Sorry I should've written a clearer description. No, --protocol-params-file is staying as of now. The only thing removed is using genesis file for protocol parameters.

@gitmachtl
Copy link
Contributor

@carbolymer ah ok, thx for clarification. i was aware because of your post here (#5052 (comment)). but ok, if that stays as it is, all good. i talked to a few others and it looks like the protocol-parameter query of just the genesis file was never really used in 3rd party tools. so, all good!

@carbolymer carbolymer added this pull request to the merge queue Apr 14, 2023
Merged via the queue into master with commit 363fc0f Apr 14, 2023
@iohk-bors iohk-bors bot deleted the mgalazyn/feature/deprecate-protocol-params-file branch April 14, 2023 07:24
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.

readProtocolParametersSourceSpec should be able to retrieve the ProtocolParameters by querying local node
4 participants