Refactor: Use KeyPair type for key pairs in cardano-testnet #5829
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
carbolymer
force-pushed
the
mgalazyn/refactor/use-keypairs-in-testnet
branch
2 times, most recently
from
5607eb8
to
68e2c8b
Compare
carbolymer
commented
May 7, 2024
| shelleyKeyGen command subCommand tmpDir keyNames = | ||
| GHC.withFrozenCallStack $ do | ||
| let | ||
| vKeyPath = tmpDir </> verificationKeyFile keyNames |
There was a problem hiding this comment.
This was very misleading. We were only calling this function with an absolute paths in keyNames which resulted in tmpDir being completely ignored. That's a property of </>:
"some/leading/path" </> "/absolutepath.txt" == "/absolutepath.txt"
There was a problem hiding this comment.
Nice catch! How did you find this?
There was a problem hiding this comment.
Some time ago when I was refactoring path building in testnet, I was quite surprised with the results.
carbolymer
commented
May 7, 2024
| @@ -151,17 +117,8 @@ data ByronDelegationCert | |||
|
|
|||
| type TmpDir = FilePath | |||
|
|
|||
| newtype File a = File {unFile :: FilePath} | |||
There was a problem hiding this comment.
This name is confusing, as we're using API's File more frequently in testnet. I've opted to remove this type and to use API one.
carbolymer
force-pushed
the
mgalazyn/refactor/use-keypairs-in-testnet
branch
2 times, most recently
from
01370d3
to
cd71efa
Compare
carbolymer
force-pushed
the
mgalazyn/refactor/use-keypairs-in-testnet
branch
3 times, most recently
from
aed1a8a
to
105d6ff
Compare
carbolymer
commented
May 8, 2024
cardano-testnet/src/Testnet/Types.hs
Outdated
There was a problem hiding this comment.
Changes:
- Moved all types from
Testnet.Runtime - Removed redundant key pair types
- Added
KeyPair,VKey,SKey, moveSomeKeyPair - updated
configurationFiletype inTestnetRuntime
Jimbo4350
approved these changes
May 9, 2024
| => H.ExecConfig -- ^ Specifies the CLI execution configuration. | ||
| -> AnyCardanoEra -- ^ Specifies the current Cardano era. | ||
| -> FilePath -- ^ Base directory path where the signed transaction file will be stored. | ||
| -> String -- ^ Prefix for the output signed transaction file name. The extension will be @.tx@. | ||
| -> File TxBody In -- ^ Transaction body to be signed, obtained using 'createCertificatePublicationTxBody' or similar. | ||
| -> [k] -- ^ List of payment key pairs used for signing the transaction. | ||
| -> [SomeKeyPair] -- ^ List of payment key pairs used for signing the transaction. |
There was a problem hiding this comment.
The comment needs to be modified to show that we can sign with other keys (e.g stake keys)
| -> ShelleyBasedEra ConwayEra -- ^ The Shelley-based era (e.g., 'ConwayEra') in which the transaction will be constructed. | ||
| -> FilePath -- ^ Base directory path where generated files will be stored. | ||
| -> String -- ^ Name for the subfolder that will be created under 'work' folder. | ||
| -> PaymentKeyInfo -- ^ Wallet that will pay for the transaction. | ||
| -> StakingKeyPair -- ^ Staking key pair used for delegation. | ||
| -> PaymentKeyPair -- ^ Delegate Representative (DRep) key pair ('PaymentKeyPair') to which delegate. | ||
| -> KeyPair StakingKey -- ^ Staking key pair used for delegation. |
| shelleyKeyGen command subCommand tmpDir keyNames = | ||
| GHC.withFrozenCallStack $ do | ||
| let | ||
| vKeyPath = tmpDir </> verificationKeyFile keyNames |
There was a problem hiding this comment.
Nice catch! How did you find this?
Jimbo4350
reviewed
May 9, 2024
| Map.foldlWithKey' (compareWithTxId txid) Nothing (L.proposalsActionsMap proposals) | ||
| Nothing -> do | ||
| error $ "Eras mismatch! expected: " <> show sbe <> ", actual: " <> show actualEra | ||
| maybeExtractGovernanceActionIndex ceo txid (AnyNewEpochState actualEra newEpochState) = conwayEraOnwardsConstraints ceo $ do |
There was a problem hiding this comment.
Refactoring suggestion:
maybeExtractGovernanceActionIndex
:: HasCallStack
=> TxId -- ^ transaction id searched for
-> AnyNewEpochState
-> Maybe Word32
maybeExtractGovernanceActionIndex txid (AnyNewEpochState sbe newEpochState) =
caseShelleyToBabbageOrConwayEraOnwards
(const $ error "Governance actions only available in Conway era onwards")
(\ceo -> conwayEraOnwardsConstraints ceo $ do
let proposals = newEpochState ^. L.newEpochStateGovStateL . L.proposalsGovStateL
Map.foldlWithKey' (compareWithTxId txid) Nothing (L.proposalsActionsMap proposals)
)
sbe
where
compareWithTxId (TxId ti1) Nothing (GovActionId (L.TxId ti2) (L.GovActionIx gai)) _
| ti1 == L.extractHash ti2 = Just gai
compareWithTxId _ x _ _ = x
105d6ff
to
c7e7545
Compare
Jimbo4350
reviewed
May 9, 2024
| @@ -1,28 +1,25 @@ | |||
| {-# LANGUAGE DataKinds #-} | |||
| {-# LANGUAGE RankNTypes #-} | |||
There was a problem hiding this comment.
Could have removed RankNTypes here
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This PR looks big but it's mostly code style changes and wrapping and unwrapping from
File. The list of changes:KeyPair, replace with parameterized typeKeyPair kNodeConfigFile InandSocketPathfor node config file, and socket path respectively. Reduces required wrapping inFileon usage sites.Testnet.TypesTestnet.Process.Cliwhere we were abusing</>in building paths.Checklist
See Runnings tests for more details
CHANGELOG.mdfor affected package.cabalfiles are updatedhlint. See.github/workflows/check-hlint.ymlto get thehlintversionstylish-haskell. See.github/workflows/stylish-haskell.ymlto get thestylish-haskellversionghc-8.10.7andghc-9.2.7Note 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.