Restore stable query cmds #955

Jimbo4350 · 2024-10-29T20:26:21Z

Resolves #953

Changelog

- description: |
    Restore stable query cmds
# uncomment types applicable to the change:
  type:
  # - feature        # introduces a new feature
  # - breaking       # the API has changed in a breaking way
  # - compatible     # the API has changed but is non-breaking
  # - optimisation   # measurable performance improvements
  # - refactoring    # QoL changes
  - bugfix         # fixes a defect
  # - test           # fixes/modifies tests
  # - maintenance    # not directly related to the code
  # - release        # related to a new release preparation
  # - documentation  # change in code docs, haddocks...

Context

Additional context for the PR goes here. If the PR fixes a particular issue please provide a link to the issue.

How to trust this PR

Highlight important bits of the PR that will make the review faster. If there are commands the reviewer can run to observe the new behavior, describe them.

Checklist

Commit sequence broadly makes sense and commits have useful messages
New tests are added if needed and existing tests are updated. See Running tests for more details
Self-reviewed the diff

smelc

Left a few comments but LGTM 👍

I checked that all commands that were in 9.0.0 are brought back 👍

smelc · 2024-10-30T08:02:38Z

cardano-cli/src/Cardano/CLI/EraBased/Options/Query.hs

+pTip envCli =
+  subParser "tip" $
+    Opt.info (pQueryTipCmd ShelleyBasedEraConway envCli) $
+      Opt.progDesc "Get the node's current tip (slot no, hash, block no)"


Suggested change

Opt.progDesc "Get the node's current tip (slot no, hash, block no)"

Opt.progDesc "Get the node's current tip (slot number, hash, block number)"

Optional.

Rationale: I think abbreviations can be misleading, in particular to non-native.

smelc · 2024-10-30T08:04:20Z

cardano-cli/src/Cardano/CLI/Options.hs

+-- Queries actually depend on the node to client version which may coincide
+-- with a hardfork but not necessarily. We will expose commands at the top level
+-- regardless if they are compatible with the era or not. The help text should be
+-- updated to make this clear. Gating commands behind eras


This comment's last sentence is incomplete.

smelc · 2024-10-30T08:05:28Z

cardano-cli/src/Cardano/CLI/Commands.hs

@@ -32,6 +33,8 @@ data ClientCommand
    KeyCommands KeyCmds
  | -- | Era agnostic node commands
    NodeCommands NodeCmds
+  | -- | Query commands
+    forall era. QueryCommands (QueryCmds era)


pQueryCmdsTopLevel is instantiated by ConwayEra, so we can avoid the existential quantifier here if we want.

I like this idea. I think we should define

type MainnetEra = ConwayEra mainnetEra :: ShelleyBasedEra MainnetEra mainnetEra = shelleyBasedEra

and use it wherever we need to reference the currently supported era.

I think I have a way to remove all mention of era.

smelc · 2024-10-30T08:09:40Z

cardano-cli/src/Cardano/CLI/EraBased/Options/Query.hs

