-
Notifications
You must be signed in to change notification settings - Fork 599
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
feat: Add invariant testing #760
feat: Add invariant testing #760
Conversation
.vscode/settings.json
Outdated
@@ -0,0 +1,3 @@ | |||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Love it so far, just took a pass and left some small comments. Think I'll need to see it a bit more fleshed out before coming up with some naming ideas
I also read through the first draft of this, and I like it a lot - it's straight to the point. My documentation requests:
|
@lucas-manuel, quick terminology question. In August 2022, you referred in this comment to the target contracts that encapsulate the calls to the real contracts that are to be invariant tested as "actors". However, in this PR, it looks like you have changed the name to "handler" - am I correct that you now prefer this terminology over "actor"? |
Thats right - we're moving to handlers since the term "actor" was used when we had to use one contract per user, before pranking was possible. Now that multiple actors can be used within a give contract, we determined that handler should be used instead. |
Thanks for confirming, @lucas-manuel. Speaking of pranking multiple actors - the |
Potentially dumb question - can I see no reference to this cheatcode in Lucas' examples, but I'm not sure if this is because Maple uses a different constraining system ( If I understand this correctly, under "Bounded Testing", the user is expected to bound the inputs to avoid reverts. |
Gonna work to push this through now, what is missing? @mds1 @gakonst @PaulRBerg @pcaversaccio |
Correct me if I'm wrong @mds1 but I believe |
This is an interesting idea, thoughts @mds1 ? |
Proper descriptions on all the invariant helper functions like |
It doesn't. I'm getting
Yes! A markdown table for this would be awesome.
The greatest conceptual leap for me were the handler contracts - I spent a few days looking at the I see that you added more explanations and a few diagrams about them in the meantime (6 hrs ago), which are awesome. But I think that there are still a few other core concepts that would be good to document, too:
|
@PaulRBerg Nice catch, just created foundry-rs/foundry#4190 to track
This is an interesting idea. More generally maybe we can add a template
Awesome, thanks! Later today I'll review what's currently here and reply with some thoughts |
I like this idea. Definitely one or two helpers for actor generation and management would be nice. I may open an issue for this later. |
@lucas-manuel I think one final pass based on the above comments and we're good to merge as a first iteration |
Haven't checked again the content of this PR but apparently you thought it would be a good idea to rename |
@pcaversaccio you should set the Nonetheless, I agree with you that all references to |
@PaulRBerg yeah agreed - however in my particular case here I didn't want to use the |
I like this idea but yeah I think we should push for the native support, thoughts @mds1 @gakonst ? |
The screenshot is correct when using v1.3.0 of forge-std, when v1.4.0 is released the release notes will document the breaking change and new usage info. I agree with updating the docs to reflect this upcoming usage / naming though, cc @lucas-manuel
Agreed native support is preferable. There may be higher priority fixes/features though so I do think a template handler contract / modifiers in forge-std would be useful in the meantime. I also like this pattern that I recently came across from @horsefacts, and people can then implement their own getters to parse the calldata |
Should I add these to the docs? |
I've talked about Native support here: foundry-rs/foundry#3607 (comment). We could but I think is for separate discussion. horsefact's pattern is still not too common so I'd avoid including it anything else we need to get this over the line? |
Just adding the target definitions and will push for final review. |
Alright good for review 👍 |
Agree, this is more an exploration than something I'd recommend right now. |
src/forge/invariant-testing.md
Outdated
There are different ways to assert invariants: | ||
Examples of invariants are: | ||
- *"The xy=k formula always holds"* for Uniswap | ||
- *"The sum of all owner balances is equal to the total supply"* for an ERC-20 token. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit, since "owner" sounds like a privileged actor
- *"The sum of all owner balances is equal to the total supply"* for an ERC-20 token. | |
- *"The sum of all user balances is equal to the total supply"* for an ERC-20 token. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated
One comment here we should fix, but otherwise this looks really great! Awesome job @lucas-manuel 🔥 |
Updated! 👍 |
@gakonst @lucas-manuel anything blocking merge here? think we might just need info around how |
Good to go on my end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just had a final read, looks lit. Thanks again @lucas-manuel for writing this up. There are only a few minor typos - besides these, LGTM.
I'm glad to see the useActor
modifier included in this PR 🙌
src/forge/invariant-testing.md
Outdated
| `excludeSender(address newExcludedSender_)` | Adds a given address to the to the `_excludedSenders` array. This set of addresses is explictly excluded from the target senders. | | ||
| `excludeArtifact(string memory newExcludedArtifact_)` | Adds a given string to the to the `_excludedArtifacts` array. This set of strings is explictly excluded from the target artifacts. | | ||
| `targetArtifact(string memory newTargetedArtifact_)` | Adds a given string to the to the `_targetedArtifacts` array. This set of strings is used for the target artifacts. | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Double to the to the
typo.
src/forge/invariant-testing.md
Outdated
### Target Contract Setup | ||
|
||
Target contracts can be set up using the following three methods: | ||
1. Contracts that are manually added to the the `targetContracts` array are added to the set of target contracts. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Double the the
typo.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
src/forge/invariant-testing.md
Outdated
|
||
Invariant testing is a powerful tool to expose incorrect logic in protocols. Due to the fact that function call sequences are randomized and have fuzzed inputs, invariant testing can expose false assumptions and incorrect logic in edge cases and highly complex protocol states. | ||
|
||
Invariant testing campaigns have two dimensions, `runs` and `depth`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Invariant testing campaigns have two dimensions, `runs` and `depth`. | |
Invariant testing campaigns have two dimensions, `runs` and `depth`: |
src/forge/invariant-testing.md
Outdated
| Function | Description | | ||
|-|-| | ||
| `excludeContract(address newExcludedContract_)` | Adds a given address to the `_excludedContracts` array. This set of contracts is explicitly excluded from the target contracts.| | ||
| `excludeSender(address newExcludedSender_)` | Adds a given address to the to the `_excludedSenders` array. This set of addresses is explictly excluded from the target senders. | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| `excludeSender(address newExcludedSender_)` | Adds a given address to the to the `_excludedSenders` array. This set of addresses is explictly excluded from the target senders. | | |
| `excludeSender(address newExcludedSender_)` | Adds a given address to the to the `_excludedSenders` array. This set of addresses is explicitly excluded from the target senders. | |
src/forge/invariant-testing.md
Outdated
|-|-| | ||
| `excludeContract(address newExcludedContract_)` | Adds a given address to the `_excludedContracts` array. This set of contracts is explicitly excluded from the target contracts.| | ||
| `excludeSender(address newExcludedSender_)` | Adds a given address to the to the `_excludedSenders` array. This set of addresses is explictly excluded from the target senders. | | ||
| `excludeArtifact(string memory newExcludedArtifact_)` | Adds a given string to the to the `_excludedArtifacts` array. This set of strings is explictly excluded from the target artifacts. | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| `excludeArtifact(string memory newExcludedArtifact_)` | Adds a given string to the to the `_excludedArtifacts` array. This set of strings is explictly excluded from the target artifacts. | | |
| `excludeArtifact(string memory newExcludedArtifact_)` | Adds a given string to the to the `_excludedArtifacts` array. This set of strings is explicitly excluded from the target artifacts. | |
Made all requested changes thanks @PaulRBerg @pcaversaccio |
let's send it |
closes #497