alexfertel/bulloak

feature request: Add support for actions without conditions.

alexfertel opened this issue · 10 comments

alexfertel commented

Discussed in #18

Originally posted by mds1 August 28, 2023
Bulloak looks really nice and I'm excited to try it out. I have a function like this OZ function:

function _efficientHash(bytes32 a, bytes32 b) internal pure returns (bytes32 hash) {
    assembly ("memory-safe") {
        mstore(0x00, a)
        mstore(0x20, b)
        hash := keccak256(0x00, 0x40)
    }
}

There's no branching or conditionals here so I expected my .tree file to look very simple:

MyContract._efficientHash.sol
├── it should match the output of a high-level hash
└── it should never revert

But running bulloak against this errors with:

•••••••••••••••••••••••••••••••••••••••••••••••••••••••••••••••••••••••••••••••
bulloak error: found an action without conditions

├── it should match the output of a high-level hash
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

--- (line 2, column 1) ---•••••••••••••••••••••••••••••••••••••••••••••••••••••••••••••••••••••••••••••••
bulloak error: found an action without conditions

└── it should never revert
^^^^^^^^^^^^^^^^^^^^^^^^^^

I could add a "when called" condition, but this would add unnecessary clutter to the tree and tests since there's no conditions or paths in this method.

Should bulloak/BTT support specs of this style, or should I be doing things differently here? I think a good approach might be to only allow this IF all requirements (it should lines) are at the shallowest level and not preceded by any given's or when's.

Related, I'd like to see a bit more info in the README on the requirements around .tree files, e.g. when are when and given required, can you only have one of the two, can their order be flipped or must it be when > given, etc. The reason I ask here is I want to mirror the given > when > then flow of cucumber vs. the when > given > it should of BTT (perhaps both can be supported?)

cc @PaulRBerg for thoughts too

Instructions:

  • Stop erroring in the semantic analysis phase when we find an Ast::Action while visiting the root.

    bulloak/src/semantics.rs

    Lines 157 to 160 in 3b4c0dc

    Ast::Action(action) => {
    self.errors
    .push(self.error(action.span, ErrorKind::ActionWithoutConditions));
    }
  • Add support for emitting a test that has no parent condition.
    • This one probably requires a non-trivial refactor.

      bulloak/src/emitter.rs

      Lines 271 to 280 in 3b4c0dc

      fn visit_action(&mut self, action: &ast::Action) -> result::Result<Self::Output, Self::Error> {
      let mut emitted = String::new();
      if self.emitter.with_actions_as_comments {
      let indentation = self.emitter.indent().repeat(2);
      emitted.push_str(format!("{}// {}\n", indentation, action.title).as_str());
      }
      Ok(emitted)
      }
  • Add tests for both of the above points.
  • Add it to the docs in https://github.com/alexfertel/bulloak/blob/main/src/lib.rs and https://github.com/alexfertel/bulloak/blob/main/README.md
mds1 commented

Amazing! Testing it out now and have two comments. Given:

MyContract._efficientHash.sol
├── It should never revert.
└── It should match the output of a high-level hash.

Then run bulloak ./**/*.tree and get:

pragma solidity 0.8.0;

contract CreateXTest {
  function test_ItShouldNeverRevert.() external {
    // It should never revert.
  }

  function test_ItShouldMatchTheOutputOfAHigh_levelHash.() external {
    // It should match the output of a high-level hash.
  }
}

So two observations:

  • The trailing period should be stripped to get a valid test name
  • I'd have thought this would result in a single test_Requirements, but it results in two separate tests (expectation shown in the solidity block below)
pragma solidity 0.8.0;

contract MyContractTest {
  function test_Requirements() external {
    // It should never revert.
    // It should match the output of a high-level hash.
  }
}
alexfertel commented

The trailing period should be stripped to get a valid test name

Ah, yes, I need to handle the period as well, good catch! Fixed in https://crates.io/crates/bulloak/0.4.4!

I'd have thought this would result in a single test_Requirements, but it results in two separate tests (expectation shown in the solidity block below)

