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

Test/demeter balancer #17

Merged
merged 67 commits into from
Nov 16, 2023
Merged

Test/demeter balancer #17

merged 67 commits into from
Nov 16, 2023

Conversation

bayou020
Copy link
Collaborator

Added BaseFarm Test cases

@notion-workspace
Copy link

Testing

@bayou020 bayou020 changed the base branch from wip/demeter-e20-farm to dev October 30, 2023 13:41
.gitignore Outdated Show resolved Hide resolved
foundry.toml Show resolved Hide resolved
.gitignore Outdated Show resolved Hide resolved
package.json Show resolved Hide resolved
@parv3213
Copy link
Member

parv3213 commented Nov 14, 2023

@parv3213
Copy link
Member

parv3213 commented Nov 14, 2023

Commendable work @bayou020 👏 .
Here are some points for improvement:

  1. Better test cases for checking the math. Such as on deposit check math for _subscribeRewardFund, _updateFarmRewardData.
  2. Tests for all functions of BaseFarms. Ex, after deposit check token balance before and after, or tests for IERC20(farmToken).safeTransferFrom(msg.sender, address(this), _amount);, or tests for recoverERC20, or checking math for _updateSubscriptionForIncrease.
  3. Tests on demeter-farm (last level). For ex, tests for FARM_ID.
  4. Tests for edge cases. This can be observed by looking at the coverage report.
  5. This is unimportant, but the test function names can be improved by setting a standard. Like how do we want to deal with fuzz, revert, etc.
  6. Right params for vm.expectEmit

contracts/BaseFarm.sol Outdated Show resolved Hide resolved
test/FarmFactory.t.sol Outdated Show resolved Hide resolved
test/FarmFactory.t.sol Outdated Show resolved Hide resolved
test/FarmFactory.t.sol Outdated Show resolved Hide resolved
test/FarmFactory.t.sol Outdated Show resolved Hide resolved
test/FarmFactory.t.sol Outdated Show resolved Hide resolved
Copy link
Collaborator

@arcantheon arcantheon left a comment

Choose a reason for hiding this comment

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

Please resolve the comments

@bayou020 bayou020 requested a review from arcantheon November 14, 2023 12:40
This was referenced Nov 15, 2023
- Fixed RevertsWhen Naming convetion
- Fixed ExpectEmit Failed assertions
- Added Maths tests
Added Maths tests for increase Deposit and withdraw Partially functions
- Added a check on Farm ID
- Added Deposit Maths check
test_deposit_noLockupFarm_revertsWhen_NoLiquidityInPosition fix
parv3213

This comment was marked as off-topic.

contracts/BaseFarm.sol Outdated Show resolved Hide resolved
@YashP16 YashP16 merged commit ae0e9ef into dev Nov 16, 2023
1 check passed
@YashP16 YashP16 deleted the test/demeter-balancer branch November 16, 2023 10:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants