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

Allow legacy dereg certs #638

Merged
merged 1 commit into from
Jan 14, 2025
Merged

Allow legacy dereg certs #638

merged 1 commit into from
Jan 14, 2025

Conversation

Soupstraw
Copy link
Contributor

@Soupstraw Soupstraw commented Jan 7, 2025

Description

This PR changes the DELEG-dereg rule so that the deposit can be zero. In the implementation the deposit is optional and if it's not provided, we translate that to a deposit of zero. The implementation does that for backwards compatibility because the previous eras did not have a deposit field in the dereg cert.

closes #636

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 January 7, 2025 14:49
@@ -130,7 +130,7 @@ data _⊢_⇀⦇_,DELEG⦈_ where

DELEG-dereg :
∙ (c , 0) ∈ rwds
∙ (CredentialDeposit c , d) ∈ dep
d ≡ 0(CredentialDeposit c , d) ∈ dep
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it'd make more sense to change the type of the deposit so that we can check d ≡ nothing instead. I guess the idea here is that the deposit must be there because of the previous line, so we don't need to check that the credential appears in the deposits map. Still looks a bit weird though...

Copy link
Contributor

@williamdemeo williamdemeo Jan 13, 2025

Choose a reason for hiding this comment

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

@WhatisRT, just to be clear, are you suggesting line 133 should be

    ∙ d ≡ nothing ⊎ (CredentialDeposit c , d) ∈ dep

...and change the type of the deposit so the above type-checks?

I must admit, I don't fully understand this change (beyond maybe "that's what the implementation does"). @Soupstraw could you explain?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed the type of the deposit field to Maybe Coin and then added an additional check that the declared deposit is eihter nothing or just d, where d is equal to the deposit in the deposits map for the credential c. The former case is the "legacy" certificate which will be phased out in a future era and the latter is the one we want people to be using from Conway onwards.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But both certificates are accepted in Conway, so it makes sense to also talk about it in the spec, because people who implement smart contracts should be aware that the legacy certificates also exist.

@Soupstraw Soupstraw changed the title Permit dereg certs with a zero deposit Allow legacy dereg certs Jan 7, 2025
@Soupstraw Soupstraw marked this pull request as draft January 8, 2025 12:41
@Soupstraw Soupstraw marked this pull request as ready for review January 13, 2025 17:17
@Soupstraw Soupstraw requested a review from WhatisRT January 13, 2025 17:21
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. I just merged another PR though, so this needs a rebase

@WhatisRT
Copy link
Collaborator

Ah, you already did that, nevermind

@WhatisRT WhatisRT merged commit c72237b into master Jan 14, 2025
9 checks passed
@WhatisRT WhatisRT deleted the jj/dereg-deposit branch January 14, 2025 14:41
github-actions bot added a commit that referenced this pull request Jan 14, 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.

Conformance failure: Dereg cert deposit is optional in the implementation
3 participants