create-testnet-data: fix computation of not-delegated amount #638

smelc · 2024-03-07T14:49:09Z

Changelog

- description: |
    create-testnet-data: fixes that amount of delegated coins was incorrect
# 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 #631

How to trust this PR

Run the new test (cabal test cardano-cli-golden --test-options '-p "/golden create testnet data deleg non deleg/"') before the fix, witness the test fails
Run the test on the head of this PR. Enjoy the passing test.
Tested on cardano-node locally
Tested CI on cardano-node: Test of https://github.com/IntersectMBO/cardano-cli/pull/638, DO NOT MERGE cardano-node#5712

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

cardano-cli/test/cardano-cli-golden/Test/Golden/CreateTestnetData.hs

carbolymer · 2024-03-08T12:05:03Z

cardano-cli/test/cardano-cli-golden/Test/Golden/CreateTestnetData.hs

+    bs <- liftIO $ LBS.readFile $ outputDir </> "genesis.json"
+    genesis :: ShelleyGenesis StandardCrypto <- Aeson.throwDecode bs


You can use here https://hackage.haskell.org/package/hedgehog-extras-0.6.1.0/docs/Hedgehog-Extras-Test-File.html#v:readJsonFileOk

Done and learnt about readJsonFileOk 👍

carbolymer · 2024-03-08T12:07:14Z

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

@@ -622,7 +621,8 @@ updateOutputTemplate
    totalSupply = fromIntegral $ maybe maximumLovelaceSupply unLovelace mTotalSupply

    delegCoinRaw, nonDelegCoinRaw :: Integer
-    delegCoinRaw = case mDelegatedSupply of Nothing -> 0; Just (Lovelace amountDeleg) -> totalSupply - amountDeleg
+    delegCoinRaw = maybe 0 unLovelace mDelegatedSupply


carbolymer · 2024-03-08T12:11:51Z

cardano-cli/test/cardano-cli-golden/Test/Golden/CreateTestnetData.hs

+    let L.Coin onlyHolderCoin = snd $ initialFunds !! 0
+
+    -- The check below may seem weird, but we cannot do a very precise check
+    -- on balances, because of the treasury "stealing" some of the money.


What do you mean by that? I thought that treasury starts empty, and gets filled with time from reserves.

@carbolymer> I mean that: https://github.com/IntersectMBO/cardano-cli/blob/main/cardano-cli/src/Cardano/CLI/EraBased/Run/CreateTestnetData.hs#L608

carbolymer · 2024-03-08T12:12:27Z

cardano-cli/test/cardano-cli-golden/Test/Golden/CreateTestnetData.hs

+    -- of --total-supply above.
+    -- https://github.com/IntersectMBO/cardano-cli/issues/631
+
+    H.assert $ fromIntegral onlyHolderCoin > totalSupply `div` 2


Can you replace it with assertWith? assertWith logs the value on failure.

Done 👍 Thanks, I didn't know about assertWith 🙏

…eg no-inversion

Jimbo4350

👍

Jimbo4350 · 2024-03-27T00:37:18Z

cardano-cli/test/cardano-cli-golden/Test/Golden/CreateTestnetData.hs

+  propertyOnce $ moduleWorkspace "tmp" $ \tempDir -> do
+
+    let outputDir = tempDir </> "out"
+        totalSupply :: Int = 2000000000000 -- 2*10^12


We need to start using NumericUnderscores more

Actually we do now for this file on main 🎉

Jimbo4350 · 2024-03-27T13:34:44Z

cardano-cli/test/cardano-cli-golden/Test/Golden/CreateTestnetData.hs

+
+    let initialFunds = ListMap.toList $ L.sgInitialFunds genesis
+
+    (length initialFunds) H.=== 1


Some commentary would be useful here. Eg "This checks that there is actually only one funded address"
We can also introduce a variable e.g let fundedAddresses = 1 to make this even more clear.

Jimbo4350 · 2024-03-27T13:41:19Z

cardano-cli/test/cardano-cli-golden/Test/Golden/CreateTestnetData.hs

+    -- The check below may seem weird, but we cannot do a very precise check
+    -- on balances, because of the treasury "stealing" some of the money.
+    -- Nevertheless, this check catches a confusion between delegated and
+    -- non-delegated coins, by virtue of --delegated-supply being a fourth


This comment needs to be updated as #631 is closed

github-advanced-security bot found potential problems Mar 7, 2024

View reviewed changes

cardano-cli/test/cardano-cli-golden/Test/Golden/CreateTestnetData.hs Fixed Show fixed Hide fixed

cardano-cli/test/cardano-cli-golden/Test/Golden/CreateTestnetData.hs Fixed Show fixed Hide fixed

cardano-cli/test/cardano-cli-golden/Test/Golden/CreateTestnetData.hs Fixed Show fixed Hide fixed

smelc force-pushed the smelc/fix-deleg-non-deleg-confusion branch 5 times, most recently from e331fb2 to 309a844 Compare March 7, 2024 16:58

smelc marked this pull request as ready for review March 7, 2024 17:07

smelc requested review from Jimbo4350, newhoggy, carbolymer, kevinhammond and disassembler as code owners March 7, 2024 17:07

smelc mentioned this pull request Mar 8, 2024

Test of https://github.com/IntersectMBO/cardano-cli/pull/638, DO NOT MERGE IntersectMBO/cardano-node#5712

Closed

carbolymer approved these changes Mar 8, 2024

View reviewed changes

smelc force-pushed the smelc/fix-deleg-non-deleg-confusion branch 2 times, most recently from c710969 to 7890add Compare March 8, 2024 13:14

smelc added 5 commits March 8, 2024 14:16

create-testnet-data: test --total-supply is honored and deleg/non del…

2255c28

…eg no-inversion

create-testnet-data: fix deleg/nonDeleg confusion

c56c9fc

Adapt an existing test

87b9131

Detail: be more idiomatic

3fc125d

Detail: use readJsonFileOk

7efe1ee

smelc force-pushed the smelc/fix-deleg-non-deleg-confusion branch from 7890add to 7efe1ee Compare March 8, 2024 13:16

smelc added this pull request to the merge queue Mar 8, 2024

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Mar 8, 2024

smelc added this pull request to the merge queue Mar 8, 2024

Merged via the queue into main with commit eafcf17 Mar 8, 2024
19 checks passed

smelc deleted the smelc/fix-deleg-non-deleg-confusion branch March 8, 2024 19:39

Jimbo4350 reviewed Mar 27, 2024

View reviewed changes

smelc mentioned this pull request Mar 28, 2024

Follow-ups of merged PRs #683

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

create-testnet-data: fix computation of not-delegated amount #638

create-testnet-data: fix computation of not-delegated amount #638

smelc commented Mar 7, 2024 •

edited

Loading

carbolymer Mar 8, 2024

smelc Mar 8, 2024

carbolymer Mar 8, 2024

carbolymer Mar 8, 2024

smelc Mar 8, 2024

carbolymer Mar 8, 2024

smelc Mar 8, 2024

Jimbo4350 left a comment

Jimbo4350 Mar 27, 2024

smelc Mar 28, 2024

Jimbo4350 Mar 27, 2024

Jimbo4350 Mar 27, 2024

		bs <- liftIO $ LBS.readFile $ outputDir </> "genesis.json"
		genesis :: ShelleyGenesis StandardCrypto <- Aeson.throwDecode bs


		let initialFunds = ListMap.toList $ L.sgInitialFunds genesis

		(length initialFunds) H.=== 1

create-testnet-data: fix computation of not-delegated amount #638

create-testnet-data: fix computation of not-delegated amount #638

Conversation

smelc commented Mar 7, 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

Jimbo4350 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

smelc commented Mar 7, 2024 •

edited

Loading