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 create-testnet-data command #488

Merged
merged 4 commits into from
Dec 5, 2023
Merged

Add create-testnet-data command #488

merged 4 commits into from
Dec 5, 2023

Conversation

smelc
Copy link
Contributor

@smelc smelc commented Nov 28, 2023

Changelog

- description: |
    Add `create-testnet-data` command, that generates files for starting a testnet, with a UI that is more conveninent than `create-staked`
# uncomment types applicable to the change:
  type:
  - feature        # introduces a new feature
  # - breaking       # the API has changed in a breaking way
  - compatible     # the API has changed but is non-breaking
  # - optimisation   # measurable performance improvements
  # - improvement    # QoL changes e.g. refactoring
  # - bugfix         # fixes a defect
  # - test           # fixes/modifies tests
  # - maintenance    # not directly related to the code
  # - release        # related to a new release preparation
  # - documentation  # change in code docs, haddocks...

Context

Fixes #473

How to trust this PR

  • Code that is moved from Run/Genesis.hs to Run/CreateTestnetData.hs is not modified
  • Hence, existing behavior of Run/Genesis.hs is unchanged.
  • The structure of the files created by create-testnet-data is in a golden file, and it is consistent with what we said we would do.

Checklist

  • Commit sequence broadly makes sense and commits have useful messages
  • New tests are added if needed and existing tests are updated. See Running tests for more details
  • Self-reviewed the diff

@smelc smelc force-pushed the smelc/new-create-staked branch 5 times, most recently from a6bed88 to 32cbe4a Compare November 29, 2023 10:11
@smelc
Copy link
Contributor Author

smelc commented Nov 29, 2023

A question to reviewers:

  • Do we want the new create-testnet-data command to be available in all eras? Right now it is, I followed the same pattern as create-staked.

Comments on design choices:

  • This is v1 version. For example I intentionally did not tackle this comment nor this one. This can be done in later PRs and I wanted this one to be merged sooner than later. Long-running PRs are harder for authors (damn I should get this merged!) and for reviewers (this PR again!?).
  • I chose to share code between create-staked and this command when there was no change to it. Sharing was easier for me and is better for the code health in the long run, whether we ultimately delete create-staked or not. To be sure that the code that was moved was unchanged, run git diff --color-moved main..smelc/new-create-staked and look at the yellow and purple lines.

@smelc smelc marked this pull request as ready for review November 29, 2023 10:18
@smelc smelc force-pushed the smelc/new-create-staked branch from 32cbe4a to 5556418 Compare November 29, 2023 10:44
Copy link
Contributor

@carlhammann carlhammann left a comment

Choose a reason for hiding this comment

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

I left quite a few comments, but mostly nitpicks. This is still a messy bit of code, and there are a few things I still feel uneasy about (for example readGenDelegsMap: the way it's implemented to compute the intersection and differences of a few maps, and then apply another transformation to the result, the fact that we even need it (going from numbers to file names seems brittle), the weird error message containing lists of meaningless (to the user) numbers,...) .

Nonetheless, I think this is an improvement. Approved, thank you!

cardano-cli/src/Cardano/CLI/EraBased/Commands/Genesis.hs Outdated Show resolved Hide resolved
cardano-cli/src/Cardano/CLI/EraBased/Options/Genesis.hs Outdated Show resolved Hide resolved
cardano-cli/src/Cardano/CLI/EraBased/Options/Genesis.hs Outdated Show resolved Hide resolved
cardano-cli/src/Cardano/CLI/EraBased/Options/Genesis.hs Outdated Show resolved Hide resolved
@smelc smelc force-pushed the smelc/new-create-staked branch 5 times, most recently from de378d5 to aa49d92 Compare November 30, 2023 10:46
@smelc smelc force-pushed the smelc/new-create-staked branch 4 times, most recently from 6cb04c4 to 3836a7c Compare November 30, 2023 15:04
, numStakeDelegators :: !Word
, numStuffedUtxo :: !Word
, numUtxoKeys :: !Word
, supply :: !(Maybe Lovelace)
Copy link
Contributor

Choose a reason for hiding this comment

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

Maximum supply of lovelace right? A comment would be useful here too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's the maximum supply of non-delegated lovelace, not the total supply. I propose to discuss the final API we want on Slack, and follow-up with another PR.

pure newTemplate

-- Genesis keys
let mkPaths :: String -> String -> String -> Map 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.

It would be clearer to factor this out IMO and have a little comment as to what the map will be used for. Could be done in a follow up PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done 👍

@@ -500,7 +502,7 @@ updateCreateStakedOutputTemplate
]
, sgStaking =
ShelleyGenesisStaking
{ sgsPools = ListMap pools
{ sgsPools = ListMap . Set.toList $ pools
Copy link
Contributor

Choose a reason for hiding this comment

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

So we use ListMap for performance reasons. You'll need to ask @newhoggy if this is OK.

Copy link
Contributor

Choose a reason for hiding this comment

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

This could be a problem. Please avoid conversion from List to Set and back again, which I think is what happens. Please also ensure that the list can be evaluated lazily.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also double-check that you can create 30 million UTxOs on a 64G laptop: IntersectMBO/cardano-node#3938

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@newhoggy> I removed the conversion and will benchmark soon 👍

Copy link
Contributor

@carbolymer carbolymer left a comment

Choose a reason for hiding this comment

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

LGTM

@smelc smelc mentioned this pull request Dec 4, 2023
2 tasks
@smelc smelc force-pushed the smelc/new-create-staked branch 2 times, most recently from 9717978 to 4b0dbda Compare December 4, 2023 11:19
@smelc smelc mentioned this pull request Dec 4, 2023
3 tasks
@smelc smelc force-pushed the smelc/new-create-staked branch 2 times, most recently from f8ce9f1 to d39d9de Compare December 4, 2023 15:54
@smelc smelc force-pushed the smelc/new-create-staked branch from d39d9de to 9075f99 Compare December 4, 2023 21:35
@smelc smelc enabled auto-merge December 4, 2023 21:40
@smelc smelc added this pull request to the merge queue Dec 5, 2023
Merged via the queue into main with commit 5fcbb56 Dec 5, 2023
19 checks passed
@smelc smelc deleted the smelc/new-create-staked branch December 5, 2023 00:47
@smelc smelc mentioned this pull request Dec 6, 2023
6 tasks
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.

Revamp the create-staked command, to make its output easier to consume
5 participants