Yes, after thinking about it for a bit, I decided to go with 1 test per action (#26 (comment)). Even though this is inconsistent with how actions behave when they have parent conditions, I think it is less surprising, since actions without conditions suggest unrelated function invariants. That is, if they relate somehow, then they would be children of a condition. So, it is best to separate the test functions. Wdyt?

For the tree above, the output would be:

contract MyContractTest {
  function test_ShouldNeverRevert() external {
    // It should never revert.
  }

  function test_ShouldMatchTheOutputOfAHigh_levelHash() external {
    // It should match the output of a high-level hash.
  }
}
mds1 commented

Even though this is inconsistent with how actions behave when they have parent conditions, I think it is less surprising, since actions without conditions suggest unrelated function invariants. That is, if they relate somehow, then they would be children of a condition.

Interesting, I'd argue it's actually more surprising, since now the structure of your tests depends on the function being tested: if it's a simple branchless method you have one test per requirement, and if it's more complex you have one test per set of preconditions. So now the test architecture also changes as the function being tested gets more complex, even if you just add a single if condition

actions without conditions suggest unrelated function invariants. That is, if they relate somehow, then they would be children of a condition.

A simple counterexample in the efficientHash test I've been using: The two requirements (must not revert and must match high-level hash) are related by being children of a condition, and the condition is simply "valid inputs" where all inputs of the correct type are valid inputs. Solidity automatically generates the code to validate function inputs, so we always ignore that implicit condition, both in our spec and in tests (i.e. no one writes tests to verify the checks solidity automatically adds, there are many other examples)

In the linked PR:

file.sol
└── it should do stuff
└── when something happens
    └── it should revert
└── it does everything

is an interesting edge case. My reaction is this should not be allowed: The presence of "when something happens" implies the inverse of that should contain the other two conditions

Might be good to pull @PaulRBerg in here for an additional opinion on the

alexfertel commented

since now the structure of your tests depends on the function being tested: if it's a simple branchless method you have one test per requirement, and if it's more complex you have one test per set of preconditions.

I'm not sure I fully understand since I believe the structure of the tests depends on the function being tested in the previous implementation. Let's say we start with:

MyContract._efficientHash.sol
├── it should match the output of a high-level hash
└── it should never revert

Which gets us:

contract MyContractTest {
  function test_ShouldNeverRevert() external {
    // It should never revert.
  }

  function test_ShouldMatchTheOutputOfAHigh_levelHash() external {
    // It should match the output of a high-level hash.
  }
}

And then we decide to update the function a bit to handle some precondition, which we now describe in our .tree like this:

MyContract._efficientHash.sol
├── it should match the output of a high-level hash
├── it should never revert
└── when some precondition occurs
		├── it should do an action
		└── it should do another action

with output:

pragma solidity 0.8.0;

contract MyContractTest {
  function test_ShouldMatchTheOutputOfAHigh_levelHash() external {
    // It should match the output of a high-level hash.
  }

  function test_ShouldNeverRevert() external {
    // It should never revert.
  }

  modifier whenSomePreconditionOccurs() {
    _;
  }

  function test_WhenSomePreconditionOccurs()
    external
    whenSomePreconditionOccurs
  {
    // it should do an action.
    // it should do another action.
  }
}

The hypothesis is that the output above is more surprising than:

pragma solidity 0.8.0;

contract MyContractTest {
  function test_Requirements() external {
    // It should match the output of a high-level hash.
    // It should never revert.
  }

  modifier whenSomePreconditionOccurs() {
    _;
  }

  function test_WhenSomePreconditionOccurs()
    external
    whenSomePreconditionOccurs
  {
    // it should do an action.
    // it should do another action.
  }
}

because the structure of the tests depends on the function being tested.

The only change to the tree above that wouldn't change the structure of the tests is adding an action as a child of the condition, like so:

MyContract._efficientHash.sol
├── it should match the output of a high-level hash
├── it should never revert
└── when some precondition occurs
		├── it should do stuff // This one is new.
		├── it should do an action
		└── it should do another action

But you would say it'd be less surprising if the following change didn't change the structure either:

MyContract._efficientHash.sol
├── it should match the output of a high-level hash
├── it should do stuff // This one is new.
├── it should never revert
└── when some precondition occurs
		├── it should do an action
		└── it should do another action

After laying it out, I'm less confident in either implementation haha, both make sense to me, so I'm happy to implement it the other way. Will wait for Paul to chime in before updating the implementation. One argument I can think of against the current one is that it'd be more work in bulloak check.

My reaction is this should not be allowed: The presence of "when something happens" implies the inverse of that should contain the other two conditions

I thought testing function invariants + possible scenarios would be a totally normal use case, so I didn't want to restrict it.

Let's take the function that calls _efficientHash:

    function _hashPair(bytes32 a, bytes32 b) private pure returns (bytes32) {
        return a < b ? _efficientHash(a, b) : _efficientHash(b, a);
    }

A dummy (but reasonable) tree for this function might be:

MyContract._hashPair.sol
├── It should match the output of a high-level hash.
├── It should hash both args.
├── It should never revert.
└── when first arg is bigger than second arg
		└── it should hash the pair swapped

wdyt?

Sorry for the long post, I just want to get this right.

mds1 commented

Sorry for the long post, I just want to get this right.

No worries at all, this is a valuable discussion and I appreciate it!

Let's take the function that calls _efficientHash:
MyContract._hashPair.sol

├── It should match the output of a high-level hash.
├── It should hash both args.
├── It should never revert.
└── when first arg is bigger than second arg
		└── it should hash the pair swapped

A dummy (but reasonable) tree for this function might be:

This is a great example. It's made me realize that my spec (and consequently this spec) are poorly specified. Given that there are two arguments, It should match the output of a high-level hash is not sufficient enough to know the expected result—should it match the output of keccak256(a,b) or keccak256(b,a)? But, to your point we do have It should never revert as always being true. So the spec should be:

MyContract._hashPair.sol
├── It should never revert.
├── When first arg is smaller than second arg
│       └── It should match the result of `keccak256(abi.encodePacked(a,b))`.
└── When first arg is bigger than second arg
        └── It should match the result of `keccak256(abi.encodePacked(b,a))`.

Given that, we can ask:

  1. What should our solidity test structure be, and
  2. How does the answer to (1) change if new conditions are introduced such that "It should never revert" is conditional?

For (1), I think having all root-level it statements within a test_RootRequirements seems reasonable, as it's a single test for parts of your function that should always be true. Then the remaining it statements follow the usual pattern.

Now let's say we add an arbitrary restriction that zero values are not allowed. This gives a new spec:

MyContract._hashPair.sol
├── When either input is 0
│    └── It should revert.
└── When both inputs are nonzero
     ├── It should never revert.
     └── When first arg is smaller than second arg
     │   └── It should match the result of `keccak256(abi.encodePacked(a,b))`.
     └── When first arg is bigger than second arg
         └── It should match the result of `keccak256(abi.encodePacked(b,a))`.

First off, I'll note this currently generates invalid solidity code—function test_WhenBothInputsAreNonzero contains everything else, e.g. other modifiers and functions.

But now it makes less sense to have a test_RootRequirements since it implies each when or given branch can have it's own root requirements. I am ok with that framing, just not convinced it's idea, and would need to think on how to name things there too. Honestly not sure of the best approaches now 😅 will to think on this more and wait for @PaulRBerg + sablier team's opinion too :)

  • ❤️1
alexfertel commented

For (1), I think having all root-level it statements within a test_RootRequirements seems reasonable, as it's a single test for parts of your function that should always be true

Yeah, I think that is reasonable. My argument was that one would want to test the parts of the function that should always be true separately, but I am not confident in this + it would be more work later in bulloak check.

First off, I'll note this currently generates invalid solidity code—function test_WhenBothInputsAreNonzero contains everything else, e.g. other modifiers and functions.

Argh, I really need to improve the tests.

If it were not broken (I probably introduced a bug recently), the implementation idea was to generate When both inputs are nonzero as a modifier and a test test_WhenBothInputsAreNonzero with the // It should never revert. action. If the condition had more than one child action, it just grouped them under the same test, as if it were a single test_Requirements. If we change the current implementation, then the treatment of the actions would be consistent across the tree.

EDIT: Fixed it (introduced a bug a few versions ago) and the output is:

pragma solidity 0.8.0;

contract MyContractTest {
  function test_ShouldNeverRevert() external {
    // It should never revert.
  }

  modifier whenFirstArgIsSmallerThanSecondArg() {
    _;
  }

  function test_WhenFirstArgIsSmallerThanSecondArg()
    external
    whenFirstArgIsSmallerThanSecondArg
  {
    // It should match the result of `keccak256(abi.encodePacked(a,b))`.
  }

  modifier whenFirstArgIsBiggerThanSecondArg() {
    _;
  }

  function test_WhenFirstArgIsBiggerThanSecondArg()
    external
    whenFirstArgIsBiggerThanSecondArg
  {
    // It should match the result of `keccak256(abi.encodePacked(b,a))`.
  }
}

But now it makes less sense to have a test_RootRequirements since it implies each when or given branch can have its own root requirements.

I think this makes sense! When something happens, we might want to assert something and then just explore the rest of the function paths with other conditions. However, I don't think this occurs in the Sablier codebase. They usually have:

               └── when the segment amounts sum does not overflow
                  ├── when the start time is greater than the first segment milestone
                  │  └── it should revert
                  ├── when the start time is equal to the first segment milestone
                  │  └── it should revert
                  └── when the start time is less than the first segment milestone
                     ├── when the segment milestones are not ordered
                     │  └── it should revert

i.e. they nest conditions, which may inform us. Yet another ping @PaulRBerg 🙈

mds1 commented

Hmm @PaulRBerg @alexfertel what do you think about the current repetitiveness? For example:

function test_WhenFirstArgIsSmallerThanSecondArg()
  external
  whenFirstArgIsSmallerThanSecondArg
{
  // It should match the result of `keccak256(abi.encodePacked(a,b))`.
}

We have whenFirstArgIsSmallerThanSecondArg appear in both the test name and the modifier name, which feels wasteful/clunky. But, assuming we keep the modifiers which I think makes sense, I'm not sure of a good alternative here. Naming everything test_RequirementsX where X is just some incrementing number doesn't feel much better

alexfertel commented

Yes @mds1 , this is being discussed here #7. I am trying to capture exactly what the Sablier team does, but can't quite figure it out. My latest understanding is:

If it is a leaf and it's the only child of a condition node, then we don't generate a modifier and only include it in the name of the test.

However, as you can see in my answer, that is not entirely accurate.

Btw, I'm currently working on bulloak check, hopefully I have some time this weekend to finish it.

  • 🚀1
mds1 commented

Ah thanks, will check out that discussion!

  • 🚀1