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

Update checkStakeKeyRegistered to use foldEpochState #5735

Merged

Conversation

Jimbo4350
Copy link
Contributor

Description

Add your description here, if it fixes a particular issue please provide a
link
to the issue.

Checklist

  • Commit sequence broadly makes sense and commits have useful messages
  • New tests are added if needed and existing tests are updated. These may include:
    • golden tests
    • property tests
    • roundtrip tests
    • integration tests
      See Runnings tests for more details
  • Any changes are noted in the CHANGELOG.md for affected package
  • The version bounds in .cabal files are updated
  • CI passes. See note on CI. The following CI checks are required:
    • Code is linted with hlint. See .github/workflows/check-hlint.yml to get the hlint version
    • Code is formatted with stylish-haskell. See .github/workflows/stylish-haskell.yml to get the stylish-haskell version
    • Code builds on Linux, MacOS and Windows for ghc-8.10.7 and ghc-9.2.7
  • Self-reviewed the diff

Note on CI

If your PR is from a fork, the necessary CI jobs won't trigger automatically for security reasons.
You will need to get someone with write privileges. Please contact IOG node developers to do this
for you.

@Jimbo4350 Jimbo4350 force-pushed the jordan/update-checkStakeKeyRegistered-to-use-foldEpochState branch 4 times, most recently from 1b9cbaa to 2a31775 Compare March 28, 2024 21:27
@Jimbo4350 Jimbo4350 changed the title Update check stake key registered to use foldEpochState Update checkStakeKeyRegistered to use foldEpochState Mar 28, 2024
@Jimbo4350 Jimbo4350 marked this pull request as ready for review March 28, 2024 21:28
@Jimbo4350 Jimbo4350 requested a review from a team as a code owner March 28, 2024 21:28
Comment on lines 123 to 130
H.failWithCustom GHC.callStack Nothing
$ unlines [ "Stake address in question: "
, Text.unpack (serialiseToBech32 sAddr)
, "was not registered"
, "Current stake info for address in question: "
, show $ map serialiseToBech32 $ Map.keys rewardsMap
, "foldEpochStateError: " <> show e
]
Copy link
Contributor

Choose a reason for hiding this comment

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

Providing a detailed error message, nice 👍

Copy link
Contributor

@smelc smelc left a comment

Choose a reason for hiding this comment

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

I'm not familiar with the part of the ledger's state being accessed, but this LGTM

Comment on lines 120 to 122
pledgerStakeInfo <- H.leftFailM $ H.readJsonFile oFpAbs
(DelegationsAndRewards (rewardsMap, _delegMap))
<- H.noteShowM $ H.jsonErrorFail $ Aeson.fromJSON @DelegationsAndRewards pledgerStakeInfo
Copy link
Contributor

Choose a reason for hiding this comment

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

can be simplified a bit

Suggested change
pledgerStakeInfo <- H.leftFailM $ H.readJsonFile oFpAbs
(DelegationsAndRewards (rewardsMap, _delegMap))
<- H.noteShowM $ H.jsonErrorFail $ Aeson.fromJSON @DelegationsAndRewards pledgerStakeInfo
(DelegationsAndRewards (rewardsMap, _delegMap)) <- H.noteShowM $ H.readJsonFileOk oFpAbs

Comment on lines 363 to 381
-- TODO: Currently we can't propagate the error message thrown by checkStakeKeyRegistered when using byDurationM
-- Instead we wait 15 seconds
Copy link
Contributor

Choose a reason for hiding this comment

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

This TODO is obsolete now

Suggested change
-- TODO: Currently we can't propagate the error message thrown by checkStakeKeyRegistered when using byDurationM
-- Instead we wait 15 seconds
-- TODO: Currently we can't propagate the error message thrown by checkStakeKeyRegistered when using byDurationM
-- Instead we wait 15 seconds

@Jimbo4350 Jimbo4350 force-pushed the jordan/update-checkStakeKeyRegistered-to-use-foldEpochState branch from 2a31775 to 537398e Compare April 2, 2024 13:41
@Jimbo4350 Jimbo4350 enabled auto-merge April 2, 2024 13:42
@Jimbo4350 Jimbo4350 disabled auto-merge April 2, 2024 14:51
@Jimbo4350 Jimbo4350 enabled auto-merge April 2, 2024 14:56
@Jimbo4350 Jimbo4350 force-pushed the jordan/update-checkStakeKeyRegistered-to-use-foldEpochState branch from 315143e to 4d2913c Compare April 2, 2024 16:40
@Jimbo4350 Jimbo4350 added this pull request to the merge queue Apr 2, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Apr 2, 2024
@Jimbo4350 Jimbo4350 added this pull request to the merge queue Apr 3, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Apr 3, 2024
@carbolymer carbolymer added this pull request to the merge queue Apr 8, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Apr 8, 2024
@carbolymer
Copy link
Contributor

@Jimbo4350 Jimbo4350 force-pushed the jordan/update-checkStakeKeyRegistered-to-use-foldEpochState branch 2 times, most recently from df25397 to 4d11b5e Compare April 9, 2024 15:08
@Jimbo4350 Jimbo4350 enabled auto-merge April 9, 2024 15:08
@Jimbo4350 Jimbo4350 force-pushed the jordan/update-checkStakeKeyRegistered-to-use-foldEpochState branch from 4d11b5e to 71537f5 Compare April 18, 2024 17:44
@Jimbo4350 Jimbo4350 added this pull request to the merge queue Apr 18, 2024
Merged via the queue into master with commit a770f8d Apr 18, 2024
22 checks passed
@Jimbo4350 Jimbo4350 deleted the jordan/update-checkStakeKeyRegistered-to-use-foldEpochState branch April 18, 2024 20:14
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.

3 participants