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

In query tip command, make epoch number available on Byron #2688

Merged
merged 1 commit into from
May 12, 2021
Merged

In query tip command, make epoch number available on Byron #2688

merged 1 commit into from
May 12, 2021

Conversation

newhoggy
Copy link
Contributor

@newhoggy newhoggy commented May 10, 2021 •
edited
Loading

This is done by using the history interpreter to convert slot number to epoch number.

@newhoggy newhoggy force-pushed the query-tip-epoch-on-byron branch 2 times, most recently from dd1bd88 to 09cbf3d Compare May 10, 2021 06:50
@newhoggy
Copy link
Contributor Author

Resolves #2568

@newhoggy
Copy link
Contributor Author

On mainnet:

CARDANO_NODE_SOCKET_PATH=local-node/socket cardano-cli query tip --mainnet
{
    "epoch": 264,
    "hash": "34197ddb795fdb50d6933f7490f79d77101392f9e66dc3f6ee358d218505124c",
    "slot": 29062271,
    "block": 5698710,
    "era": "Mary"
}

@newhoggy
Copy link
Contributor Author

On testnet:

CARDANO_NODE_SOCKET_PATH=example/node-bft2/node.sock cardano-cli query tip --testnet-magic 42
{
    "epoch": 11,
    "hash": "85d1f515ccede00557e28ff3fece650b509108806dfb255dee4e539bb3fe316a",
    "slot": 1112,
    "block": 1070,
    "era": "Byron"
}

Jimbo4350
Jimbo4350 reviewed May 10, 2021
View reviewed changes
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.

Nice work! This was a lot simpler than I thought it would be. A few comments below.

cardano-api/src/Cardano/Api/Query.hs Outdated
-> History.Interpreter xs
-> EraHistory mode

slotToEpoch :: SlotNo -> EraHistory mode -> Either Qry.PastHorizonException (EpochNo, Word64, Word64)
Copy link
Contributor

Choose a reason for hiding this comment

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

What are the two Word64s? A comment here would help.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added type aliases to help with that.

Copy link
Contributor

Choose a reason for hiding this comment

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

Where are the type aliases? Would it not be better the be a newtype ?

cardano-cli/src/Cardano/CLI/Shelley/Run/Query.hs Outdated Show resolved Hide resolved
cardano-cli/src/Cardano/CLI/Shelley/Run/Query.hs Outdated
@@ -201,6 +214,13 @@ runQueryTip (AnyConsensusModeParams cModeParams) network mOutFile = do
Aeson.Object $ obj <> HMS.fromList [name .= Aeson.Null]
toObject _ _ _ = Aeson.Null

toJsonPastHorizonException :: Qry.PastHorizonException -> Aeson.Value
Copy link
Contributor

Choose a reason for hiding this comment

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

We should propagate this through the existing error type ShelleyQueryCmdError. I think it would be strange to render some errors via ShelleyQueryCmdError and some via JSON output.

Copy link
Contributor Author

@newhoggy newhoggy May 10, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

It depends. If we go via ShelleyQueryCmdError if something wrong happens we get no values even if some queries succeed. If we do this instead, we will get partial data even when some queries fail.

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've taken the approach where if the epoch query fails, the JSON will be printed without the epoch and an error is logged to stdout.

@newhoggy newhoggy requested a review from Jimbo4350 May 10, 2021 08:01
@newhoggy newhoggy force-pushed the query-tip-epoch-on-byron branch 4 times, most recently from 5532d6d to 275f366 Compare May 11, 2021 03:05
Jimbo4350
Jimbo4350 approved these changes May 11, 2021
View reviewed changes
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.

Looking good! One minor comment

cardano-cli/src/Cardano/CLI/Shelley/Run/Query.hs Outdated
ShelleyQueryCmdUnsupportedMode mode -> "Unsupported mode: " <> renderMode mode
ShelleyQueryCmdPastHorizon e -> "Past horizon: " <> show e

renderMode :: AnyConsensusMode -> Text
Copy link
Contributor

Choose a reason for hiding this comment

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

This function already exists in: Cardano.CLI.Shelley.Run.Transaction

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've moved the function to a common module.

@newhoggy
Copy link
Contributor Author

bors merge

@iohk-bors
Copy link
Contributor

iohk-bors bot commented May 12, 2021

Build succeeded:

@iohk-bors iohk-bors bot merged commit 7821d2b into master May 12, 2021
@iohk-bors iohk-bors bot deleted the query-tip-epoch-on-byron branch May 12, 2021 09:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Jimbo4350 Jimbo4350 Jimbo4350 approved these changes

@erikd erikd erikd approved these changes

@DavidEichmann DavidEichmann Awaiting requested review from DavidEichmann

@dcoutts dcoutts Awaiting requested review from dcoutts

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

Successfully merging this pull request may close these issues.
3 participants
@newhoggy @erikd @Jimbo4350