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.

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

Jump to bottom

Remove RewardsManager.sol #1041

Merged
merged 11 commits into from
Dec 20, 2023
Merged

Remove RewardsManager.sol #1041

merged 11 commits into from
Dec 20, 2023

Conversation

MikeHathaway
Copy link
Collaborator

@MikeHathaway MikeHathaway commented Dec 17, 2023 •
edited by ith-harvey
Loading

Description

  • Removed RewardsManager.sol and all respective tests
  • Unwound RewardsManager invariants with PositionManager invariants

Purpose

During our Sherlock and Kiril audits there was a bug discovered in RewardsManager.sol. Although we had previously agreed not to deploy the contract prior to the audit, having a implementation example with a bug is a bad idea. We opted to remove RewardsManager.sol all together in the event someone used it.

Impact

Tasks

  • Changes to protocol contracts are covered by unit tests executed by CI.
  • Protocol contract size limits have not been exceeded.
  • Gas consumption for impacted transactions have been compared with the target branch, and nontrivial changes cited in the Impact section above.
  • Scope labels have been assigned as appropriate.
  • Invariant tests have been manually executed as appropriate for the nature of the change.

MikeHathaway
MikeHathaway commented Dec 17, 2023
View reviewed changes
tests/forge/invariants/PositionsAndRewards/handlers/ERC20PoolPositionHandler.sol Outdated
@@ -47,4 +57,87 @@ contract ERC20PoolPositionHandler is PositionPoolHandler, BaseERC20PoolHandler {
_quote = TokenWithNDecimals(_pool.quoteTokenAddress());
_collateral = TokenWithNDecimals(_pool.collateralAddress());
}

function lenderKickAuction(
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Do we still need access to lenderKick and takeOrSettle in position if we're no longer handling rewards?

Copy link
Contributor

Choose a reason for hiding this comment

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

Unsure. We have regression tests which were using these with no staked liquidity. Want to ensure we don't eliminate any PositionManager invariant coverage.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think lenderKick() and takeOrSettle() are necessary. They were originally added to check invariants against rewards manager. See the original PR here -> ac303b8

@ith-harvey ith-harvey marked this pull request as ready for review December 19, 2023 19:16
@ith-harvey ith-harvey changed the title Remove rewards mgr Remove RewardsManager.sol Dec 19, 2023
ith-harvey
ith-harvey approved these changes Dec 19, 2023
View reviewed changes
Copy link
Contributor

@ith-harvey ith-harvey left a comment

Choose a reason for hiding this comment

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

LGTM

EdNoepel
EdNoepel requested changes Dec 19, 2023
View reviewed changes
Copy link
Contributor

@EdNoepel EdNoepel left a comment

Choose a reason for hiding this comment

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

Commit 9fd555c removes ERC20PoolPositionHandler and regression tests, which I spent substantial effort making operational.

grandizzy
grandizzy reviewed Dec 19, 2023
View reviewed changes
Copy link
Contributor

@grandizzy grandizzy left a comment

Choose a reason for hiding this comment

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

I prefer to be consistent in naming, that is rename Positions directories in tests to PositionManager (to match ERC20Pool and ERC721Pool namings)

@ith-harvey
Copy link
Contributor

Commit 9fd555c removes ERC20PoolPositionHandler and regression tests, which I spent substantial effort making operational.

Apologies @EdNoepel , didn't take the time to review "why" you made the changes. Your changes successfully resolved test_regression_index_out_of_bounds() and test_regression_max_less_than_min(). I added them again in this commit -> 3a63048

@ith-harvey
Copy link
Contributor

I prefer to be consistent in naming, that is rename Positions directories in tests to PositionManager (to match ERC20Pool and ERC721Pool namings)

Resolved -> 3a63048

EdNoepel
EdNoepel approved these changes Dec 19, 2023
View reviewed changes
Copy link
Contributor

@EdNoepel EdNoepel left a comment

Choose a reason for hiding this comment

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

LGTM; much thanks.

@grandizzy grandizzy self-requested a review December 20, 2023 04:04
prateek105
prateek105 approved these changes Dec 20, 2023
View reviewed changes
Copy link
Contributor

@prateek105 prateek105 left a comment

Choose a reason for hiding this comment

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

LGTM

@ith-harvey ith-harvey merged commit 6332e3e into develop Dec 20, 2023
3 checks passed
dmitriia
dmitriia reviewed Dec 21, 2023
View reviewed changes
Copy link

@dmitriia dmitriia left a comment

Choose a reason for hiding this comment

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

Ok

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dmitriia dmitriia dmitriia left review comments

@EdNoepel EdNoepel EdNoepel approved these changes

@grandizzy grandizzy grandizzy approved these changes

@ith-harvey ith-harvey ith-harvey approved these changes

@prateek105 prateek105 prateek105 approved these changes

@mattcushman mattcushman Awaiting requested review from mattcushman

Assignees
No one assigned
Labels
Priority: Low Scope: Edits /src Status: Ready for Review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@MikeHathaway @ith-harvey @dmitriia @prateek105 @grandizzy @EdNoepel