Feature/modular testing natspec #150

funkyenough · 2025-05-27T13:38:16Z

Fix natspec and remove `setUp` in `StakerTestBase`

Add contract level natspec
Add test level natspec
Remove unused slither comments
Remove setUp in StakerTestBase and inheriting contracts

alexkeating

Great start, left a few comments!

alexkeating · 2025-05-29T17:21:55Z

src/test/MintRewardNotifierTestBase.sol

-/// Simulates calling notify function on MintRewardNotifier.
+/// @title MintRewardNotifierTestBase
+/// @author [ScopeLift](https://scopelift.co)
+/// @notice Base contract for testing `MintRewardNotifier` functionality. Extends `StakerTestBase`


Suggested change

/// @notice Base contract for testing `MintRewardNotifier` functionality. Extends `StakerTestBase`

/// @notice Base contract for testing a staker contract with a single mint reward notifier. Extends `StakerTestBase`

Fixed in a0cac50
Modified other test base comments as well.

alexkeating · 2025-05-29T17:22:54Z

src/test/MintRewardNotifierTestBase.sol

+/// @title MintRewardNotifierTestBase
+/// @author [ScopeLift](https://scopelift.co)
+/// @notice Base contract for testing `MintRewardNotifier` functionality. Extends `StakerTestBase`
+/// and provides the necessary setup for the `notify` on `MintRewardNotifier`.


Suggested change

/// and provides the necessary setup for the `notify` on `MintRewardNotifier`.

/// and implements the necessary notifying a reward.

I am not sure about the English phrasing here, so I went with implements the reward notification logic in a0cac50. What do you think?

alexkeating · 2025-05-29T17:24:09Z

src/test/MintRewardNotifierTestBase.sol

+/// @notice Base contract for testing `MintRewardNotifier` functionality. Extends `StakerTestBase`
+/// and provides the necessary setup for the `notify` on `MintRewardNotifier`.
+/// @dev This contract is designed to be used in conjunction with the deployment scripts in
+/// `src/script/notifiers/DeployMintRewardNotifier.sol`.


They don't need to use our deployment scripts. They can have their own, but they need to make sure the test contract sets a mintRewardNotifier.

Fixed in a0cac50

alexkeating · 2025-05-29T17:25:41Z

src/test/StakerTestBase.sol

+/// @notice This abstract contract provides a common foundation and essential helper functions for
+/// constructing modular integration tests. It is designed to be inherited by test suites that
+/// verify the functionality of Staker contracts, various reward notifiers and earning power
+/// calculators.


I would add an example here of what this inheritance will look like.

Added example here a0cac50, open to feedback!

alexkeating · 2025-05-29T17:27:51Z

src/test/StandardTestSuite.sol

+/// This includes tests for single and multiple depositors staking and earning rewards
+/// over various timeframes and reward periods.
+/// @dev Inherit this contract to test the fundamental staking and reward accrual logic of a Staker
+/// implementation.


I would be more specific here which staking logic. It doesn't need to be exhaustive but it may be good to indicate in scenarios of single, multiple stakers, multiple periods etc...

I would also think about this for the other test examples as well

Added more descriptions for all bases in a0cac50

alexkeating · 2025-05-29T17:29:28Z

src/test/StandardTestSuite.sol

+  /// calculated earned rewards.
+  /// @param _depositor The address of the staker.
+  /// @param _amount The amount of stake tokens to deposit.
+  /// @param _delegatee The address to delegate voting power to.


The

Suggested change

/// @param _delegatee The address to delegate voting power to.

/// @param _delegatee The address with the stake's voting power.

Fixed all instances of _delegatee in a0cac50

alexkeating · 2025-06-02T16:11:09Z

src/test/TransferRewardNotifierTestBase.sol

-/// used in conjunction with the deployment scripts in
+/// @notice Base contract for testing `TransferRewardNotifier` functionality. Extends
+/// `StakerTestBase`
+/// and provides the necessary setup for `notify` on `TransferRewardNotifier`.


Weird wrapping here

Fixed in a0cac50

alexkeating · 2025-06-02T16:11:41Z

src/test/TransferRewardNotifierTestBase.sol

+/// `StakerTestBase`
+/// and provides the necessary setup for `notify` on `TransferRewardNotifier`.
+/// @dev This contract is designed to be used in conjunction with the deployment scripts in
 /// `src/script/notifiers/DeployTransferRewardNotifier.sol`.


