General feedback

[andreivladbrg]

As discussed privately I will leave my feedback here:

When naming the function tests, it should be "RevertWhen" instead of "RevertsWhen" (without the "s"). All functions should include underscores between the word "test" and "RevertWhen," or "test" and the function name. You can have a look at point 6 in the Foundry book's best practices.

When handling branches that are a bit more complex, bulloak prints more modifiers than needed. Let's take, for example, this tree I just wrote:

https://github.com/sablier-labs/v2-periphery/blob/feat/airstreams-linear/test/integration/airstream/campaign-ll/claim/claim.tree

The modifiers and functions should look like what I wrote here:

https://github.com/sablier-labs/v2-periphery/blob/feat/airstreams-linear/test/integration/airstream/campaign-ll/claim/claim.t.sol

What bulloak prints can be seen here: https://app.warp.dev/block/eKFFjxZ1wm3LinXNX1ZDRC

There shouldn't be a modifier for each branch inside the "when the claim is not included in the Merkle tree" branch; instead, the conditions should be at the end of the function.

Also, it would be really cool if you could manage to merge the modifiers and functions all together, rather than printing all the modifiers at the beginning and then all the functions. It would look like this:

    function test_RevertWhen_FirstCondition() external {
    }

    modifier whenNotFirstCondition() {
        _;
    }

    function test_RevertWhen_SecondCondition() external whenNotFirstCondition {
    }

    modifier whenNotSecondCondition() {
        _;
    }

    function test_Foo() external whenNotFirstCondition whenNotSecondCondition {
    }

Please let me know if you have any questions about these suggestions.

[alexfertel]

Thanks for the feedback @andreivladbrg ! Excited to iterate until we get something that matches your workflow at Sablier.

When naming the function tests, it should be "RevertWhen" instead of "RevertsWhen" (without the "s"). All functions should include underscores between the word "test" and "RevertWhen," or "test" and the function name. You can have a look at point 6 in the Foundry book's best practices.

Yes, this makes sense. I wasn't aware that this was a best practice.

There shouldn't be a modifier for each branch inside the "when the claim is not included in the Merkle tree" branch; instead, the conditions should be at the end of the function.

Hmmm, I think I need help understanding exactly this one. For the subtree bellow, we don't need either of whenTheIndexIsNotValid, whenTheRecipientAddressIsNotValid or whenTheMerkleProofIsNotValid, right?

      ├── when the claim is not included in the merkle tree
      │  ├── when the index is not valid
      │  │  └── it should revert
      │  ├── when the recipient address is not valid
      │  │  └── it should revert
      │  └── when the merkle proof is not valid
      │     └── it should revert

I guess the invariant here would be that leaf when nodes would not become modifiers. The problem is that you did create a modifier for this other part of the subtree:

      └── when the claim is included in the merkle tree
         ├── it should mark the index as claimed
         ├── it should create a lockup linear airstream
         └── it should emit a {Claim} event

That resulted in:

    modifier whenIncludedInMerkleTree() {
        _;
    }

    function test_Claim() external whenCampaignHasNotExpired whenNotClaimed whenIncludedInMerkleTree {
...
    }

So, it is not immediately clear to me what the criteria for generating the modifiers is. What are your thoughts?

Also, it would be really cool if you could manage to merge the modifiers and functions all together, rather than printing all the modifiers at the beginning and then all the functions

Yes, this makes total sense to me, I'm not sure why I didn't do this from the beginning.

Will create issues for the actionable items for now, and will work on them when I have some time.

---

PS: You have an extra test case not covered in the tree:

    function test_RevertWhen_InvalidAmount()
        external
        whenCampaignHasNotExpired
        whenNotClaimed
        whenNotIncludedInMerkleTree
    {
        uint256 index1 = defaults.INDEX1();
        uint128 invalidAmount = 1;
        bytes32[] memory merkleProof = defaults.index1Proof();
        vm.expectRevert(abi.encodeWithSelector(Errors.SablierV2AirstreamCampaign_InvalidProof.selector));
        campaignLL.claim(index1, users.recipient1.addr, invalidAmount, merkleProof);
    }

[andreivladbrg]

Thanks for the feedback @andreivladbrg ! Excited to iterate until we get something that matches your workflow at Sablier.

No worries, glad I can help.

