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

Add governance Info action test #5629

Merged
merged 1 commit into from
Feb 5, 2024

Conversation

carbolymer
Copy link
Contributor

@carbolymer carbolymer commented Jan 8, 2024

Description

Add a governance info testnet scenario.

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.

@carbolymer carbolymer force-pushed the mgalazyn/test/governance-info-action branch 2 times, most recently from 396e4a3 to e67fe96 Compare January 16, 2024 08:47
@carbolymer carbolymer force-pushed the mgalazyn/test/governance-info-action branch 4 times, most recently from cb8da1f to f9b1c99 Compare January 22, 2024 18:17
@carbolymer carbolymer marked this pull request as ready for review January 22, 2024 18:24
Comment on lines +394 to +365
let actions =
map snd
$ filter ((== govActionIdx) . fst)
$ map (first (\gai -> do
let (L.GovActionIx i) = L.gaidGovActionIx gai
i
))
$ Map.assocs $ L.proposalsGovActionStates proposals
case actions of
[L.GovActionState{L.gasDRepVotes=votes}]
| length votes == 3 -> ias{hasReceivedVotes = True}
Copy link
Contributor

Choose a reason for hiding this comment

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

Quite readable for what it does, 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.

Compared with the existing cardano-testnet/test/cardano-testnet-test/Cardano/Testnet/Test/LedgerEvents/Gov/ProposeNewConstitution.hs and this one works similarly 👍

One question though: don't we want to use query gov-state (or alike) instead of using foldBlocks at the end? I thought we were moving away from this foldBlock pattern, in its existing shape at least.

Comment on lines 124 to 135
void $ H.execCli' execConfig
[ "conway", "query", "utxo"
, "--address", Text.unpack $ paymentKeyInfoAddr $ head wallets
, "--cardano-mode"
, "--testnet-magic", show @Int testnetMagic
, "--out-file", work </> "utxo-1.json"
]

txin1 <- do
utxoJson <- H.leftFailM . H.readJsonFile $ work </> "utxo-1.json"
UTxO utxo <- H.noteShowM $ decodeEraUTxO sbe utxoJson
H.noteShow =<< H.headM (Map.keys utxo)
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you make this a function? This is used a number of times in this test, but also in others.

Making it a function would reduce the test's length and make the important part stand out a bit more.

@carbolymer
Copy link
Contributor Author

One question though: don't we want to use query gov-state (or alike) instead of using foldBlocks at the end? I thought we were moving away from this foldBlock pattern, in its existing shape at least.

@smelc an info action is not visible in the ledger state, not sure about gov-state. I can't use checkLedgerStateCondition, because I need to get notified about votes, new proposal, and the proposal getting removed. I can't do that using checkLedgerStateCondition function, because it's not returning anything from ledger state, so I can't aggregate on multiple conditions. We should expand checkLedgerState with this feature.

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!

{-# LANGUAGE ScopedTypeVariables #-}
{-# LANGUAGE TypeApplications #-}

{-# OPTIONS_GHC -Wno-unrecognised-pragmas #-}
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

afair HLint was complaining about !! 0, which is fine here in my opinion

]

-- Create Drep registration certificates
let drepCertFile :: Int -> FilePath
Copy link
Contributor

Choose a reason for hiding this comment

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

This is fine for now but we can inject the dreps into the genesis after the next release of api and cli. We will probably want a single standalone test that makes sure the cli can generate the certificates and successfully submit them.

@carbolymer carbolymer force-pushed the mgalazyn/test/governance-info-action branch from 0bcf75e to dfc7f4c Compare February 2, 2024 16:01
@smelc smelc mentioned this pull request Feb 2, 2024
3 tasks
@carbolymer carbolymer added this pull request to the merge queue Feb 2, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Feb 2, 2024
@carbolymer carbolymer added this pull request to the merge queue Feb 5, 2024
Merged via the queue into master with commit 1375419 Feb 5, 2024
17 of 19 checks passed
@carbolymer carbolymer deleted the mgalazyn/test/governance-info-action branch February 5, 2024 11:56
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