fix: Make Rewards contract work with PoolAddressesProvider updateImpl #123
Conversation
…omponents--aave-v3-periphery chore(master): release 3.0.1
| @@ -41,12 +41,16 @@ contract RewardsController is RewardsDistributor, VersionedInitializable, IRewar | |||
| _; | |||
| } | |||
|
|
|||
| address public ADDRESSES_PROVIDER; | |||
There was a problem hiding this comment.
address public ADDRESSES_PROVIDER; violates code-style standard though i assume?
as it's neither immutable, nor constant should rather be addressesProvider
There was a problem hiding this comment.
I removed this variable since its not needed at all.
| function initialize(address emissionManager) external initializer { | ||
| _setEmissionManager(emissionManager); | ||
| function initialize(address provider) external initializer { | ||
| ADDRESSES_PROVIDER = provider; |
There was a problem hiding this comment.
i'm not sure if storing ADDRESSES_PROVIDER is good or not as it's not needed for anything and in practice RewardsController is not actually bound to a single ADDRESSES_PROVIDER i think?
So why create an artificial bond here that's just confusing.
I guess a more reasonable alternative would be:
function initialize(address) external initializer { }
There was a problem hiding this comment.
This is a good point.
If the Rewards contracts can be used by multiple Aave Pools at the same time, then: i) having the same emissionManager shouldn't be problematic ii) linking the rewards contract to a specific Pool's PoolAddressesProvider contract does not make so much sense to me (would be better if we use a separate ProxyAdmin contract).
An alternative is keeping the same implementation of the Rewards contract and deploy a proxy per Aave Pool. This approach will give us more flexibility in terms of emissionManager roles, but at the cost of having 2 more contracts per pool (rewards and emissionManager)
There was a problem hiding this comment.
If emissionManager & rewardsController are always 1to1, why have 2 contracts in first place?
There was a problem hiding this comment.
Yes. The EmissionManager contract allows to allowlist an entity to govern the rewards for a particular asset (aToken/debtToken).
Its not an issue of having the same emissionManager for multiple pools at the same time because entities are allowlisted per asset (aToken/debtToken) and the address of these assets are different from one pool to another.
fix: update test suite to match latest @aave/deploy-v3 version
|
The deployment of the rewards contracts have changed a bit due to the cross-reference between the EmissionManager and the RewardsController (the EmissionManager is linked to the RewardsController contract on deployment and a reference of the RewardsController proxy is needed in the EmissionManager contract). |
Codecov ReportBase: 56.14% // Head: 55.73% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## master #123 +/- ##
==========================================
- Coverage 56.14% 55.73% -0.41%
==========================================
Files 25 25
Lines 976 967 -9
Branches 123 123
==========================================
- Hits 548 539 -9
Misses 428 428
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
miguelmtzinf
requested review from
kartojal and
sakulstra
and removed request for
sakulstra
…aave#123) * chore(master): release 3.0.1 * fix: Make Rewards contract work with PoolAddressesProvider updateImpl logic * fix: Remove unneeded event * fix: Remove unneeded addressProvider variable * fix: Update docs contracts/rewards/RewardsController.sol * fix: Add creation of emissionManager in Rewards deployment * chore: Bump version of package to 1.21.0-beta.0 * fix: update test suite to match latest @aave/deploy-v3 version * fix: Revert creation of EmissionManager within Rewards contract * fix: Bump version of deploy package with latest scripts * ci: Fix package-lock for ci * chore: fix CHANGELOG * fix: use EmissionManager from deploy * chore: Bump version of deploy package Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: David K <canillas.mail@gmail.com>
The current initialization function of the
RewardsControllercontract is confusing given that its proxy is owned by thePoolAddressesProvider.The
PoolAddressesProvideruses theupdateImplfunction to upgrade the implementation and passes the address of itself (code here).This PR introduces the following changes:
RewardsControllernow expects the PoolAddresesProvider's address as parameter.emissionManageris not initialized in the constructor as an immutable. This is not an issue because: i) it always goes with theEmissionManagercontract hand in hand and ii) if something goes wrong we can always upgrade the contract. Side effect: we should keep the_emissionManagerstorage slot for not messing the storage layout up.