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

cardano-testnet: Avoid rewrite of sgMaxLovelaceSupply #5746

Merged
merged 2 commits into from
Apr 9, 2024

Conversation

smelc
Copy link
Contributor

@smelc smelc commented Apr 3, 2024

Description

Avoid this hacky rewrite that was necessary for the treasury growth test to pass (without the rewrite the treasury was negative).

How to trust this PR

The treasury growth test now passes without the genesis' rewrite (try it with PARALLEL_TESTNETS=1 DISABLE_RETRIES=1 cabal test cardano-testnet-test --test-options '-p "/Treasury Growth/"')

Checklist

  • Commit sequence broadly makes sense and commits have useful messages
  • CI passes. See note on CI.
  • Self-reviewed the diff

@smelc smelc marked this pull request as ready for review April 3, 2024 13:56
@smelc smelc requested a review from a team as a code owner April 3, 2024 13:56
@@ -137,8 +137,8 @@ createSPOGenesisAndFiles (NumPools numPoolNodes) era shelleyGenesis alonzoGenesi
, "--spec-conway", inputGenesisConwayFp
, "--testnet-magic", show testnetMagic
, "--pools", show numPoolNodes
, "--total-supply", show @Int 2_000_000_000_000
, "--delegated-supply", show @Int 1_000_000_000_000
, "--total-supply", show @Int 2_000_000_000_000 -- 2 trillions
Copy link
Contributor

@Jimbo4350 Jimbo4350 Apr 3, 2024

Choose a reason for hiding this comment

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

So we are overwriting the inputGenesisShelleyFp. We don't want to do this.

Copy link
Contributor

@Jimbo4350 Jimbo4350 Apr 3, 2024

Choose a reason for hiding this comment

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

It would be better to either:

  • Disallow values that result in a negative treasury
  • Add a check that will fail if the treasury in the node state is negative

We don't to hard code values unless its specifically contained in a default Haskell 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> Please note that this was the case before this PR, I'm not changing this behavior here 🙂

Disallow values that result in a negative treasury

Add a check that will fail if the treasury in the node state is negative

That was actually my first instinct, but alas, looking at the ledger's spec; it seems quite involved to catch cases that will cause treasury to be negative (at least with my level of knowledge) and also it seemed wrong to have this code here. I would favor reusing the ledger's core that does this (if any).

We don't to hard code values unless its specifically contained in a default Haskell value

Agreed, but I would rather do that in a follow-up PR. This PR is an improvement over the current state, so it makes sense as a first step.

Copy link
Contributor

@Jimbo4350 Jimbo4350 Apr 3, 2024

Choose a reason for hiding this comment

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

Please note that this was the case before this PR, I'm not changing this behavior here

Yep definitely wasn't blaming you. It was likely me who put those values there 🤡

Agreed, but I would rather do that in a follow-up PR.

👍

@Jimbo4350 Jimbo4350 self-requested a review April 3, 2024 20:28
@smelc smelc added this pull request to the merge queue Apr 4, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Apr 4, 2024
@smelc smelc added this pull request to the merge queue Apr 4, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Apr 4, 2024
@smelc smelc force-pushed the smelc/remove-max-lovelace-supply-rewrite branch from 116f78a to 40406dd Compare April 5, 2024 08:29
@smelc smelc enabled auto-merge April 5, 2024 08:30
@smelc smelc force-pushed the smelc/remove-max-lovelace-supply-rewrite branch from 40406dd to a3aaa72 Compare April 8, 2024 12:14
@smelc smelc force-pushed the smelc/remove-max-lovelace-supply-rewrite branch from a3aaa72 to 7397a3c Compare April 9, 2024 08:56
@Jimbo4350 Jimbo4350 disabled auto-merge April 9, 2024 14:59
@Jimbo4350 Jimbo4350 merged commit ea879a3 into master Apr 9, 2024
20 of 23 checks passed
@Jimbo4350 Jimbo4350 deleted the smelc/remove-max-lovelace-supply-rewrite branch April 9, 2024 15:00
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