+    Opt.info (pQueryPoolStateCmd ShelleyBasedEraConway envCli) $
+      Opt.progDesc $
+        mconcat
+          [ "DEPRECATED.  Use query pool-state instead.  Dump the pool parameters "


Since this was deprecated, maybe not reintroduce it?

carbolymer · 2024-10-30T08:30:42Z

cardano-cli/src/Cardano/CLI/EraBased/Options/Query.hs

+pStakeDistribution :: EnvCli -> Parser (QueryCmds ConwayEra)
+pStakeDistribution envCli =
+  subParser "stake-distribution" $
+    Opt.info (pQueryStakeDistributionCmd ShelleyBasedEraConway envCli) $


Let's not hardcode eras in multiple places. The current era ShelleyBasedEraConway should be defined in one place only and referenced from where it's needed.

carbolymer · 2024-10-30T08:53:41Z

cardano-cli/src/Cardano/CLI/EraBased/Options/Query.hs

+    Opt.info (pQueryTxMempoolCmd envCli) $
+      Opt.progDesc "Local Mempool info"
+
+pSlotNumber :: EnvCli -> Parser (QueryCmds ConwayEra)


All those parsers should accept era parameter (instead of hardcoding an era) and get reused in pQueryCmds.

carbolymer · 2024-10-30T08:56:46Z

cardano-cli/src/Cardano/CLI/Options.hs

@@ -60,6 +61,13 @@ addressCmdsTopLevel envCli = AddressCommand <$> pAddressCmds envCli
 nodeCmdsTopLevel :: Parser ClientCommand
 nodeCmdsTopLevel = NodeCommands <$> pNodeCmds

+-- Queries actually depend on the node to client version which may coincide
+-- with a hardfork but not necessarily. We will expose commands at the top level


We will expose commands at the top level (...)

The help text should be (...)

I don't think mentioning our intentions in the comments brings any value. Unless it's a TODO, which is easier to spot and correct than a prose with wishes.

carbolymer · 2024-10-30T09:00:54Z

cardano-cli/src/Cardano/CLI/EraBased/Options/Query.hs

+      i =
+        Opt.progDesc $
+          mconcat
+            [ "Node query commands. Will query the local node whose Unix domain socket is "


It would be nice to mention why this commands set is narrowed down in comparison to conway query.

carbolymer

Makes sense. I left some non-critical remarks (can be done in a follow-up PR if we're in a hurry here).

Jimbo4350 force-pushed the jordan/restore-stable-queries branch from d1a63a1 to a7bb1ad Compare October 29, 2024 20:45

Jimbo4350 added 2 commits October 29, 2024 16:48

Restore stable query cmds

bf8d247

Golden file updates

ce10769

Jimbo4350 force-pushed the jordan/restore-stable-queries branch from a7bb1ad to ce10769 Compare October 29, 2024 20:48

Jimbo4350 marked this pull request as ready for review October 29, 2024 20:50

Jimbo4350 requested review from newhoggy, carbolymer, smelc, palas, kevinhammond and disassembler as code owners October 29, 2024 20:50

smelc approved these changes Oct 30, 2024

View reviewed changes

carbolymer reviewed Oct 30, 2024

View reviewed changes

carbolymer approved these changes Oct 30, 2024

View reviewed changes

carbolymer added this pull request to the merge queue Oct 30, 2024

Merged via the queue into main with commit 112b13b Oct 30, 2024
27 checks passed

carbolymer deleted the jordan/restore-stable-queries branch October 30, 2024 10:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Restore stable query cmds #955

Restore stable query cmds #955

Jimbo4350 commented Oct 29, 2024 •

edited

Loading

smelc left a comment

smelc Oct 30, 2024

smelc Oct 30, 2024

smelc Oct 30, 2024

carbolymer Oct 30, 2024 •

edited

Loading

Jimbo4350 Oct 30, 2024

smelc Oct 30, 2024

carbolymer Oct 30, 2024 •

edited

Loading

carbolymer Oct 30, 2024 •

edited

Loading

carbolymer Oct 30, 2024

carbolymer Oct 30, 2024

carbolymer left a comment

	Opt.progDesc "Get the node's current tip (slot no, hash, block no)"
	Opt.progDesc "Get the node's current tip (slot number, hash, block number)"

Restore stable query cmds #955

Restore stable query cmds #955

Conversation

Jimbo4350 commented Oct 29, 2024 • edited Loading

Changelog

Context

How to trust this PR

Checklist

smelc left a comment

Choose a reason for hiding this comment

smelc Oct 30, 2024

Choose a reason for hiding this comment

smelc Oct 30, 2024

Choose a reason for hiding this comment

smelc Oct 30, 2024

Choose a reason for hiding this comment

carbolymer Oct 30, 2024 • edited Loading

Choose a reason for hiding this comment

Jimbo4350 Oct 30, 2024

Choose a reason for hiding this comment

smelc Oct 30, 2024

Choose a reason for hiding this comment

carbolymer Oct 30, 2024 • edited Loading

Choose a reason for hiding this comment

carbolymer Oct 30, 2024 • edited Loading

Choose a reason for hiding this comment

carbolymer Oct 30, 2024

Choose a reason for hiding this comment

carbolymer Oct 30, 2024

Choose a reason for hiding this comment

carbolymer left a comment

Choose a reason for hiding this comment

Jimbo4350 commented Oct 29, 2024 •

edited

Loading

carbolymer Oct 30, 2024 •

edited

Loading

carbolymer Oct 30, 2024 •

edited

Loading

carbolymer Oct 30, 2024 •

edited

Loading