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

feat: add claim-rewards-to-self functionality to incentives controller #4

Open
wants to merge 24 commits into
base: aave-v2-incentives
Choose a base branch
from

Conversation

SeanJCasey
Copy link

This PR adds a claimRewardsToSelf() function to the incentives controller that reuses the same underlying _claimRewards() function as other claim functions, but auto-populating the to field with msg.sender.

@eboadom
Copy link
Collaborator

eboadom commented Jun 1, 2021

2 comments @SeanJCasey :

@eboadom
Copy link
Collaborator

eboadom commented Jun 1, 2021

I also suggest @SeanJCasey to create mainnet fork tests reproducing a real scenario of claiming rewards from a system that will be using this function. For that you will need to simulate the upgrade of the proxy contract with the new implementation

@SeanJCasey
Copy link
Author

@ernesto-usal I implemented the suggested contract changes and added the suggested mainnet fork test suite.

I also added a script to package.json for running the mainnet fork test suite.

I realized during this process that the current tests in test-fork/ don't work (unless I'm mistaken). I'm guessing that they were just cloned over from the staked aave repo. Anyways, I needed to bump the fork block to get incentives working and needed to change a type that didn't exist in this repo.

Also, I wasn't sure if I needed to preserve the use of DRE.ethers in this file, so I included it anyways.

@SeanJCasey
Copy link
Author

Also, I kept the initialize() function, but just emptied it and removed the no-longer-necessary modifier. I assume this is what you wanted.

@eboadom
Copy link
Collaborator

eboadom commented Jul 1, 2021

@SeanJCasey actually the initializer modifier should remain, because if not, the state (on VersionedInitializable) concerning the versioning will not be modified, and could lead to inconsistencies on consequent updates

Copy link

@miguelmtzinf miguelmtzinf left a comment

Choose a reason for hiding this comment

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

I would change the name of the audit file to include the date.

@@ -19,7 +19,9 @@
"coverage": "hardhat coverage",
"test": "npm run test-incentives",
"test-incentives": "TS_NODE_TRANSPILE_ONLY=1 hardhat test ./test/__setup.spec.ts ./test/AaveIncentivesController/*.spec.ts",

Choose a reason for hiding this comment

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

Lets add FORKING_BLOCK=12290275 for the previous tests to make them work.

@miguelmtzinf miguelmtzinf changed the base branch from master to aave-v2-incentives April 27, 2022 09:30
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.

3 participants