Hmmm, I think I need help understanding exactly this one. For the subtree bellow, we don't need either of whenTheIndexIsNotValid, whenTheRecipientAddressIsNotValid or whenTheMerkleProofIsNotValid, right?

Yes, we don't need the modifiers enumerated because we declare another modifier, whenNotIncludedInMerkleTree, which will only be used for the functions in the subtree. After the subtree is finished, we will only use whenIncludedInMerkleTree instead of all three modifiers.

I guess the invariant here would be that leaf when nodes would not become modifiers.

I am not sure I understand this part.
The invariant would be that if ├── when is followed by │ ├── when (and not │ └── when, │ └── it, ├── it or └── it), then a modifier is created for the ├── when branch, which is included until its negation. Let's look at the example:

https://github.com/sablier-labs/v2-periphery/blob/e1f4c9d9a98303bc6a814b2a3b8cb04fe0508462/test/integration/airstream/campaign-ll/claim/claim.tree#L7-L20

    modifier whenNotIncludedInMerkleTree() {
        _;
    }

    function test_RevertWhen_InvalidIndex() external whenNotIncludedInMerkleTree { }

    function test_RevertWhen_InvalidRecipient() external whenNotIncludedInMerkleTree { }

    function test_RevertWhen_InvalidAmount() external whenNotIncludedInMerkleTree { }

    function test_RevertWhen_InvalidMerkleProof() external whenNotIncludedInMerkleTree { }

    modifier whenIncludedInMerkleTree() {
        _;
    }

    function test_Claim() external whenIncludedInMerkleTree { }

See, for the test_Claim() function, it's whenIncludedInMerkleTree and not whenNotIncludedInMerkleTree.

I hope it's cleared now.

PS: You have an extra test case not covered in the tree:

That's right, thank you🙌.

[alexfertel]

See, for the test_Claim() function, it's whenIncludedInMerkleTree and not whenNotIncludedInMerkleTree.

Yeah, that's what bulloak generates as well:

70  function testWhenTheClaimIsIncludedInTheMerkleTree()
71    external
72    whenTheCampaignHasNotExpired
73    whenTheRecipientHasNotClaimed
74    whenTheClaimIsIncludedInTheMerkleTree
75  {
76    // it should mark the index as claimed
77    // it should create a lockup linear airstream
78    // it should emit a {Claim} event
79  }

---

The invariant would be that if ├── when is followed by │ ├── when (and not │ └── when, │ └── it, ├── it or └── it), then a modifier is created for the ├── when branch, which is included until its negation.

Yeah, this is true right now as well:

15  modifier whenTheClaimIsNotIncludedInTheMerkleTree() {
16    _;
17  }
...
27  modifier whenTheClaimIsIncludedInTheMerkleTree() {
28    _;
29  }

---

I guess the invariant here would be that leaf when nodes would not become modifiers.

I am not sure I understand this part.

I'm trying to understand why you wouldn't generate a modifier for some when branches, and not for others. The invariant you stated is true in the current output of bulloak. The only difference I see is that for:

      │  ├── when the index is not valid
      │  │  └── it should revert
      │  ├── when the recipient address is not valid
      │  │  └── it should revert
      │  ├── when the amount is not valid
      │  │  └── it should revert
      │  └── when the merkle proof is not valid
      │     └── it should revert

you don't generate modifiers, but instead we add them to test function names, like in your example:

    function test_RevertWhen_InvalidIndex() external whenNotIncludedInMerkleTree { }

    function test_RevertWhen_InvalidRecipient() external whenNotIncludedInMerkleTree { }

    function test_RevertWhen_InvalidAmount() external whenNotIncludedInMerkleTree { }

    function test_RevertWhen_InvalidMerkleProof() external whenNotIncludedInMerkleTree { }

---

After writing this down, I think I see it now: If the leaf is a revert (it should revert) and it's the only child of a when node, then we don't generate a modifier and include it in the name. Is this aligned with your understanding? This would indeed generate what you have in the test contract. wdyt?

[andreivladbrg]

After writing this down, I think I see it now: If the leaf is a revert (it should revert) and it's the only child of a when node, then we don't generate a modifier and include it in the name. Is this aligned with your understanding? This would indeed generate what
you have in the test contract. wdyt?

