feat: Update InvariantTest inheritance, add new functions, fix CI [WIP]
#263
Conversation
mds1
left a comment
mds1
left a comment
There was a problem hiding this comment.
A few more thoughts that came to mind
| address[] private _targetedContracts; | ||
| address[] private _targetedSenders; | ||
|
|
||
| string[] private _excludedArtifacts; | ||
| string[] private _targetedArtifacts; | ||
|
|
||
| FuzzSelector[] internal _targetedArtifactSelectors; | ||
| FuzzSelector[] internal _targetedSelectors; | ||
|
|
There was a problem hiding this comment.
Some of these vars are private and others are internal. Is there a reason for this difference? Wondering which we should prefer
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 targetArtifact(string memory newTargetedArtifact_) internal { | ||
| _targetedArtifacts.push(newTargetedArtifact_); | ||
| } |
There was a problem hiding this comment.
@joshieDo do all of these methods dedupe the arrays in forge or should we check for duplicates when adding to the array?
There was a problem hiding this comment.
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"); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
afaik there's no need for that
| 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; | ||
| } |
There was a problem hiding this comment.
Two UX improvement suggestions, lmk what you think:
- 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 - Renaming
targetContracttoaddTargetContractso it's more clear and descriptive
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 them4 participants
Incorporating feedback from #261