Skip to content

Conversation

@javierdiaz72
Copy link
Contributor

As the first task in implementing conformance testing with respect to the formal specification, we shall focus on making all the State Transition Systems (STSs) within the formal spec executable.

@javierdiaz72 javierdiaz72 added enhancement New feature or request formal-spec Changes related to formal specifications conformance Changes related to conformance testing labels Sep 27, 2024
@javierdiaz72 javierdiaz72 self-assigned this Sep 27, 2024
@javierdiaz72 javierdiaz72 marked this pull request as ready for review September 28, 2024 00:28
Copy link

@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.

One nice thing about the Computational class is that you can't make any wrong instances. It it type checks, it's correct. That being said, you could provide more informative error messages if you do (no ¬p) → failure (genErrors ¬p) instead of (no _) → failure "My error string". genErrors will branch on all the different possibilities for ¬p and generate an error message based on their type. They are really ugly, but they tell you what went wrong (and they'll never be incorrect if you move some things around).

@javierdiaz72 javierdiaz72 requested a review from a team September 30, 2024 17:04
@javierdiaz72
Copy link
Contributor Author

@WhatisRT: Thanks a lot for your review, I have just changed the code to use genErrors whenever possible (I got a type error when trying to use it for one of the STSs).

@IntersectMBO/ouroboros-consensus-maintainers: The PR is now ready to be merged.

Copy link
Contributor

@nfrisby nfrisby left a comment

Choose a reason for hiding this comment

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

  1. I did not explicitly check for missing logic. But I didn't notice any glaring omissions.
  2. I did not scrutinize all the chicken scratches... the conformance checking we're working towards has the wonderful benefit of bidirectionality, so I'm deferring the typo-hunting until we're guided by known disagreements.
  3. We prefer rebasing PR branches onto main instead of merging main into branches. Especially since this directory is your sovereign domain, I would expect a rebase is trivial for you to execute locally. (please eliminate the merge commit f65e92a)
  4. I would expect to find this kind of notation inscribed on the stone walls of an asylum cell.
module _ (M : Type↑) where
  _⁴ : ∀ {ℓ} {A B C D : Type ℓ} → (A → B → C → D → Type ℓ) → Type _
  _⁴ _~_~_~_ = ∀ {x y z w} → M (x ~ y ~ z ~ w)

_⁇⁴ = _⁇ ⁴

Just teasing :D, I'll be happy to understand it better some day (mumble mumble passing dictionaries implicitly?)

@javierdiaz72 javierdiaz72 force-pushed the javierdiaz72/make-stss-computable branch from 7be352f to a7f61ed Compare October 4, 2024 17:06
@javierdiaz72
Copy link
Contributor Author

@nfrisby:

please eliminate the merge commit f65e92a)

Done. but now my commits show up as unverified. How can we fix this so that we can merge the PR?

@amesgen
Copy link
Member

amesgen commented Oct 7, 2024

@nfrisby:

please eliminate the merge commit f65e92a)

Done. but now my commits show up as unverified. How can we fix this so that we can merge the PR?

Hmm, usually, rebasing should also cause the commits to be re-signed. If you don't have any other changes from this PR, I or someone else from the Consensus team can also rebase (only the committer needs to sign, not the author).

@jasagredo
Copy link
Contributor

You can use this @javierdiaz72

git rebase --exec 'git commit --amend --no-edit -n -S' -i HEAD~<N>

where <N> is the number of commits to sign, in your case 4.

@javierdiaz72 javierdiaz72 force-pushed the javierdiaz72/make-stss-computable branch from a7f61ed to 44131c1 Compare October 7, 2024 17:59
@javierdiaz72 javierdiaz72 added this pull request to the merge queue Oct 7, 2024
Merged via the queue into main with commit 16fa875 Oct 7, 2024
18 checks passed
@javierdiaz72 javierdiaz72 deleted the javierdiaz72/make-stss-computable branch October 7, 2024 18:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

conformance Changes related to conformance testing enhancement New feature or request formal-spec Changes related to formal specifications

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants