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

cardano-testnet | Add stake address registration/deregistration test #6017

Merged
merged 6 commits into from
Oct 28, 2024
Merged

cardano-testnet | Add stake address registration/deregistration test #6017

merged 6 commits into from
Oct 28, 2024

Conversation

carbolymer
Copy link
Contributor

@carbolymer carbolymer commented Oct 18, 2024 •
edited
Loading

Description

Reproducer for: IntersectMBO/cardano-cli#942

Adds also a few refactors:

  • Use KeyPair k type in more places
  • Replace SomeKeyPair with Some KeyPair
  • Read required key deposit from protocol parameters instead of hardcoding it
  • Use createStakeKeyDeregistrationCertificate and createStakeKeyRegistrationCertificate instead of manually building cardano-cli command.

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/add-stake-address-registration-deregistration-test branch from 4c3f082 to c9906ef Compare October 18, 2024 14:01
@carbolymer carbolymer mentioned this pull request Oct 18, 2024
Closed
3 tasks
@carbolymer carbolymer force-pushed the mgalazyn/test/add-stake-address-registration-deregistration-test branch 4 times, most recently from 31bebc9 to da2f968 Compare October 21, 2024 12:03
@carbolymer carbolymer changed the title Add stake address registration/deregistration test cardano-testnet | Add stake address registration/deregistration test Oct 22, 2024
@carbolymer carbolymer force-pushed the mgalazyn/test/add-stake-address-registration-deregistration-test branch 5 times, most recently from 5f5bcf5 to a849ea3 Compare October 23, 2024 17:18
@carbolymer carbolymer marked this pull request as ready for review October 23, 2024 17:21
@carbolymer carbolymer requested a review from a team as a code owner October 23, 2024 17:21
@carbolymer carbolymer force-pushed the mgalazyn/test/add-stake-address-registration-deregistration-test branch from a849ea3 to a3bb676 Compare October 24, 2024 13:40
@carbolymer carbolymer force-pushed the mgalazyn/test/add-stake-address-registration-deregistration-test branch from a3bb676 to 6fa52d3 Compare October 24, 2024 16:10
Jimbo4350
Jimbo4350 approved these changes Oct 25, 2024
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.

LGTM. A few comments that can be addressed in follow up PRs.

cardano-testnet/src/Testnet/Process/Cli/SPO.hs Outdated Show resolved Hide resolved
.../cardano-testnet-test/Cardano/Testnet/Test/Cli/Transaction/RegisterDeregisterStakeAddress.hs Outdated Show resolved Hide resolved
.../cardano-testnet-test/Cardano/Testnet/Test/Cli/Transaction/RegisterDeregisterStakeAddress.hs Outdated
do
(_, stdout', stderr') <- execCliAny execConfig
[ eraName, "query", "stake-address-info"
, "--address", stakeAddress
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 confirm that this command succeeds with the expected output.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This one was actually a leftover from debugging. It's removed in the final version. The query itself should be tested in Cardano.Testnet.Test.Cli.Query, which currently is not (needs fixing).

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 check in: #6026

.../cardano-testnet-test/Cardano/Testnet/Test/Cli/Transaction/RegisterDeregisterStakeAddress.hs
]

void $ execCli' execConfig
[ eraName, "transaction", "submit"
Copy link
Contributor

Choose a reason for hiding this comment

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

Again we should confirm that the stake address was actually unregistered.

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 check in: #6026

cardano-testnet/src/Testnet/Process/Cli/SPO.hs
@@ -52,10 +52,10 @@ checkStakePoolRegistered
:: (MonadTest m, MonadCatch m, MonadIO m, HasCallStack)
=> TmpAbsolutePath
-> ExecConfig
-> FilePath -- ^ Stake pool cold verification key file
-> File (VKey StakeKey) In -- ^ Stake pool cold verification key file
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be helpful to include the fact that this is a cold key in the type.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, that's a typo. I've used StakePoolKey type instead in #6026

cardano-testnet/src/Testnet/Types.hs
@@ -91,15 +92,20 @@ instance MonoFunctor (KeyPair k) where
deriving instance Show (KeyPair k)
deriving instance Eq (KeyPair k)

instance {-# OVERLAPPING #-} Show (Some KeyPair) where
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 due to:

instance Show (Some f) where
  showsPrec _ (Some v) = showsTypeRep (typeOf v)

I don't think it's used anywhere so lets remove it to avoid having to use OVERLAPPING.

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 agree, I think it will be better to provide specific show instances e.g. Show (Some Era instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Follow-up pr: IntersectMBO/cardano-api#665

@carbolymer carbolymer enabled auto-merge October 28, 2024 08:13
@carbolymer carbolymer added this pull request to the merge queue Oct 28, 2024
Merged via the queue into master with commit ee8501c Oct 28, 2024
23 of 25 checks passed
@carbolymer carbolymer deleted the mgalazyn/test/add-stake-address-registration-deregistration-test branch October 28, 2024 09:03
This was referenced Oct 28, 2024
Merged
Merged
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

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

Successfully merging this pull request may close these issues.
2 participants
@carbolymer @Jimbo4350