Skip to content
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

Add a Shared Modifiers Contract #1028

Closed
andreivladbrg opened this issue Aug 28, 2024 · 3 comments
Closed

Add a Shared Modifiers Contract #1028

andreivladbrg opened this issue Aug 28, 2024 · 3 comments
Assignees
Labels
effort: epic Multi-stage task that may require multiple PRs. priority: 2 We will do our best to deal with this. type: test Adding, updating, or removing tests. work: complex Probe-sense-respond. The relationship between cause and effect can only be perceived in retrospect.

Comments

@andreivladbrg
Copy link
Member

andreivladbrg commented Aug 28, 2024

After reviewing the PR #1022, I realized how many modifiers are declared multiple times.

For example, the givenNotNull modifier is declared 23 times in different files: https://app.warp.dev/block/6GAXDA0EH90tdp6jocyZ20

IMO this is very redundant, so I suggest adding a common Modifiers contract in the shared dir that can be inherited in Integration_Test.

Also, aside from being redundant, this also makes refactoring and making changes to tests more difficult, resulting in higher maintenance costs.

wdyt @sablier-labs/solidity ?

@andreivladbrg andreivladbrg added work: complex Probe-sense-respond. The relationship between cause and effect can only be perceived in retrospect. priority: 2 We will do our best to deal with this. type: test Adding, updating, or removing tests. effort: medium Default level of effort. labels Aug 28, 2024
@andreivladbrg
Copy link
Member Author

tagged engineers by mistake, it should have been only solidity 😆

@smol-ninja
Copy link
Member

Totally on board with that idea. What do you think about shifting all the modifiers, that are used in at least two files, over to the Modifiers contract?

Like, in the shared folder, we've modifiers scattered across various contracts, and some are even duplicates. For instance, both withdrawMax.t.sol and withdraw.t.sol have the modifier givenEndTimeInFuture. We could just merge them into one, right? And the modifiers that are only used once can be kept in the same test contracts.

@PaulRBerg
Copy link
Member

Agree and makes sense to dump all modifiers in one place.

@andreivladbrg andreivladbrg self-assigned this Aug 28, 2024
@andreivladbrg andreivladbrg added effort: high Large or difficult task. effort: epic Multi-stage task that may require multiple PRs. and removed effort: medium Default level of effort. effort: high Large or difficult task. labels Oct 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
effort: epic Multi-stage task that may require multiple PRs. priority: 2 We will do our best to deal with this. type: test Adding, updating, or removing tests. work: complex Probe-sense-respond. The relationship between cause and effect can only be perceived in retrospect.
Projects
None yet
Development

No branches or pull requests

3 participants