Imptests - treasury withdrawals

[teodanciu]

Description

This is the first part of @aniketd 's PR: https://github.com/IntersectMBO/cardano-ledger/pull/4179/commits, closing: #4133

I have re-arranged the test a little bit and added his commit that is actually adding the tests, and improved the tests with the comments from the review.

Checklist

• Commit sequence broadly makes sense and commits have useful messages
• New tests are added if needed and existing tests are updated
• When applicable, versions are updated in .cabal and CHANGELOG.md files according to the
  versioning process.
• The version bounds in .cabal files for all affected packages are updated. If you change the bounds in a cabal file, that package itself must have a version increase. (See RELEASING.md)
• All visible changes are prepended to the latest section of a CHANGELOG.md for the affected packages. New section is never added with the code changes. (See RELEASING.md)
• Code is formatted with fourmolu (use scripts/fourmolize.sh)
• Cabal files are formatted (use scripts/cabal-format.sh)
• hie.yaml has been updated (use scripts/gen-hie.sh)
• Self-reviewed the diff

[teodanciu]

Sorry, the last two commits were already reviewed as part of #4201, but the base branch was this one instead of master.

Accidental approval

[Lucsanszky]

I'm currently reviewing this and I intend to finish the review later today. I have some questions and minor suggestions.

[Lucsanszky]

Sorry, it took me long enough! 😬 Overall, I quite like it but since I'm new to the codebase, I felt that some more comments and assertions could improve the readability of the tests. However, my additions might be a bit too verbose so feel free to change or ignore them. Especially, if I didn't get the jargon right. :)

I mainly looked at the Treasury withdrawals spec, here is the gist of my suggestions:
I felt that it might be useful to also assert for the presence/absence of both the current proposals and previous ones (so the ones that are in the pulser), so I added those assertions as suggestions and also added an expectNoChange assertion that checks multiple things when a Treasury Withdrawal governance action's pre-condition is not met and hence the proposal will not be enacted as a result. Hopefully, I did not misunderstand the state changes and my suggestions are indeed helpful rather than misleading.

It seems like GitHub doesn't support suggestions over deleted lines so sometimes I had to separate suggestions into Step 1 and Step 2.

I was thinking about reorganising and rewording the its and describes as I would have preferred something along the lines of this:

        describe "Treasury withdrawals"
          it "modify EnactState as expected"
          describe "in a single proposal"
            it "should only be enacted when treasury has sufficient funds"
            it "should not exceed maxBound Word64"
          describe "in several proposals within the same epoch"
            it "should only be enacted until treasury has sufficient funds"

However, it seems like we don't really use this style throughout the codebase, so I opted to leave things intact and I also try to avoid too many nested describes.

[teodanciu]

@Lucsanszky Sorry I'm a bit lost about your comments with Step 1 and Step 2.
Do you mean, we should extract the check that the withdrawal proposal had no effect? So basically:

      getTreasury `shouldReturn` initialTreasury
      sumRewardAccounts withdrawals `shouldReturn` zero
```  ? 

[Lucsanszky]

@Lucsanszky Sorry I'm a bit lost about your comments with Step 1 and Step 2. Do you mean, we should extract the check that the withdrawal proposal had no effect? So basically:

      getTreasury `shouldReturn` initialTreasury
      sumRewardAccounts withdrawals `shouldReturn` zero
```  ? 

Yes, exactly. I added it because I also added the proposal assertions and at that point I thought that it might worth extracting the whole thing.

[aniketd]

Just a few suggestions for better test-naming than I originally thought.

[lehins]

Looks good overall. Just needs to be finished up and comments need to be addressed

[lehins]

Looks great. Thank you!
I made a minor suggestion, but feel free to ignore it