Skip to content

feat: Update InvariantTest inheritance, add new functions, fix CI [WIP] #263

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Jan 3, 2023
Merged

feat: Update InvariantTest inheritance, add new functions, fix CI [WIP] #263

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
bda7d1c
feat: update to add new functions
Jan 3, 2023
5409a82
fix: run forge fmt
Jan 3, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 33 additions & 4 deletions src/InvariantTest.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,9 @@
// SPDX-License-Identifier: MIT
pragma solidity >=0.6.2 <0.9.0;

import {Test} from "./Test.sol";

contract InvariantTest is Test {
pragma experimental ABIEncoderV2;

contract InvariantTest {
struct FuzzSelector {
address addr;
bytes4[] selectors;
Expand All @@ -15,8 +14,21 @@ contract InvariantTest is Test {
address[] private _targetedContracts;
address[] private _targetedSenders;

string[] private _excludedArtifacts;
string[] private _targetedArtifacts;

FuzzSelector[] internal _targetedArtifactSelectors;
FuzzSelector[] internal _targetedSelectors;

Comment on lines 14 to 22
Copy link
Collaborator

@mds1 mds1 Jan 3, 2023

Choose a reason for hiding this comment

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

Some of these vars are private and others are internal. Is there a reason for this difference? Wondering which we should prefer

Copy link
Contributor Author

@lucas-manuel lucas-manuel Jan 4, 2023

Choose a reason for hiding this comment

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

Good call out, I don't really see any advantage to private, what do you think? Who know someone might want access to these values in the inherited contracts.

Copy link
Collaborator

@mds1 mds1 Jan 4, 2023

Choose a reason for hiding this comment

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

Hmm, I think I lean towards keeping it private until someone has a use case for needing access to the array directly, just a bit safer / more clear API that way since you can't accidentally overwrite it / directly set it

function excludeArtifact(string memory newExcludedArtifact_) internal {
_excludedArtifacts.push(newExcludedArtifact_);
}

function excludeArtifacts() public view returns (string[] memory excludedArtifacts_) {
require(_excludedArtifacts.length != uint256(0), "NO_EXCLUDED_ARTIFACTS");
excludedArtifacts_ = _excludedArtifacts;
}

function excludeContract(address newExcludedContract_) internal {
_excludedContracts.push(newExcludedContract_);
}
Expand All @@ -35,6 +47,24 @@ contract InvariantTest is Test {
excludedSenders_ = _excludedSenders;
}

function targetArtifact(string memory newTargetedArtifact_) internal {
_targetedArtifacts.push(newTargetedArtifact_);
}
Comment on lines +50 to +52
Copy link
Collaborator

@mds1 mds1 Jan 3, 2023

Choose a reason for hiding this comment

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

@joshieDo do all of these methods dedupe the arrays in forge or should we check for duplicates when adding to the array?

Copy link
Contributor Author

@lucas-manuel lucas-manuel Jan 4, 2023

Choose a reason for hiding this comment

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

Good question

Copy link
Contributor

@joshieDo joshieDo Jan 5, 2023

Choose a reason for hiding this comment

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

they're deduped yeah.... but to be honest if we're talking about targetArtifactSelector they might be getting replaced instead of appended for the same artifact. i'll have to check it out.


function targetArtifacts() public view returns (string[] memory targetedArtifacts_) {
require(_targetedArtifacts.length != uint256(0), "NO_TARGETED_ARTIFACTS");
Copy link
Collaborator

@mds1 mds1 Jan 3, 2023

Choose a reason for hiding this comment

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

How come the artifact handlers have require statements to ensure non-zero length but others don't? Feels like they should be consistent. Maybe a @joshieDo question

Copy link
Contributor

@joshieDo joshieDo Jan 5, 2023

Choose a reason for hiding this comment

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

afaik there's no need for that

targetedArtifacts_ = _targetedArtifacts;
}

function targetArtifactSelector(FuzzSelector memory newTargetedArtifactSelector_) internal {
_targetedArtifactSelectors.push(newTargetedArtifactSelector_);
}

function targetArtifactSelectors() public view returns (FuzzSelector[] memory targetedArtifactSelectors_) {
require(targetedArtifactSelectors_.length != uint256(0), "NO_TARGETED_ARTIFACT_SELECTORS");
targetedArtifactSelectors_ = _targetedArtifactSelectors;
}
Comment on lines +59 to +66
Copy link
Collaborator

@mds1 mds1 Jan 3, 2023

Choose a reason for hiding this comment

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

Two UX improvement suggestions, lmk what you think:

  1. Re-ordering the contract so the methods user's are intended to call (e.g. targetContract) are at the top, and ones forge calls (e.g. targetContracts, plural), are at the bottom. Maybe with some comments indicating this difference, to make it easier to learn how to use this file
  2. Renaming targetContract to addTargetContract so it's more clear and descriptive

Copy link
Contributor Author

@lucas-manuel lucas-manuel Jan 4, 2023

Choose a reason for hiding this comment

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

Agree with both of these.

I remember asking about updating the naming conventions here a few months ago. I agree that its confusing that targetContract adds a target contract and targetContracts returns all targetedContracts.

I'm good with addTargetContract naming convention. Will make that change if people agree with it.

Copy link
Contributor Author

@lucas-manuel lucas-manuel Jan 4, 2023

Choose a reason for hiding this comment

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

I wanted to do 1) but hesitated because I didn't know which section label convention to use 😆 I'll use whichever you guys like.

Copy link
Collaborator

@mds1 mds1 Jan 4, 2023

Choose a reason for hiding this comment

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

I wanted to do 1) but hesitated because I didn't know which section label convention to use 😆 I'll use whichever you guys like.

Heh, what about something like this:

// Functions for users: These are intended to be called in your tests

// Functions for forge: These are called by forge to run invariant tests. You
// do not need to use these or worry about them


function targetContract(address newTargetedContract_) internal {
_targetedContracts.push(newTargetedContract_);
}
Expand All @@ -61,5 +91,4 @@ contract InvariantTest is Test {
require(_targetedSenders.length != uint256(0), "NO_TARGETED_SENDERS");
targetedSenders_ = _targetedSenders;
}

}