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

Make cardano create-testnet-data register DReps and delegate stake delegators to them #646

Merged
merged 1 commit into from
Mar 15, 2024
Merged

Make cardano create-testnet-data register DReps and delegate stake delegators to them #646

merged 1 commit into from
Mar 15, 2024

Conversation

palas
Copy link
Contributor

@palas palas commented Mar 12, 2024 •
edited

Changelog

- description: |
    Modified `create-testnet-data` option so that it registers DReps generated and delegates stake delegators to them. Also introduced transient drep delegation and refactored existing code so that more is reused.
# uncomment types applicable to the change:
  type:
  - feature        # introduces a new feature
  - improvement    # QoL changes e.g. refactoring

Context

In order to do realistic benchmarking we need to have DReps be registered and to exercise delegation, before this PR DReps were simply extra keys that were generated. We also want it to be possible to generate keys without writing them to disk (other than from the bit in the genesis file), so that tests scale better.

This PR depends on #482 in cardano-api in order to not re-add dependencies to cardano-cli. It may be a good idea to review both simultaneously.

How to trust this PR

Check that the conversion of the keys is done correctly, that the right things are being delegated, and that refactorings didn't alter the behaviour of existing code.

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

@palas palas added enhancement New feature or request conway-feature labels Mar 12, 2024
@palas palas self-assigned this Mar 12, 2024
@palas palas marked this pull request as ready for review March 12, 2024 02:17
@smelc
Copy link
Contributor

smelc commented Mar 12, 2024

@palas> Please complete this test to also test the new transient flags.

Probably a good opportunity to move this test to this non golden location, because it's actually not a golden test.

@smelc
Copy link
Contributor

smelc commented Mar 12, 2024

@palas> can you update this test to check basic things about the generated conway-genesis.json file? For example that the number of values inside initialDReps is the expected one?

smelc
smelc approved these changes Mar 12, 2024
View reviewed changes
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.

Great work @palas ❤️ I'm approving with the caveat that you add the tests I requested in comments above.

Also, can you augment the README file generated in the dreps keys folder, so that it mentions the new delegation relation?

cardano-cli/src/Cardano/CLI/EraBased/Run/CreateTestnetData.hs Outdated Show resolved Hide resolved
cardano-cli/src/Cardano/CLI/EraBased/Run/CreateTestnetData.hs Outdated Show resolved Hide resolved
cardano-cli/src/Cardano/CLI/EraBased/Commands/Genesis.hs Outdated Show resolved Hide resolved
carbolymer
carbolymer approved these changes Mar 13, 2024
View reviewed changes
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.

Nice work! Can you test this with cardano-testnet test suites?

cardano-cli/src/Cardano/CLI/EraBased/Run/CreateTestnetData.hs Outdated Show resolved Hide resolved
cardano-cli/test/cardano-cli-test-lib/Test/Cardano/CLI/Util.hs Show resolved Hide resolved
@smelc
Copy link
Contributor

smelc commented Mar 13, 2024

Also a small suggestion, would you mind using NumericUnderscores for large numbers?

Amen to that 😄 @palas> when you do so, you'll probably need to update some dependencies of cardano-node, to match the ones of cardano-cli. When you do that, can you PR it on cardano-node? I will benefit of it too.

@palas palas mentioned this pull request Mar 13, 2024
Merged
3 tasks
@smelc smelc mentioned this pull request Mar 14, 2024
Merged
4 tasks
@palas palas force-pushed the create-testnet-data-dreg branch 5 times, most recently from abd2a7a to 03b7872 Compare March 14, 2024 18:26
@palas palas mentioned this pull request Mar 14, 2024
Merged
3 tasks
palas pushed a commit to IntersectMBO/cardano-node that referenced this pull request Mar 15, 2024
@palas
Fix tests for IntersectMBO/cardano-cli#646
043c48c
@palas
Copy link
Contributor Author

palas commented Mar 15, 2024 •
edited

Can you test this with cardano-testnet test suites?

I have tested it now and it passes after doing these modifications: IntersectMBO/cardano-node@043c48c

@smelc smelc mentioned this pull request Mar 15, 2024
Closed
6 tasks
smelc
smelc approved these changes Mar 15, 2024
View reviewed changes
cabal.project Outdated Show resolved Hide resolved
@palas
Make cardano create-testnet-data register DReps and delegate stake de…
c028789 
…legators to them

- Create default conway genesis file and register DReps
- Delegate vote to DReps
- Add option to not write DReps to disk and update tests
- Pass template conway genesis spec as parameter
- Add alonzo genesis parameter with default
- Separate insecure key generation from pool assignation
- Change distribution algorithm so that remainder is distributed
- Refactor stake delegator creation to use same code for `OnDisk` and `Transient`
- Use generateInsecureSigningKey for generating DReps
- Improve generated README.md for DReps folder
- Move transient test and check no dir created
- Add basic checks about Conway genesis to test
- Invalidate Hydra cache
- Ensure deposit for DReps is at least what ConwayGenesis says
@palas palas enabled auto-merge March 15, 2024 20:53
@palas palas added this pull request to the merge queue Mar 15, 2024
Merged via the queue into main with commit fc9386e Mar 15, 2024
17 checks passed
@palas palas deleted the create-testnet-data-dreg branch March 15, 2024 22:34
This was referenced Mar 18, 2024
Closed
Closed
Jimbo4350
Jimbo4350 reviewed Mar 26, 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.

Nice 👍

@palas palas mentioned this pull request Mar 26, 2024
Merged
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Jimbo4350 Jimbo4350 Jimbo4350 left review comments

@carbolymer carbolymer carbolymer approved these changes

@smelc smelc smelc approved these changes

@newhoggy newhoggy Awaiting requested review from newhoggy newhoggy is a code owner

@kevinhammond kevinhammond Awaiting requested review from kevinhammond kevinhammond is a code owner

@disassembler disassembler Awaiting requested review from disassembler disassembler is a code owner

Assignees

@palas palas

Labels
conway-feature enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@palas @smelc @carbolymer @Jimbo4350