create-testnet-data: better UX for supply arguments #581

smelc · 2024-01-19T16:41:23Z

Changelog

- description: |
    create-testnet-data: better UX for supply arguments: have a flag for total supply and delegated supply is a fraction of that.
# 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

As discussed with @CarlosLopezDeLara, the UI for --supply and --supply-delegated in create-staked was counter-intuitive. As @Jimbo4350 requested here, we can seize the occasion of introducing create-testnet-data to improve.

Corresponding cardano-node PR: IntersectMBO/cardano-node#5646

Fixes #574

How to trust this PR

Tests on cardano-node pass, see CI runs on Adapt to create-testnet-data change cardano-node#5646

Checklist

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

…e former afterwards

carbolymer · 2024-01-29T12:41:57Z

cardano-cli/src/Cardano/CLI/EraBased/Commands/Genesis.hs

-  , supply :: !(Maybe Lovelace) -- ^ The number of Lovelace to distribute over initial, non-delegating stake holders.
-  , supplyDelegated :: !(Maybe Lovelace) -- ^ The number of Lovelace to distribute over delegating stake holders.
+  , totalSupply :: !(Maybe Lovelace) -- ^ The total number of Lovelace
+  , delegatedSupply :: !(Maybe Lovelace) -- ^ The number of Lovelace being delegated


Much more understandable 👍🏻

does total supply indicate total maximum possible supply?

Humm not sure what you mean. totalSupply indicates the entire amount of currency in the testnet being created IIUC.

cardano-cli/src/Cardano/CLI/EraBased/Options/Genesis.hs

carbolymer · 2024-01-29T13:11:10Z

cardano-cli/src/Cardano/CLI/EraBased/Run/Genesis.hs

@@ -660,6 +661,75 @@ runGenesisCreateStakedCmd

 -- -------------------------------------------------------------------------------------------------

+updateOutputTemplate


Why this code is duplicated: https://github.com/IntersectMBO/cardano-cli/pull/581/files#diff-8cad416779bd68d2dc00788d194d042bec6fc12b8231d09f6169b911ab37c1a7L584 ?

@carbolymer> It's not duplicated: the code you refer to was previously shared was previously used both by create-staked and create-testnet-data. To keep create-staked's existing behavior, I unshared it, by copying the preexisting version to create-staked's file.

There's a lot of similar code, like utilities in where. Especially this funds distribution part:

distribute (nonDelegCoin - subtractForTreasury) nUtxoAddrsNonDeleg utxoAddrsNonDeleg ++ distribute (delegCoin - subtractForTreasury) nUtxoAddrsDeleg utxoAddrsDeleg ++ mkStuffedUtxo stuffedUtxoAddrs

Can you de-duplicate parts of this function?

Since we don't want to change behavior of create-staked and don't plan on improving it, I am more in favor of duplicating, so that we have ours hand more free for changing create-testnet-data in the future; if you don't mind.

I'm in favour of de-duplicating. I'm un-requesting changes. @Jimbo4350 your call.

Let's focus on create-testnet-data for now. Benchmarking depends on create-staked. We can revisit in the future and clean it up. I would rename it to something create testnet data specific though.

This function is not exported by the module, so I think its name doesn't need to be create-testnet-data specific.

carbolymer · 2024-01-29T13:13:20Z

cardano-cli/test/cardano-cli-golden/files/golden/help/alonzo_genesis_create-testnet-data.cli

+                                                        [--total-supply LOVELACE]
+                                                        [--delegated-supply LOVELACE]


Suggested change

[--total-supply LOVELACE]

[--delegated-supply LOVELACE]

[--supply-total LOVELACE]

[--supply-delegated LOVELACE]

I think supply prefix may be more intuitive. I'm not sure - current version is fine too.

Actually I agree with you, but I chose to stay consistent with the existing flags, e.g. in --drep-keys, --utxo-keys, and --genesis-keys; the prefix is the disambiguation. By having --supply-total and --supply-delegated, I stick to this convention 🤷

carbolymer

https://github.com/IntersectMBO/cardano-cli/pull/581/files#r1469571596

Not blocking on de-duplication

Jimbo4350 · 2024-02-01T22:14:02Z

cardano-cli/src/Cardano/CLI/EraBased/Run/CreateTestnetData.hs

-    delegCoin = maybe 0 fromIntegral amountDeleg
+    totalSupply, nonDelegCoin, delegCoin :: Integer
+    -- if --total-supply is not specified, supply comes from the template passed to this function:
+    totalSupply = fromIntegral (maybe maximumLovelaceSupply unLovelace mTotalSupply)


Why not modify: sgMaxLovelaceSupply = fromIntegral $ nonDelegCoin + delegCoin
to sgMaxLovelaceSupply = totalSupply

Good catch, done 👍

Jimbo4350 · 2024-02-01T22:17:30Z

cardano-cli/src/Cardano/CLI/EraBased/Run/Genesis.hs

@@ -660,6 +661,75 @@ runGenesisCreateStakedCmd

 -- -------------------------------------------------------------------------------------------------

+updateOutputTemplate


Let's focus on create-testnet-data for now. Benchmarking depends on create-staked. We can revisit in the future and clean it up. I would rename it to something create testnet data specific though.

Jimbo4350 · 2024-02-02T17:11:26Z

