-
Notifications
You must be signed in to change notification settings - Fork 157
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
Add PParams for Conway #3498
Add PParams for Conway #3498
Conversation
29c7fda
to
d76c67c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great. Voting thresholds need some work still. There is a related spec PR was opened earlier today: IntersectMBO/formal-ledger-specifications#134
193fb95
to
d7b462f
Compare
LGTM, good job @aniketd! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please add:
- roundtrip tests for PParams and PParamsUpdates
- CDDL tests and spec for PParamsUpdates
There was a few bugs that I caught with regards to serialization, so tests would help us catch them and possibly others
bc895b7
to
8dee5f0
Compare
8dee5f0
to
2dd1826
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, just wondering about the minimumAVS
param that appears in the spec but not in the implementation, but probably something that I missed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Before merging, check that the version of cardano-ledger-conway is more than 1.6.1.1 (either by changing it in this PR or by rebasing, whichever is needed at the time of merging)
Actually nevermind, 1.6.1.1 is not on chap, my mistake (unless... someone bumps to 1.6.1.1 and releases to chap before you merge this 😆 )
d4770d2
to
7b9bb85
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome stuff
d0be5d9
to
7b20016
Compare
Description
Add
PParams
forConway
Fixes #3146
Checklist
.cabal
andCHANGELOG.md
files according to theversioning process.
.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)CHANGELOG.md
for the affected packages. New section is never added with the code changes. (See RELEASING.md)fourmolu
(usescripts/fourmolize.sh
)scripts/cabal-format.sh
)hie.yaml
has been updated (usescripts/gen-hie.sh
)