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

Automagically determine the era of cli queries #2470

Merged
merged 2 commits into from
Mar 17, 2021

Conversation

Jimbo4350
Copy link
Contributor

  • No longer necessary to specify the era when submitting a cli query

Copy link
Contributor

@dcoutts dcoutts left a comment

Choose a reason for hiding this comment

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

Looking good! Definitely the right direction.

cardano-cli/src/Cardano/CLI/Shelley/Run/Query.hs Outdated Show resolved Hide resolved
cardano-cli/src/Cardano/CLI/Shelley/Run/Query.hs Outdated Show resolved Hide resolved
@Jimbo4350 Jimbo4350 force-pushed the jordan/automagically-determine-cli-era branch from 0ba9696 to 87f118a Compare March 10, 2021 16:25
Copy link
Contributor

@dcoutts dcoutts 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 better.

At a minimum we should just squash the fixes commit into the first commit before merging.

But there's one more thing I think that's worth thinking about:

This patch expands the code quite a bit still because it does case analysis on the mode and duplicates quite a bit for each mode, whereas the old versions were given the era in an function argument and there were mostly generic in the mode.

I think with a little creativity we could restore that generic approach where we treat all the modes uniformly, and don't have to do case analysis on the mode.

What the old code does is to construct the eraInMode early in each action, and without having to do case analysis on the mode. Of course now we only want to do the automagic era lookup query if we are in a multi-era mode. But I think we can still do that. We just need a helper function that is given the mode and returns the era in mode. That helper function can do case analysis on the mode and return constant eras for the single era modes, or execute the query for the multi-era modes (currently only the CardanoMode of course).

I think it's worth giving this a go. We're going to get more multi-era modes in future (for prototype modes) and it's good to have as much of this code as possible be generic, rather than duplicating per mode.

@Jimbo4350 Jimbo4350 force-pushed the jordan/automagically-determine-cli-era branch 7 times, most recently from 49e39f2 to b85e930 Compare March 15, 2021 09:50
Copy link
Contributor

@dcoutts dcoutts 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 fine. Optional suggestions below.

cardano-cli/src/Cardano/CLI/Shelley/Run/Query.hs Outdated Show resolved Hide resolved
cardano-cli/src/Cardano/CLI/Shelley/Run/Query.hs Outdated Show resolved Hide resolved
cardano-cli/src/Cardano/CLI/Shelley/Run/Query.hs Outdated Show resolved Hide resolved
cardano-cli/src/Cardano/CLI/Shelley/Run/Query.hs Outdated Show resolved Hide resolved
@Jimbo4350 Jimbo4350 force-pushed the jordan/automagically-determine-cli-era branch from b85e930 to 6a7941c Compare March 17, 2021 11:23
@Jimbo4350 Jimbo4350 force-pushed the jordan/automagically-determine-cli-era branch from 6a7941c to 12011c9 Compare March 17, 2021 11:32
@Jimbo4350
Copy link
Contributor Author

bors r+

Copy link
Contributor

@dcoutts dcoutts left a comment

Choose a reason for hiding this comment

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

Nice.

@iohk-bors
Copy link
Contributor

iohk-bors bot commented Mar 17, 2021

@iohk-bors iohk-bors bot merged commit 0337288 into master Mar 17, 2021
@iohk-bors iohk-bors bot deleted the jordan/automagically-determine-cli-era branch March 17, 2021 12:16
@@ -10,7 +10,7 @@ The deregistration certificate contains the _epoch_ in which we want to retire t
First, you need to figure out the current epoch. The simplest way to get the current epoch is to use the `tip` command:

```bash
> cardano-cli query tip --testnet-magic 42 --mary-era --cardano-mode
> cardano-cli query tip --testnet-magic 42 --cardano-mode

Choose a reason for hiding this comment

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

I don't think --cardano-mode is required any longer.

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's strictly not required, the default is cardano mode

@gitmachtl
Copy link
Contributor

gitmachtl commented Mar 17, 2021

@dcoutts @Jimbo4350 Do we have a simple method in the future to query the current era of the node? Can we implement an output into the tip json? This was very handy in the past to check the era the node is currently in. In synched mode and during re-syncing.

@gitmachtl
Copy link
Contributor

gitmachtl commented Mar 18, 2021

Is this only for queries but also for transactions? If only for queries, than we please need a method to query the era to use it later.

The current method is a trial and error method to query the protocol-parameters with the different era switches, that one thats not failing is the right era.

How does this work in offline mode? @Jimbo4350

@gitmachtl
Copy link
Contributor

gitmachtl commented Mar 20, 2021

Latest master now dropped the backwards compatibility to at least use the specified era for a query command. Now the cli exits with an error. This breaks all 3rd-Party tools so far and all tutorials again. So, please include the current era in the tip query or in the protocol-parameters query and maybe also reactivate the command line parameters for he query commands to not fail if an era is specified.

@robinboening
Copy link

robinboening commented Apr 8, 2021

I believe its the right decision to drop the era argument and default to the current 👍

dropped the backwards compatibility to at least use the specified era for a query command. Now the cli exits with an error. This breaks all 3rd-Party tools so far and all tutorials again.

I haven't tried it myself yet, but if this is true it should be highlighted in the release notes as it needs more attention.

It would have been better if the argument was deprecated and will only be removed in the next bigger release, so people would have more time to update their scripts and can go ahead with updating their nodes.

I also recommend to follow semantic versioning which means increasing the major version if an API breaking change is introduced. It makes clear there are breaking changes in the release just by looking at the version number.

✌️

@dcoutts

@gitmachtl
Copy link
Contributor

gitmachtl commented Apr 8, 2021

I haven't tried it myself yet, but if this is true it should be highlighted in the release notes as it needs more attention.

It is in the release notes.. i submitted it yesterday to the team and they included a few additional points in there:
https://github.com/input-output-hk/cardano-node/releases/tag/1.26.1

It is also addressed here on how to do such things in the future:
#2523

@robinboening
Copy link

I haven't tried it myself yet, but if this is true it should be highlighted in the release notes as it needs more attention.

It is in the release notes.. i submitted it yesterday to the team and they included a few additional points in there:
https://github.com/input-output-hk/cardano-node/releases/tag/1.26.1

It is also addressed here on how to do such things in the future:
#2523

I saw it in the release notes, but it's not highlighted as a breaking change. I only understood it's breaking because of the comments here. That is what I tried to point out.

@gitmachtl
Copy link
Contributor

the json output of the utxo query also totally changed, also breaking. yes, we had many breaking changes in the past. we (the tool-provider) are dealing with them since the first jormungandr release. i requested a single source of truth a long time ago for 3rd party developers, but thats difficult. meanwhile i dig thru all the code changes as good as i can. because the 3rd party tool development and testing needs time. getting the infos on the release date is way too late. people want to use the new releases with there prefered managment tools. but i also understand that there is no way we get an info on each and every thing the devs are changing in the code, trying out things, etc.... but basically scanning github and checking the repos is the best we get right now.

robinboening added a commit to lacepool/cardano-node-docker that referenced this pull request Apr 9, 2021
the era argument has been removed from cardano-cli, see IntersectMBO/cardano-node#2470
@robinboening
Copy link

but i also understand that there is no way we get an info on each and every thing the devs are changing in the code, trying out things, etc

I do believe it is possible. One common approach is to maintain a CHANGELOG in the root directory and every PR needs to consider if its worth adding a small note. The other part is to follow SemVer. Its simple, it just needs to become part of the process.

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.

None yet

6 participants