cardano-cli/src/Cardano/CLI/EraBased/Run/CreateTestnetData.hs

-    nonDelegCoin = totalSupply - delegCoin
+    totalSupply = maybe maximumLovelaceSupply unLovelace mTotalSupply
+    totalSupply', delegCoin, nonDelegCoin :: Integer
+    totalSupply' = fromIntegral totalSupply


I would remove totalSupply' and call fromIntegral within the definitions of other variables.

smelc mentioned this pull request Jan 19, 2024

create-testnet-data: various enhancements #575

Merged

3 tasks

smelc force-pushed the smelc/create-testnet-data-rework-supply-arguments branch from 86c135d to f87de12 Compare January 24, 2024 10:59

smelc mentioned this pull request Jan 24, 2024

Adapt to create-testnet-data change IntersectMBO/cardano-node#5646

Closed

smelc force-pushed the smelc/create-testnet-data-rework-supply-arguments branch from f87de12 to af8d6bf Compare January 24, 2024 14:24

smelc changed the base branch from main to smelc/fix-create-testnet-data-readme-failures January 24, 2024 14:24

smelc linked an issue Jan 24, 2024 that may be closed by this pull request

create-testnet-data/create-staked: Augment documentation of --supply and --supply-delegated flags #574

Closed

smelc mentioned this pull request Jan 24, 2024

Follow-ups on create-testnet-data v1 #506

Closed

6 tasks

smelc force-pushed the smelc/fix-create-testnet-data-readme-failures branch from 93f3929 to e83e4ec Compare January 25, 2024 13:16

Base automatically changed from smelc/fix-create-testnet-data-readme-failures to main January 25, 2024 15:10

smelc added 2 commits January 26, 2024 09:31

create-testnet-data: test minimal example

c6203ac

Unshare create-testnet-data/create-staked function, to only modify th…

1813191

…e former afterwards

smelc force-pushed the smelc/create-testnet-data-rework-supply-arguments branch from af8d6bf to d0c60ea Compare January 26, 2024 08:31

smelc marked this pull request as ready for review January 29, 2024 09:47

smelc requested review from Jimbo4350, newhoggy, carbolymer, kevinhammond and disassembler as code owners January 29, 2024 09:47

carbolymer reviewed Jan 29, 2024

View reviewed changes

cardano-cli/src/Cardano/CLI/EraBased/Options/Genesis.hs Outdated Show resolved Hide resolved

carbolymer reviewed Jan 29, 2024

View reviewed changes

cardano-cli/src/Cardano/CLI/EraBased/Options/Genesis.hs Outdated Show resolved Hide resolved

smelc force-pushed the smelc/create-testnet-data-rework-supply-arguments branch from 289d218 to 7c7f6cf Compare January 29, 2024 12:57

carbolymer reviewed Jan 29, 2024

View reviewed changes

carbolymer previously requested changes Jan 29, 2024

View reviewed changes

Jimbo4350 reviewed Feb 1, 2024

View reviewed changes

Jimbo4350 self-requested a review February 1, 2024 22:21

Jimbo4350 approved these changes Feb 1, 2024

View reviewed changes

Jimbo4350 reviewed Feb 2, 2024

View reviewed changes

smelc force-pushed the smelc/create-testnet-data-rework-supply-arguments branch from d3e6c97 to 303afd8 Compare February 2, 2024 19:25

smelc added 3 commits February 5, 2024 10:45

create-testnet-data: better UX for supply arguments

82cd079

Adapt golden files

4bd8b73

Detail: simplification in create-testnet-data's updateOutputTemplate

8c399c9

smelc force-pushed the smelc/create-testnet-data-rework-supply-arguments branch from 303afd8 to 8c399c9 Compare February 5, 2024 09:47

smelc enabled auto-merge February 5, 2024 09:47

smelc added this pull request to the merge queue Feb 5, 2024

Merged via the queue into main with commit 67e410a Feb 5, 2024
14 checks passed

smelc deleted the smelc/create-testnet-data-rework-supply-arguments branch February 5, 2024 10:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

create-testnet-data: better UX for supply arguments #581

create-testnet-data: better UX for supply arguments #581

smelc commented Jan 19, 2024 •

edited

Loading

carbolymer Jan 29, 2024

Jimbo4350 Feb 1, 2024

smelc Feb 2, 2024

carbolymer Jan 29, 2024

smelc Jan 29, 2024

carbolymer Jan 29, 2024

smelc Jan 29, 2024

carbolymer Jan 29, 2024

Jimbo4350 Feb 1, 2024

smelc Feb 2, 2024

carbolymer Jan 29, 2024

smelc Jan 29, 2024

carbolymer left a comment

Jimbo4350 Feb 1, 2024

smelc Feb 2, 2024

Jimbo4350 Feb 1, 2024

Jimbo4350 Feb 2, 2024

smelc Feb 2, 2024

		@@ -660,6 +661,75 @@ runGenesisCreateStakedCmd

		-- -------------------------------------------------------------------------------------------------

		updateOutputTemplate

create-testnet-data: better UX for supply arguments #581

create-testnet-data: better UX for supply arguments #581

Conversation

smelc commented Jan 19, 2024 • edited Loading

Changelog

Context

How to trust this PR

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carbolymer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smelc commented Jan 19, 2024 •

edited

Loading