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

Fix Arbitrary instance and invariant checking for Proposals #4016

Merged
merged 7 commits into from
Jan 30, 2024

Conversation

lehins
Copy link
Collaborator

@lehins lehins commented Jan 26, 2024

Description

  • Fix flaky cddl test triggered on CI
  • Fix invalid fallback implementation of Mirror downloading, which made into a flake setop on CI resulting in this failure
  • Fix flaky proposal tests that happened here and here. More on that below

Fix test data generation and improve invariant checking:

  • Fix Arbitrary instance for Proposals:

    • Allow empty to be generated
    • Generate InfoAction and TreasuryWithdrawal
  • Remove proposalsAreConsistent in favor of toPForest. Besides
    checking the invariant it also constructs a forest of Trees that
    can be used for debugging.

  • Invariant checking improved:

    • Remove invariant that pProps domain should equal to all children
      and that domain of pGraph equals to the domain of pProps
    • Enforce that all children are uniqe and that parents are correctly
      specified and matches the inverse relation from a parent to the
      child

toPForest not only checks the invariant, but serves as a nice debugging tool, since it allows us to print the hierarchy in a readable fashion, eg.:

{pfPParamUpdate =
x
, pfHardFork =
x
|
+- be4eaa8cc3a8e95fd0d02daaa55705a467f1327392dd7ea3749d128b8e0815c5#671359595
|
`- c0a4ef9bfeb257488525de250468e24b3c6340a6872ef366877c46a28a842c37#1165802016
   |
   `- 6cad5468f5320b34d411913292c2a40b1258fa11c7d5eecff6ad4b7325285422#1412147132
, pfCommittee =
a3e6fc1f9eec9a66c5ee56819f4a93658f6d018ef74d991d6d25242b8915d163#104556382
|
+- e6d969f5d7f10a8681aa8f9cf42328e98aaf653ec00353f340f37397c8181378#402911737
|  |
|  `- bca38b2ec2b04afb4d6fa61f3f1b8d5f3fbc77646626e4e7998b141cb41421fc#1225600682
|
`- e718c251f603af13b459fb22c4c0c0fdf03452ac06a3dbf45653ee87ce1c6ca7#1350459191
, pfConstitution =
f829acfb70d5a73396db0b1de29e34053cf80475b69382c9f390aef21e82d58a#767672592
|
`- 51f119927ec29d32df70c4bd90ba47b9542e59394ec9d8d5f8ffa1faebc56424#3360066490
   |
   +- 66a72530e4e79424a584e6f92f5a040f3b7122192622d06969e9318f4e5b49b8#45552015
   |
   +- 68b88d61580ee03573e0162a7546d1b5928b5798dad719ee40d2a801c77f279a#2339408668
   |
   `- dd04849e45e40f23775c0db04652641d8257593a7f5186625e3c836da57ecf66#179964133
}

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

@lehins lehins force-pushed the lehins/fixes-to-proposals branch from 3f7c89d to e3861b7 Compare January 29, 2024 12:03
@lehins lehins marked this pull request as ready for review January 29, 2024 12:04
@lehins lehins requested a review from aniketd January 29, 2024 12:14
@lehins lehins force-pushed the lehins/fixes-to-proposals branch from e3861b7 to a432bea Compare January 29, 2024 12:16
@lehins lehins requested a review from TimSheard January 29, 2024 15:10
Copy link
Contributor

@aniketd aniketd left a comment

Choose a reason for hiding this comment

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

Super awesome! 💯 🙌

@lehins lehins force-pushed the lehins/fixes-to-proposals branch from c8899d5 to 8b7cb1b Compare January 29, 2024 21:43
Reference for order paramter names should be derived from Plutus,
rather than listed in the file in the ledger repo. Starting with Conway
cost model parameters decoding is lenient, in that it accepts even
unknown parameters, so there is no point in listing all of the ones that
are known.
* Fix `Arbitrary` instance for `Proposals`:

   * Allow empty to be generated
   * Generate `InfoAction` and `TreasuryWithdrawal`

* Remove `proposalsAreConsistent` in favor of `toPForest`. Besides
  checking the invariant it also constructs a forest of `Tree`s that
  can be used for debugging.

* Invariant checking improved:

  * Remove invariant that `pProps` domain should equal to all children
    and that domain of `pGraph` equals to the domain of `pProps`
  * Enforce that all children are uniqe and that parents are correctly
    specified and matches the inverse relation from a parent to the
    child
@lehins lehins force-pushed the lehins/fixes-to-proposals branch from 8b7cb1b to eafb95a Compare January 29, 2024 23:40
@lehins lehins enabled auto-merge January 29, 2024 23:40
@lehins lehins merged commit 996231f into master Jan 30, 2024
11 of 27 checks passed
@neilmayhew neilmayhew deleted the lehins/fixes-to-proposals branch March 8, 2024 21:07
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