It's not about the revert, it's about that there are no more child nodes. The depth of the sub-tree matters too. You can create modifiers for the leaves in the sub-tree, but they would only be used within that specific sub-tree, similar to how we handle the cancel function in v2-core.

[alexfertel]

it's about that there are no more child nodes.

Yeah, I see that, however:

In cancel you have: https://github.com/sablier-labs/v2-core/blob/main/test/integration/concrete/lockup/cancel/cancel.tree#L63-L93

And that generates: https://github.com/sablier-labs/v2-core/blob/main/test/integration/concrete/lockup/cancel/cancel.t.sol#L431-L487

And when there is no reentrancy is the last node (a leaf), however it does have a corresponding modifier whenNoSenderReentrancy

[alexfertel]

First and second issues addressed #13 #14

[alexfertel]

Okay, published the above two changes https://crates.io/crates/bulloak/0.2.1, so you can now install it locally.

[andreivladbrg]

Great 🙌

[PaulRBerg]

Main Issue

Hey guys, I have just been able to catch up with the discussion here. Great feedback @andreivladbrg, and thanks for your questions, @alexfertel - poking holes in our tests is the best way to improve them and make our implicit assumptions explicit.

In short, the issue is that at Sablier we have applied a seemingly inconsistent methodology for mapping leaf nodes (when or given) to their corresponding Solidity implementations. While this hasn't posed a problem until now due to the lack of automated solutions, agreeing upon a unified approach is necessary for Bulloak.

Detour: "It" Branches

