Support deprecation of a single asset from strategies

[tomlinton]

If we could transfer a single asset out of a strategy and remove support for it future migrations of funds will become simpler. Right now to do this we need to liquidate the strategy, redeploy a new one with the assets we want it to support, remove the old one from vault, add the new one, and call allocate again.

Ideally:

• A new function like liquidate that takes a single asset, i.e. liquidate(address _asset)
• A deprecateAsset(address _asset) that calls that liquidate method and removes from assetsMapped (there is complexity here around removing from an array) and updates the assetToPToken mapping.
• This whole setup should not allow a deprecateAsset call when there is only a single asset.

[nickgeoca]

I created a branch for this, depricateSingleAsset. This isn't yet tested, so it is more starting out. Here are the api changes.

What is a good way to test this? Maybe using these test scripts?

With threepool, if you call deprecateAsset, then it calls liquidate(address _asset). But this requires the asset to be the first item in assetsMapped. Meaning calling deprecateAsset with ThreePool only works if it is the first asset in that array. How do you guys want to handle this case?

Let me know what you think, thanks!

[DanielVF]

Hey @nickgeoca.

We've got a super rocking test setup with lots of mocks. Take a look at contracts/tests/. You should be able to test these pretty easily.

On the threepool strategies, each strat only works with one coin at a time, and we store that coins index in contract.

[nickgeoca]

What if you have two coins in threepool?

[nickgeoca]

Ok awesome. I'll check more into that testing folder

[nickgeoca]

This liine of code is producing the error below. It seems to work if it is connect(governor) rather than connect(vault).

Has anyone seen this error?

     Error: invalid signer or provider (argument="signerOrProvider", value="[object Object]", code=INVALID_ARGUMENT, version=contracts/5.0.3)
      at Logger.makeError (node_modules/@ethersproject/logger/src.ts/index.ts:205:28)
      at Logger.throwError (node_modules/@ethersproject/logger/src.ts/index.ts:217:20)
      at Logger.throwArgumentError (node_modules/@ethersproject/logger/src.ts/index.ts:221:21)
      at new Contract (node_modules/@ethersproject/contracts/src.ts/index.ts:606:20)
      at Contract.connect (node_modules/@ethersproject/contracts/src.ts/index.ts:813:26)
      at Context.<anonymous> (test/strategies/aave.js:145:10)
      at processImmediate (internal/timers.js:439:21)

[tomlinton]

Hey @nickgeoca, I think this'd be because vault is not an EOA, but a contract. You need a valid signer for the connect arg. :)

[nickgeoca]

Hey @tomlinton, this makes sense. I am wondering what a good workaround to this is?

Maybe we can add a few functions in the MockVault and run proxy calls from the Vault to the MockVault?

    function deposit(address _strategy, address _token, uint _amount) external {
        InitializableAbstractStrategy(_strategy).deposit(_token, _amount);
    }

    function liquidate(address _strategy, address _token) external {
    ...
    function liquidate(address _strategy) external {
    ...

[tomlinton]

Does it need to be connect(vault)? That call to setRewardLiquidationThreshold should be from the governor. I don't think that call is needed in the context of that test, it should only matter for testing reward token collection and harvesting from the Vault.

[nickgeoca]

It looks like it. Calling await aaveStrategy.connect(vault).deposit(usdc.address, usdcUnits("1000")); ends up calling a solidity function with a onlyVault modifier.

    function deposit(address _asset, uint256 _amount)
        external
        onlyVault

[nickgeoca]

Comparing compound.js and aave.js tests, it looks like even though they are both implementing the identical strategy interface, their tests are different. Do you want to put the liqudation test currently in compound.js into aave.js too?

liquidation tests

  it("Should liquidate a single asset", async () => {
  it("Should deprecate an asset, but not a last remaining asset", async () => {
  it("Should liquidate all assets", async () => {

aave.js

    it("Should be able to mint DAI and it should show up in the aave core", async function () {
    it("Should not send USDC to aave strategy", async function () {
    it("Should be able to mint and redeem DAI", async function () {
    it("Should be able to redeem and return assets after multiple mints", async function () {
    it("Should allow transfer of arbitrary token by Governor", async () => {
    it("Should not allow transfer of arbitrary token by non-Governor", async () => {

compound.js

  it("Should allow a withdraw", async () => {
  it("Should collect rewards", async () => {
  it("Should read reward liquidation threshold", async () => {
  it("Should allow Governor to set reward liquidation threshold", async () => {
  it("Should not allow non-Governor to set reward liquidation threshold", async () => {

[tomlinton]

@nickgeoca Sorry for my slow reply here. Regarding the deposit() call you can use the same method found in test/strategies/compound.js where the vault is set to the governor. We generally have two styles of test for strategies, one where the strategy is just integrated with the Vault and that is tested (e.g. test/vault/z_compound.js) and one where the strategy is tested as a standalone (e.g. test/strategies/compound.js) which is largely for the problem you encountered.

I guess you can tell those tests were written by different people! It'd be nice to have it in both, if its a simple copy paste job, but as long as it is in at least one it is fine. 👍

[nickgeoca]

It looks like the Mock Aave contract is not minting the collateral tokens during a deposit. It's taking the coins, but not creating collateral. I see this in the aave.js test when printing balances, code is below.

I'll take a look into this today.

.. aave test code

      console.log((await dai.balanceOf(aaveStrategy.address)).toString());
      console.log((await usdc.balanceOf(aaveStrategy.address)).toString());

      // Run deposit()
      await aaveStrategy
        .connect(fakeVault)
        .deposit(usdc.address, usdcUnits("1000"));
      await aaveStrategy
        .connect(fakeVault)
        .deposit(dai.address, daiUnits("1000"));
      console.log((await dai.balanceOf(aaveStrategy.address)).toString());
      console.log((await cdai.balanceOf(aaveStrategy.address)).toString());
      console.log((await cusdc.balanceOf(aaveStrategy.address)).toString());

[nickgeoca]

Oh... I think this is causing the errors- the aave.js test needs to use "A" tokens, rather than Compound "C" tokens.

[nickgeoca]

So using A tokens mostly works now. aDAI works fine.

However, I'm having trouble depositing USDC and USDT tokens. I used hardhat console log to print the A token balances. It looks like it should be minting USDC tokens here, when attempting to deposit. _getLendingPool().deposit(_asset, _amount, referralCode);

Does anyone have ideas?

[nickgeoca]

I got the Aave test to pass, including liquidating and depreciating an asset 😁

Compound is good. I'll try n knock out Threepool tomorrow.

[nickgeoca]

Things seem to be working now. It is probably a good time for a review!

[nickgeoca]

@tomlinton , it looks like you renamed liquidate to withdrawAll? I can fork master and throw those tests back in, for withdrawAll. Then we can close this out. How does that sound?