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

PRT contracts unit tests #110

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Conversation

stephenctw
Copy link
Collaborator

@stephenctw stephenctw commented Feb 11, 2025

  • LeafTournament.sol is at 74% coverage, mostly because the solidity-step codes are not included in the scope.
  • 100% coverage for all other contracts.
  • Currently there are three kinds of commitments being tested
    • all leafs are 0
    • all leafs are 1
    • all leafs are 0 except the last leaf is 1
    • this covers the cases where
      • divergence happens at very first node (go down the tree to left always)
      • divergence happens at very last node (go down the tree to right always)
      • we could probably create more complex cases for later improvements.
  • (Optional) Run ./coverage.sh from prt/contracts folder, and open report/index.html in a browser can give you a very clear visual presentation of the test coverage report.

@stephenctw stephenctw force-pushed the feature/prt-solidity-test branch from 7badf23 to f7b67ea Compare February 11, 2025 06:37
@stephenctw stephenctw force-pushed the feature/prt-solidity-test branch from f7b67ea to b26ef84 Compare February 13, 2025 14:01
@stephenctw stephenctw self-assigned this Feb 14, 2025
Copy link
Collaborator

@GCdePaula GCdePaula left a comment

Choose a reason for hiding this comment

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

This is great :)

I've left a few comments. I'm ready to merge this, and if we feel the comments make sense, open a new PR.

There are a few granularities to testing, and I think we should test on all of them. On one end is more on the function level, and on the other end is testing the whole "executable". I think we should add more tests on the function level.

On this perspective, a few high level things we want to test, most of which I believe you're already testing:

  • Matchmaking: We can use single level tournament for this. We test join, for 1 to 4 players (in all of these numbers, something interesting happens). We test advance (the most important thing here is to also test the invalid cases). We test timeout. We test win by step (we don't need to test meta-step here, only test that the consequences of a successful step are what we want).
  • Match: For height zero, match for height one, match for height two. Here we should get to full coverage of all edge cases. As such, we won't need to test the details of a match anywhere else, we can assume it's working.
  • Meta-step: We don't have to instantiate and advance a whole dispute to the point of meta-step to test meta-step; we can test this function as a unit (feel free to move it to a library if you feel it helps!). Here we should test all corner cases of meta-step.
  • Multi-level: I guess this is the larger integration test. The main thing we haven't covered with the previous tests is on the "switch" between levels (both going down and going up). I think we need to also devise a clock test on this switch. We can assume what we tested before like the meta-step function works (we should make sure a match reaches the point of meta-step and calls it, but we don't need to test all the edge cases of meta-step here). Likewise, we can consider commitments with the minimum height, since we've already tested a match with varying heights.
  • Parallel inner tournaments: Integration test with 3 players. Integration test with 4 players (means that there would be two inner tournaments concurrently).

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 we should also test advanceMatch here. Like, I think this file alone should get to 100% coverage on Match.sol.

Now that we can easily set the tree/commitment height, it should be a lot easier.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is great! A few comments.

It's possible to test all the meta-step cases (compute step, rollup step, compute reset, rollup reset, gio) as units. Like, we don't have to create a top, middle, and bottom, doing bisections and sealing, just to get to meta-step. I believe we can also just directly test the meta-step function. Sometimes it's better to test a single thing at a time.

It's possible to create a BottomTournament without the top and middle. Just remember that it won't point to a valid "parent", but that's ok depending on the test.

As a sort of integration test, I love how it's possible to test a whole "refutation" (top to bottom) in a single function in a clear way. I think it might also be a good test to "finish" the tournament. Like, you managed to reach the bottom tournament and win the match --- is it possible to now "bubble up", reach the top tournament, vm.roll and win the dispute?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also, one case that's interesting to test is timeout. We should test what happens when we timeout on the top, then test bisect on top until middle and timeout on middle, and finally test bisect on top+middle and timeout on bottom.

(Not here on this file though, I think.)

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 the clock is the most delicate part of our code. I'll look more closely later, but it looks great :)

@GCdePaula GCdePaula self-requested a review February 17, 2025 12:20
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