The main issue is connected to another inconsistency regarding the multiplicity of "it" leaves (a point made by @mds1 in #22 (comment)). How should multiple "it"s be handled by Bulloak?

1. Generate one test_ function for each it
2. Generate one big test_ function for all its

There is no one-size-fits-all solution, unfortunately. There are cases where we've chosen the 1st approach in Sablier, and other cases when we've chosen the 2nd approach. It all depends upon the "topology" of the function under test.

In an ideal world, Bulloak would let users specify their preference via a CLI flag. But to make progress in this discussion, let's assume that we go with the 1st approach and generate one test-per-it-branch (1st approach).

Proposed Solution

It has been suggested above that the solution is related to whether the leaf is a revert or whether it is the only child of a when node. However, this is not the key differentiator.

What matters is whether the when node (or, equivalently, the given node) is part of a binary system, i.e., whether there is a companion when x is not branch after the when x is branch.

• If the node is part of a binary system, then we should absolutely generate a modifier
• If it is not, a modifier is not necessary, and the node content should be applied only to the function name

Examples

Let's look at two examples:

Claim¹

• The given branches are in a binary system, thus a modifier SHOULD be generated
• The when branches are not in a binary system, thus a modifier SHOULD NOT be generated

├── given the claim is not included in the Merkle tree
│  ├── when the index is not valid
│  │  └── it should revert
│  ├── when the recipient address is not valid
│  │  └── it should revert
│  ├── when the amount is not valid
│  │  └── it should revert
│  └── when the Merkle proof is not valid
│     └── it should revert
└── given the claim is included in the Merkle tree
   ...

Cancel²

The following when binary system SHOULD generate a modifier:

├── when there is reentrancy
│  ... 
└── when there is no reentrancy
   ...

Footnotes

1. https://github.com/sablier-labs/v2-periphery/blob/08dcd62cd946c7ea838e65a05551da13ed72cf28/test/integration/airstream/campaign-ll/claim/claim.tree ↩

2. https://github.com/sablier-labs/v2-core/blob/d1157b49ed4bceeff0c4e437c9f723e88c134d3a/test/integration/concrete/lockup/cancel/cancel.tree ↩

[alexfertel]

Thank you, Paul, this clears things up a lot 👍🏻

---

Follow-up questions

• What happens when the arity of the node is > 2? Take for example the following dummy spec:

├── given the claim is not included in the Merkle tree
│  ├── when the index is 0
│  │  └── it should revert // --------------------------------> First test
│  └── when the index is bigger than 0
│     └── it should do something // --------------------------> Second test
├── given the claim is included in a Verkle tree
│  ├── when the index is 0
│  │  └── it should revert // --------------------------------> Third test
│  └── when the index is valid
│     └── it should do something // --------------------------> Fourth test
└── given the claim is included in the Merkle tree
   ├── when the index is valid
   │  └── it should revert // --------------------------------> Fifth test
   └── when the index is not valid
      └── it should revert // --------------------------------> Sixth test

If it is not (as in not part of a binary system), a modifier is not necessary, and the node content should be applied only to the function name

In the above case, every given node should generate a modifier, right?

---

• For binary systems, do we generate a modifier for each branch or one modifier that we append to the positive branch? i.e. For:

   ├── when the index is valid
   │  └── it should revert
   └── when the index is not valid
      └── it should revert

Should we generate

    modifier whenTheIndexIsValid() {
        _;
    }

    function test_RevertWhen_TheIndexIsValid() external whenTheIndexIsValid { }
    
    function test_RevertWhen_TheIndexIsNotValid() external { }

---

One final note regarding:

inconsistency regarding the multiplicity of "it" leaves

Note that "it" leaves never generate a test unless they have no parent conditions. That is, "it" leaves always get grouped into the parent test.

      └── when the claim is included in the merkle tree
         ├── it should mark the index as claimed
         ├── it should create a lockup linear airstream
         └── it should emit a {Claim} event

Generates one test with all the it children as comments inside the test.

---

I know it's been a long discussion, but I think getting the defaults right is important, so I hope you guys don't mind helping me strike the right balance.

[mds1]

In an ideal world, Bulloak would let users specify their preference via a CLI flag

There is no one-size-fits-all solution, unfortunately. There are cases where we've chosen the 1st approach in Sablier, and other cases when we've chosen the 2nd approach. It all depends upon the "topology" of the function under test.

Can you given an example of when you'd choose one approach vs. the other? Personally I feel like I'd always prefer a single test for all it statements avoid the solidity getting too fragmented and hard to follow

• The given branches are in a binary system, thus a modifier SHOULD be generated
• The when branches are not in a binary system, thus a modifier SHOULD NOT be generated

├── given the claim is not included in the Merkle tree
│  ├── when the index is not valid
│  │  └── it should revert
│  ├── when the recipient address is not valid
│  │  └── it should revert
│  ├── when the amount is not valid
│  │  └── it should revert
│  └── when the Merkle proof is not valid
│     └── it should revert
└── given the claim is included in the Merkle tree

Hmm, technically aren't all given/when branches are part of a binary system, and sometimes that system is just implicit in the tree? Another way to write the above spec to make it explicit would be:

├── given the claim is not included in the Merkle tree
│  ├── when the index is not valid
│  │  └── it should revert
│  ├── when the index is valid BUT the recipient address is not valid
│  │  └── it should revert
│  ├── when the index is valid AND the recipient address is valid BUT the amount is not valid
│  │  └── it should revert
│  └── when the index is valid AND the recipient address is valid AND the amount is valid BUT the Merkle proof is not valid
│     └── it should revert
└── given the claim is included in the Merkle tree

or equivalently:

├── given the claim is not included in the Merkle tree
│  ├── when the index is not valid
│  │  └── it should revert
│  └── when the index is valid
│      ├── when the recipient address is not valid
│      │  └── it should revert
│      └── when the recipient address is valid
│          ├── when the amount is not valid
│          │  └── it should revert
│          └── when the amount is valid
│              └── when the Merkle proof is not valid
│                 └── it should revert
└── given the claim is included in the Merkle tree

Another framing here is that the number of given/when branches should always be 0, or >=1, at any level. If you have exactly 1, you're missing a requirement for the negation of it. But greater than 2 is ok because you might have e.g. "when x is 10, when x is 20, when x is anything else". This framing is nice because it's lintable, i.e. tooling can look for this to help find missing conditions.

But my last version of the spec contradicts that idea, because our final branch is when the Merkle proof is not valid. We can't have when the Merkle proof is valid to balance it because that violates the initial given the claim is not included in the Merkle tree. In reality, all the when conditions below that given are describing ways that initial condition can occur, and that given is not an explicit branch that exists in reality. So the problem here is that then initial given the claim is not included in the Merkle tree is really more of a Gherkin Scenario, and not a true a given/when. Potential solutions to this:

• Flatten the last branch to when the amount is valid BUT the Merkle proof is not valid (this is arguably a hacky workaround)
• Consider adding scenario support (TBD how this would look exactly) so you can just have a flat list of when statements to indicate all ways the scenario of "claim not in merkle tree" can occur (this is implicitly done in @PaulRBerg's example)
• Allow defining unreachable branches, this way the aforementioned type of linting can occur and pass, but bulloak would not generate tests for it:

│          └── when the amount is not valid
│              ├── when the Merkle proof is not valid
│              │  └── it should revert
│              └── when the Merkle proof is not valid
│                 └── Unreachable, because of "given the claim is not included in the Merkle tree"

[alexfertel]

+1 to unreachable branches

[PaulRBerg]

Replying to @alexfertel:

every given node should generate a modifier, right?

Yes.

For binary systems, do we generate a modifier for each branch or one modifier that we append to the positive branch?

We tended not to define the first modifier—only the 2nd. But I can't think of any major issues with defining both, especially if that simplifies the code generator in Bulloak.

Note that "it" leaves never generate a test unless they have no parent conditions. That is, "it" leaves always get grouped into the parent test.

Oh, I see. That's fine as well. As I've said in my previous comment, what matters on this head is to agree upon a deterministic approach (what specific approach is chosen is not so important).

I hope you guys don't mind helping me strike the right balance.

Not at all. I think we're doing important work for the Solidity space as a whole (if we manage to turn BTT+Bulloak into a well-known best practice).

[PaulRBerg]

Replying to @mds1:

Can you given an example of when you'd choose one approach vs. the other?

Great question. I was living with the impression that we had more cases where we wrote one-test-per-it-branch. But that isn't true. We've way more cases of "bundled-its", which test all the tree "it" branches.

Here's a random example of one-test-per-it-branch (few tests are like this):

https://github.com/sablier-labs/v2-periphery/blob/873c6ca4dda0881447f0004fa1fcb5d6f21c2d14/test/integration/archive/list/list.t.sol#L31-L41

Random example of bundled-its (the vast majority of our tests are like this):

https://github.com/sablier-labs/v2-core/blob/e626e6f9f9c460a8b42b3501de71debb684965b3/test/integration/concrete/comptroller/set-protocol-fee/setProtocolFee.t.sol#L43-L56

So now I'm much more in favor of the bundled-its approach.

Ia single test for all it statements avoid the solidity getting too fragmented and hard to follow

Agree

technically aren't all given/when branches are part of a binary system, and sometimes that system is just implicit in the tree?

That is an interesting point, but given/when branches are not always part of a binary system. To explain why, let's take the same example:

├── when the index is not valid
│  └── it should revert
├── when the index is valid BUT the recipient address is not valid
│  └── it should revert

First, you can get the order wrong while the tests are still passing. Suppose the index is never checked to be valid in the 2nd branch above because your Solidity implementation (in src) checks the recipient address before checking the index.

Second, there may be cases where it is impossible to disentangle the inputs. For instance, a test that checks the output of ecrecover. Though, I concede that in this case, it's more a question of "indifference" concerning the branching order rather than your preferred branching being incorrect. I could have written the "recipient address is not valid" first, and the test would still correspond to the src implementation.

But my last version of the spec contradicts that idea

That contradiction does not seem to be related to the hard-and-fast binary system point. It's just a matter of logical redundancy: if the claim is not included in the Merkle tree, it follows that the Merkle proof must be invalid (I've opened an issue to fix this in V2 Peripher). Thanks for your feedback!

I don't think that it is possible for Bulloak to detect subtle logical inferences like this. But this is good news for smart contract auditors! 😅

Allow defining unreachable branches, this way the aforementioned type of linting can occur and pass, but bulloak would not generate tests for it:

Interesting idea. I like it for the simple reason that it expands the spec space for end users. But I still regard the problem you referred to as a matter of logical inconsistency.

[alexfertel]

I got some good feedback about modifiers for leaf when nodes, so I decided to stop generating them since it seems we are gonna move away from that anyway.

[mds1]

Just to clarify: I think you're referring to the "duplication" where you'd have a modifier of whenSomethingIsTrue along with function test_WhenSomethingIsTrue() whenSomethingIsTrue external {}, since it's redundant to have that twice. And now we'll change that to remove the modifier and just have function test_WhenSomethingIsTrue() external {}. Is that correct?

[alexfertel]

Yeah, that's exactly it 👍🏻