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

create-testnet-data: better behavior for create-testnet-data --total-supply and --delegated-supply #874

Merged
merged 3 commits into from
Aug 28, 2024

Conversation

smelc
Copy link
Contributor

@smelc smelc commented Aug 23, 2024

Changelog

- description: |
    create-testnet-data's --total-supply option doesn't have a default anymore. The default value is to take the value from the shelley genesis file (if provided, otherwise this file is defaulted, so total supply comes from the default shelley genesis). create-testnet-data's --delegated-supply option doesn't have a default anymore. The default is to use half of the total supply.
# 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
  # - refactoring    # QoL changes
  - 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

Contributes to fixing IntersectMBO/cardano-node#5953

The API for create-testnet-data's total supply and delegated supply was bad:

  1. --total-supply had a default value. So we would never pick up the value coming from the shelley genesis file. Now the behavior is that, if --total-supply is specified, it overrides the value from the shelley genesis file. Because the shelley genesis file is defaulted, there is always a value available.
  2. --delegated-supply had a default value. This does not play well with the value of total supply coming from either the genesis file or --total-supply. Now the default is to take half of the total supply (no matter from where it comes from).

How to trust this PR

  • Tested with cardano-testnet at fecf2bf0c728060c0ff408d2a7fecc152c25dd4f

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

H.readJsonFileOk $ outputDir </> "shelley-genesis.json"

-- Because we don't test this elsewhere in this file:
(L.sgMaxLovelaceSupply outputGenesis) H.=== tweakedValue
Copy link
Contributor

Choose a reason for hiding this comment

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

Rather than checking a single value, why not do a golden test on the entire genesis value?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Jimbo4350> I hadn't done it initially, because the generated genesis file is not stable.

However, I brought cardano-node's redactJsonFieldsInFile function to redact the unstable fields, and I now made this a golden test.

Copy link
Contributor Author

@smelc smelc Aug 26, 2024

Choose a reason for hiding this comment

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

@Jimbo4350> I did that in an additional commit (that I will fixup later on), to make your review easier.

@smelc smelc force-pushed the smelc/fix-create-testnet-data-default-total-supply branch from 8c954a4 to 099cb78 Compare August 26, 2024 09:53
@smelc smelc force-pushed the smelc/fix-create-testnet-data-default-total-supply branch from 099cb78 to e765b47 Compare August 26, 2024 09:59
@smelc smelc force-pushed the smelc/fix-create-testnet-data-default-total-supply branch from e765b47 to 059b553 Compare August 28, 2024 09:06
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!

let outputDir = tempDir </> "out"
void $
execCardanoCLI
[ "conway"
Copy link
Contributor

Choose a reason for hiding this comment

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

We shouldn't hardcore the era here. Use eraToString.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually there is no era witness floating around here. This is the one place where we define the era. (and by the way, eraToString is not a thing in CLI nor API, it's a cardano-testnet thing)(which maybe we should move to API?).

So I'm merging like this.

--
-- This test checks that we use the total supply value from the shelley template when --total-supply
-- is not specified. It was broken in the past (see https://github.com/IntersectMBO/cardano-node/issues/5953).
hprop_golden_create_testnet_data_total_supply_template :: Property
Copy link
Contributor

Choose a reason for hiding this comment

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

The property name should be updated. It's a golden test of the output of create-testnet-data.

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 👍

@smelc smelc force-pushed the smelc/fix-create-testnet-data-default-total-supply branch from 059b553 to bc34881 Compare August 28, 2024 14:04
…y genesis + default delegated supply to half the total supply (no default anymore)
@smelc smelc force-pushed the smelc/fix-create-testnet-data-default-total-supply branch from bc34881 to 7ee805f Compare August 28, 2024 14:05
@smelc smelc enabled auto-merge August 28, 2024 14:05
@smelc smelc added this pull request to the merge queue Aug 28, 2024
Merged via the queue into main with commit 5e4a77f Aug 28, 2024
25 checks passed
@smelc smelc deleted the smelc/fix-create-testnet-data-default-total-supply branch August 28, 2024 16:45
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.

2 participants