I would add more detail here

…ewardBase, AlterClaimerBase, AlterDelegateeBase to remaining TestBase tests

…TestBase and DeployStakedBinaryEligibilityOracleEarningPowerCalculatorTestBase

funkyenough · 2025-06-04T07:44:13Z

Given that StakedBinaryEligibilityOracleEarningPowerCalculatorTestBase is expected to be removed in subsequent PR, I decided not to update its natspec.

github-actions · 2025-06-04T07:44:50Z

Coverage after merging feature/modular-testing-natspec into feature/modular-testing will be

100.00%

Coverage Report

File	Stmts	Branches	Funcs	Lines
src
DelegationSurrogate.sol	100%	100%	100%	100%
DelegationSurrogateVotes.sol	100%	100%	100%	100%
Staker.sol	100%	100%	100%	100%
src/calculators
BinaryEligibilityOracleEarningPowerCalculator.sol	100%	100%	100%	100%
IdentityEarningPowerCalculator.sol	100%	100%	100%	100%
src/extensions
StakerCapDeposits.sol	100%	100%	100%	100%
StakerDelegateSurrogateVotes.sol	100%	100%	100%	100%
StakerOnBehalf.sol	100%	100%	100%	100%
StakerPermitAndStake.sol	100%	100%	100%	100%
src/notifiers
MintRewardNotifier.sol	100%	100%	100%	100%
RewardTokenNotifierBase.sol	100%	100%	100%	100%
TransferFromRewardNotifier.sol	100%	100%	100%	100%
TransferRewardNotifier.sol	100%	100%	100%	100%

--------- Co-authored-by: Alexander Keating <keating.dev@protonmail.com> Add modular testing natspec (#150) * Add modular testing natspec

funkyenough requested a review from alexkeating May 28, 2025 15:00

funkyenough marked this pull request as ready for review May 29, 2025 14:13

alexkeating reviewed Jun 2, 2025

View reviewed changes

funkyenough added 5 commits June 4, 2025 16:12

fix natspec

0fcb99f

remove unnecessary slither comments

5d4b22f

remove setUp from StakerTestBase and inheriting contracts, add ClaimR…

224a74c

…ewardBase, AlterClaimerBase, AlterDelegateeBase to remaining TestBase tests

add natspec for tests

c2ac103

fix natspec

7d33b4b

funkyenough force-pushed the feature/modular-testing-natspec branch from a0cac50 to 7d33b4b Compare June 4, 2025 07:14

funkyenough added 3 commits June 4, 2025 16:27

remove setUp from DeployBinaryEligibilityOracleEarningPowerCalculator…

c763d5a

…TestBase and DeployStakedBinaryEligibilityOracleEarningPowerCalculatorTestBase

Fix: natspec for BinaryEligibilityOracleEarningPowerCalculatorTestBase

1c334db

Fix: modify test file name to .t.sol

88b7fea

funkyenough requested a review from alexkeating June 4, 2025 14:17

alexkeating merged commit b33295f into feature/modular-testing Jun 4, 2025
6 checks passed

alexkeating added a commit that referenced this pull request Jun 4, 2025

Add modular testing utilites

e02db2e

--------- Co-authored-by: Alexander Keating <keating.dev@protonmail.com> Add modular testing natspec (#150) * Add modular testing natspec

alexkeating added a commit that referenced this pull request Jun 4, 2025

Add modular testing utilites

b2138c7

--------- Co-authored-by: Alexander Keating <keating.dev@protonmail.com> Add modular testing natspec (#150) * Add modular testing natspec

	/// @notice Base contract for testing `MintRewardNotifier` functionality. Extends `StakerTestBase`
	/// @notice Base contract for testing a staker contract with a single mint reward notifier. Extends `StakerTestBase`

	/// and provides the necessary setup for the `notify` on `MintRewardNotifier`.
	/// and implements the necessary notifying a reward.

	/// @param _delegatee The address to delegate voting power to.
	/// @param _delegatee The address with the stake's voting power.

Feature/modular testing natspec #150

Feature/modular testing natspec #150

Uh oh!

Conversation

funkyenough commented May 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Fix natspec and remove setUp in StakerTestBase

Uh oh!

alexkeating left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

funkyenough commented Jun 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Jun 4, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

funkyenough commented May 27, 2025 •

edited

Loading

Fix natspec and remove `setUp` in `StakerTestBase`

funkyenough commented Jun 4, 2025 •

edited

Loading