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

docs: complete interface & use inheritdoc #763

Merged
merged 29 commits into from
Dec 13, 2022
Merged

docs: complete interface & use inheritdoc #763

merged 29 commits into from
Dec 13, 2022

Conversation

sakulstra
Copy link
Collaborator

@sakulstra sakulstra commented Dec 9, 2022 •
edited
Loading

moving structs & docs to the interface so it's easier to consume

@sakulstra sakulstra changed the title feat: complete interface & use inheritdoc docs: complete interface & use inheritdoc Dec 9, 2022
@sakulstra
Copy link
Collaborator Author

@kartojal / @miguelmtzinf also noticed that
https://github.com/aave/aave-v3-core/blob/master/contracts/interfaces/IReserveInterestRateStrategy.sol#L11
only defines a super small subset of
https://github.com/aave/aave-v3-core/blob/master/contracts/protocol/pool/DefaultReserveInterestRateStrategy.sol#L21

So again ppl relying on this usually completely redefine this interface (at least we do). Might also make sense to apply a similar change there? I'm OOO now, so can't do this.

@sakulstra sakulstra mentioned this pull request Dec 9, 2022
Merged
miguelmtzinf
miguelmtzinf reviewed Dec 12, 2022
View reviewed changes
contracts/misc/AaveProtocolDataProvider.sol Outdated
* @dev Handling MKR and ETH in a different way since they do not have standard `symbol` functions.
* @return The list of reserves, pairs of symbols and addresses
*/
/// @inheritdoc IPoolDataProvider
function getAllReservesTokens() external view returns (TokenData[] memory) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing override on all these functions

@miguelmtzinf
Copy link
Contributor

I was tempted to do these changes on the Interest Rate contracts back in the days but declined that idea because an asset could have a custom interest rate strategy, and the only one function needed for that is already part of the IReserveInterestRateStrategy.
Therefore, I am in favor of leaving the IReserveInterestRateStrategy as is, and create an IDefaultInterestRateStrategy that inherits from the other that contains all the functions of DefaultInterestRateStrategy

miguelmtzinf
miguelmtzinf reviewed Dec 13, 2022
View reviewed changes
contracts/protocol/pool/DefaultReserveInterestRateStrategy.sol
import {IPoolAddressesProvider} from '../../interfaces/IPoolAddressesProvider.sol';
import {Errors} from '../libraries/helpers/Errors.sol';
import {IDefaultInterestRateStrategy} from "../../interfaces/IDefaultInterestRateStrategy.sol";
import {IReserveInterestRateStrategy} from "../../interfaces/IReserveInterestRateStrategy.sol";
Copy link
Contributor

Choose a reason for hiding this comment

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

seems not needed

Copy link
Contributor

Choose a reason for hiding this comment

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

This is needed for the inherit docs

Copy link
Contributor

Choose a reason for hiding this comment

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

which function use it? Don't see it tbh

Copy link
Contributor

Choose a reason for hiding this comment

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

https://github.com/aave/aave-v3-core/pull/763/files#diff-1f7acd1ca2faeee390ca9f1bde1da92aa4397a3307a8f43f2b651f8e01dc5a0cR158

miguelmtzinf
miguelmtzinf reviewed Dec 13, 2022
View reviewed changes
contracts/mocks/tests/MockReserveInterestRateStrategy.sol

contract MockReserveInterestRateStrategy is IReserveInterestRateStrategy {
contract MockReserveInterestRateStrategy is IDefaultInterestRateStrategy {
Copy link
Contributor

Choose a reason for hiding this comment

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

Would be nice to check if a MockReserveInterestRateStrategy that only implements IReserveInterestRateStrategy is enough to satisfy the test suite. if so, I would be in favor of pruning this mock contract.

Copy link
Contributor

Choose a reason for hiding this comment

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

It is enough (tested it) but I think its ok to leave the full mock, as for what i have seen, all interest rate strategies use all the methods, so could be used for someone

miguelmtzinf
miguelmtzinf approved these changes Dec 13, 2022
View reviewed changes
Copy link
Contributor

@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.

LGTM
Thanks @sakulstra and @sendra !

@miguelmtzinf miguelmtzinf changed the base branch from feat/3.0.1 to fix/complete-interfaces December 13, 2022 17:00
@miguelmtzinf
Copy link
Contributor

Merging to an intermediate branch so I can fix the format of some contracts.

@miguelmtzinf miguelmtzinf merged commit dd15e23 into aave:fix/complete-interfaces Dec 13, 2022
@miguelmtzinf miguelmtzinf mentioned this pull request Dec 13, 2022
Merged
miguelmtzinf added a commit that referenced this pull request Dec 14, 2022
@miguelmtzinf @sakulstra
@eboadom @sendra
fix: Complete interfaces of IReserveInterestRateStrategy and IPoolDat…
a00dda8 
…aProvider (#766)

* fix: Complete interfaces of IReserveInterestRateStrategy and IPoolDataProvider (#763)

* feat: complete interface & use inheritdoc

* Added missing functions on IReserveInterestRateStrategy and inheritdoc on DefaultInterestRateStrategy

* fix: added missing overrides, docs for constructor and added ADDRESSES_PROVIDER to interface

* fix: sepparated interest rate strategy interface into default and rate interfaces

* fix: added visibility to false rule

* fix: minor fixes

* fix: fixed natspec. Fixed import order

* Update contracts/interfaces/IDefaultInterestRateStrategy.sol

* Apply suggestions from code review

* Apply suggestions from code review

* fix: fixed correct import order

Co-authored-by: eboado <[email protected]>
Co-authored-by: sendra <[email protected]>
Co-authored-by: sendra <[email protected]>
Co-authored-by: miguelmtz <[email protected]>

* fix: Fix format of multiple contracts

Co-authored-by: Lukas <[email protected]>
Co-authored-by: eboado <[email protected]>
Co-authored-by: sendra <[email protected]>
Co-authored-by: sendra <[email protected]>
@miguelmtzinf miguelmtzinf mentioned this pull request Dec 19, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sendra sendra sendra left review comments

@miguelmtzinf miguelmtzinf miguelmtzinf approved these changes

@The-3D The-3D Awaiting requested review from The-3D

@stevenvaleri stevenvaleri Awaiting requested review from stevenvaleri

@foodaka foodaka Awaiting requested review from foodaka

@kartojal kartojal Awaiting requested review from kartojal

@Zer0dot Zer0dot Awaiting requested review from Zer0dot

@donosonaumczuk donosonaumczuk Awaiting requested review from donosonaumczuk

@BenSparksCode BenSparksCode Awaiting requested review from BenSparksCode

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@sakulstra @miguelmtzinf @sendra @eboadom