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

Aave V3 - Opty 1560 #3

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
Open

Aave V3 - Opty 1560 #3

wants to merge 11 commits into from

Conversation

leodinh
Copy link
Collaborator

@leodinh leodinh commented Aug 30, 2022

No description provided.

contracts/AaveV3Adapter.sol Outdated Show resolved Hide resolved
contracts/AaveV3Adapter.sol Outdated Show resolved Hide resolved
contracts/AaveV3Adapter.sol Outdated Show resolved Hide resolved
Comment on lines +147 to +157
function getPoolValue(address _liquidityPoolAddressProviderRegistry, address _underlyingToken)
public
view
override
returns (uint256)
{
return
ERC20(_underlyingToken).balanceOf(
getLiquidityPoolToken(_underlyingToken, _liquidityPoolAddressProviderRegistry)
);
}

Choose a reason for hiding this comment

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

For this function, should we rely on the underlying token balance of the aToken contract (supplied - borrowed) or should we rely on the total underlying tokens supplied?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm not sure. I cannot find availableLiquidity in their V3 code. However, as I check in their V2 code here: availableLiquidity = IERC20Detailed(asset).balanceOf(reserve.aTokenAddress)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

However, your point may be right, In V3 they replace availableLiquidity with totalAToken, which is the total supply of a token

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@dhruvinparikh we need your opinion.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Liquidity is the cash in underlying token available within the pool, For AaveV3, I checked the underlying token is being held in atoken contract.

contracts/AaveV3Adapter.sol Outdated Show resolved Hide resolved
contracts/AaveV3Adapter.sol Outdated Show resolved Hide resolved
/**
* @notice the A tokens list that is used for claiming reward token
*/
address[] public aaveAssetsList;

Choose a reason for hiding this comment

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

When Aave updates this list, would it be more costly to update this variable or to take all the aToken addresses from the getReserveData function in Lending Pool contract by running a for loop?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

_assetsList in Aave is also an array, so if they add more tokens, we need to update our variable also. This variable aaveAssetsList is because Aave rewards functions require _assetsList as a param.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@mariogutval to take all the aToken addresses from the getReserveData function in Lending Pool contract by running a for loop I'm not sure what you mean. In the Adapter, I don't call getReserveData in Lending Pool contract.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I doubt if there even need to for assetList Array as the vault will have atmost hold only single aToken , I can see it is used for claiming rewards and reading unclaimed rewards; we need just pass the single array element with one aToken address to the parameter that accepts aaveAssetList.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

To do that, I will need to find a way to get the aToken based on its underlying token. If not, I will create a mapping.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I found it, will implement it

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

addressed

test/AaveFinanceAdapter.behavior.ts Show resolved Hide resolved
test/AaveV3Adapter.behavior.ts Outdated Show resolved Hide resolved
test/AaveV3Adapter.behavior.ts Show resolved Hide resolved
Copy link
Collaborator

@dhruvinparikh dhruvinparikh left a comment

Choose a reason for hiding this comment

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

You can keep separate behaviour test files being called from single setup test.For e.g.
https://github.com/Opty-Fi/curve-polygon-adapter/blob/main/test/CurveFinanceAdapter.ts

@leodinh
Copy link
Collaborator Author

leodinh commented Sep 1, 2022

You can keep separate behaviour test files being called from single setup test.For e.g. https://github.com/Opty-Fi/curve-polygon-adapter/blob/main/test/CurveFinanceAdapter.ts

@dhruvinparikh Feedback is addressed

const aaveV3AdapterArtifact: Artifact = await hre.artifacts.readArtifact("AaveV3Adapter");
this.aaveV3Adapter = <AaveV3Adapter>(
await hre.waffle.deployContract(this.signers.deployer, aaveV3AdapterArtifact, [this.mockRegistry.address])
);
});
Object.keys(pools).map((token: string) => {
const poolItem: PoolItem = pools[token];
shouldBeHaveLikeAaveAdapter(token, poolItem);
});
Copy link
Collaborator

@dhruvinparikh dhruvinparikh Sep 1, 2022

Choose a reason for hiding this comment

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

  • Enclose it in a separate describe with title "AaveV2 On Polygon"

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

addressed

Comment on lines 46 to 49
Object.keys(poolsV3).map((token: string) => {
const poolItem: PoolItem = poolsV3[token];
shouldBeHaveLikeAaveV3Adapter(token, poolItem);
});
Copy link
Collaborator

@dhruvinparikh dhruvinparikh Sep 1, 2022

Choose a reason for hiding this comment

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

  • Enclose it in a separate describe with title "AaveV3 On Polygon"

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

addressed

@dhruvinparikh
Copy link
Collaborator

For @mariogutval

  • Remove invest-limit feature and hence IAdapterInvestLimit interface

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