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

Moved action specific predicates to actionWellFormed and fix a conformance failure #673

Merged
merged 4 commits into from
Feb 11, 2025

Conversation

Soupstraw
Copy link
Contributor

@Soupstraw Soupstraw commented Feb 4, 2025

Description

This PR moves the action-specific predicates in GOV-Propose to actionWellFormed and adds a check that makes sure all the receiving staking credentials of a TreasuryWithdrawal are registered when the proposal is made.

This probably requires some changes to the prose. It seemed like the prose about actionWellFormed was already outdated before these changes, because it only mentioned we check the parameter updates, but we also had some conditions about treasury withdrawals as well.

I also added two other predicate checks to the GOV rule which are present in the implementation:

  • In GOV-Propose: the return address of a new proposal must be a registered staking credential
  • In GOV-Vote: the proposal being voted for must not be expired

Checklist

  • Commit sequence broadly makes sense and commits have useful messages
  • Any semantic changes to the specifications are documented in CHANGELOG.md
  • Code is formatted according to CONTRIBUTING.md
  • Self-reviewed the diff

@Soupstraw Soupstraw marked this pull request as ready for review February 4, 2025 13:41
Copy link
Collaborator

@WhatisRT WhatisRT left a comment

Choose a reason for hiding this comment

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

Actually, now that I look at it I think it's better to move it into a separate helper function. The idea is that actionWellFormed follows this pattern: #31 And that's destroyed by adding these extra arguments.

@Soupstraw Soupstraw marked this pull request as draft February 5, 2025 10:47
@Soupstraw Soupstraw force-pushed the jj/withdrawal-return-addr branch 2 times, most recently from a716f3a to 7c35621 Compare February 5, 2025 14:24
@Soupstraw Soupstraw marked this pull request as ready for review February 6, 2025 12:08
@Soupstraw Soupstraw force-pushed the jj/withdrawal-return-addr branch 2 times, most recently from 35e0909 to 10973d3 Compare February 7, 2025 10:33
Copy link
Collaborator

@WhatisRT WhatisRT left a comment

Choose a reason for hiding this comment

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

Looks pretty good, but actionValid and actionWellFormed are now in hidden code blocks, i.e. this removes actual logic from the PDF. I think just making the two functions but not the decision procedures visible is good enough right now, but it probably needs to be improved later.

Also, I'm not so sure about the usage of in the new functions. It makes sense, but it's not consistent with how we write non-STS predicates elsewhere. Maybe remove them for now and we can open a discussion about that in a new issue?

@Soupstraw Soupstraw force-pushed the jj/withdrawal-return-addr branch 3 times, most recently from 5e773c0 to ce34026 Compare February 7, 2025 15:16
@Soupstraw Soupstraw requested a review from WhatisRT February 7, 2025 15:17
@Soupstraw Soupstraw force-pushed the jj/withdrawal-return-addr branch from ce34026 to 90229ae Compare February 10, 2025 12:57
@williamdemeo
Copy link
Collaborator

Sorry @Soupstraw I didn't see this earlier. I'll review it today!

@williamdemeo
Copy link
Collaborator

williamdemeo commented Feb 10, 2025

I've moved actionValid and actionWellFormed out of the hidden code block and into a new figure. I also added prose near that figure to explain the logic, reduced the prose about this logic that was below the GOV rule figure (at the bottom of Gov.lagda), and replaced it with a reference to the new figure.

+ added some explanatory documentation
+ split figure involving types and functions used the GOV sts so they
fit on the page
+ added `\clearpage` commands to make prose line up better with the
figures they're explaining
Copy link
Collaborator

@williamdemeo williamdemeo left a comment

Choose a reason for hiding this comment

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

I just pushed my suggested changes directly to your branch since they merely reorganize and document the code a bit. Otherwise, looks really nice. Good work!

Copy link
Collaborator

@WhatisRT WhatisRT left a comment

Choose a reason for hiding this comment

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

LGTM

@WhatisRT WhatisRT merged commit c20574e into master Feb 11, 2025
8 of 10 checks passed
@WhatisRT WhatisRT deleted the jj/withdrawal-return-addr branch February 11, 2025 11:35
github-actions bot added a commit that referenced this pull request Feb 11, 2025
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.

3 participants