c1.0 - Figure out harvesting timing / sequence details wrt to DR (on DR change && to report profit to vault)

[steve0xp]

Live Notes from Chats with Mellow Team && Val John

• • No difference in when we call registerWithdraw() when it comes to how the period works within MellowStrategy. Thus no need to design harvest() timing around periods really.
• Keep in mind the queued up registeredWithdraws that Yearn already has. That logic matters. Empty out the withdraws before calling registerWithdraw() perhaps? Gotta think about it.

Decide between:

1. Go with typical harvestTriggers() && get possibly incorrect losses or not have them reported due to the async nature of this strategy, OR
2. Prompt harvest() from the SMS (strategist multisig) --> 3/9 multisig of strategists. --> SMS calls harvestTrigger() once && KEEP3Rs harvest() when gas makes sense.

[steve0xp]

PR #6 is where commits related to this issue will be recorded, unless the PR is deemed out of scope for some reason.

[0xValJohn]

For discussion: how does Tokemak strategy handles this? We would want to automate this as much as possible under normal operations (scale up, scale down, maintain DR).

[steve0xp]

Tokemak strategy that you shared for me to check out: https://github.com/charlesndalton/generic-tokemak-strat/blob/master/contracts/Strategy.sol

[steve0xp]

From looking through the Tokemak Strategy.sol and test_accounting_edge_cases.py I've gathered the following:

• Tokemak strategy required 1 week timelock for withdrawals. As stated here

Further details (but can ignore as they are simply my rough notes)

- In `liquidatePosition()`: conditional logic checking if there is enough in the withdraw queue, and if not it requests a `newWithdraw` --> if it doesn't succeed (ex. timelock still in progress so it can't withdraw anything) then it returns `return (_existingLiquidAssets, 0);`, where `_existingLiquidAssets` is just the wantTokens inside the strategy already at that time. **This results in a scenario where there is `profit` but there is no `debtPayment` due to the `timelock`.** - It is called upon by `prepareReturn(uint256 _debtOutstanding)` where `_debtOutstanding` can be `0`. - If it is zero, `prepareReturn()` is being called just to `harvest` profits into strategy I guess. It updates accounting too for profits? **- So basically `debtPayment` is deferred to a `prepareReturn()` that is in the window of when there is actual `wantToken` within the strategy.** - So there could be a situation where someone `harvest(uint256 _debtOutstanding)` and there is `profit` but the strategy can't pay back the `_debtOutstanding` at that moment. - `debtOutstanding` can't be paid back yet, it is deferred... how do we know that when the timelock is done, that the right person will get the `debtOutstanding`? - I guess the right person... is the vault in this case. And we wouldn't have a situation where the vault relies solely on the liquidity of this illiquid strategy, so vault depositors would always be made whole. **- Thus, the accounting of the vault... shows profits and losses properly. It just doesn't have them right there and then, it is postponed by one or more `prepareReturns()`. The idea is that eventually the `debt` will be paid, it is just deferred possibly.**

[steve0xp]

What does this all mean for this strategy?

1. The flow of the Tokemak strategy can be used, assuming that this was all good and my understanding is correct.
2. Manual SMS withdrawRequest(), withdraw(), and other manual functions are probably good to have so the SMS can get the assets out of course.

@0xValJohn please comment when you have a chance

Moving forward to implement the flow of the Tokemak strategy. Just want clarification from Val John on whether my understanding is correct or not.

[steve0xp]

Spoke w/ Val John about this. Here's the summary tweaking my thoughts (full context below in toggle):

Summary

Two Main Points

1. estimatedTotalAssets should only be a function of claimable wantToken - That is the key here that I was missing! AKA, profitFromCurrentAndFutureHarvest should not be included in the estimatedTotalAssets calculation

That way, we have the new profit accounted for when we have the actual profits realized. This is a key tweak from the regular setup of estimatedTotalAssets because most strategies are liquid and not illiquid like this one

---

2. Also, what is the trigger for claiming rewards? For example, we should be attempting to withdraw profits at each harvest, then realize this profit when it is deposited back into the strategy.

My current thoughts on the trigger question:

a. Cycle 1: I believe the amount we queue up to withdraw (which includes the profits from that respective cycle) in one cycle, would eventually transfer from being 'active' in the mellow-gearbox credit account.

b. Cycle 2: At that point, we would have a harvestTrigger() monitoring the amount waiting to be claimed, or we'd have a harvestTrigger() be triggered via a bot from mellow (as per past conversations where they said they could automatically push the ready-to-go amounts to us.

I'd need to triple-check on the fact that the 'ready-to-claim' tokens are no longer active in the credit account and are thus claimable at any point (not some window).

---

Context (can ignore)

Recall: 1. `profit` is updated in `prepareReturn` regardless of what the return values are of `liquidatePosition.` --> is `profit` not just a function of `estimatedTotalAssets` and `debt` for the `strategy` from the `vault`? 2. `estimatedTotalAssets` shouldn't account for rewards that are not available to the strategy 3. So `estimatedTotalAssets` should only be a function of **claimable** `wantToken` - That is the key here that I was missing! aka, `profitFromLastHarvest` should not be included in the `estimatedTotalAssets` calculation that way, we have the `new profit` accounted for when we have the `actual profits` **realized**. _This is a key tweak from the regular setup of `estimatedTotalAssets` because most strategies are liquid and not illiquid like this one_ Val brought up the question: What is the trigger for claiming rewards? For example we should be attempting to withdraw profits at each harvest, then realize this profit when it is deposited back into the strategy. **My current thoughts:** I think that is what the Tokemak strategy is doing. I didn't realize/remember that the profit was realized in `adjustPosition()` or just before that call from the sounds of what you're saying. So this is what I'm thinking: 1. Cycle 1: I believe the amount we queue up to `withdraw` (which includes the profits from that respective cycle) in one cycle, would eventually transfer from being 'active' in the mellow-gearbox credit account. 2. Cycle 2: At that point, we would have a `harvestTrigger()` monitoring the amount waiting to be `claimed,` or we'd have a `harvestTrigger()` be triggered via a `bot` from mellow (as per past conversations where they said they could automatically push the ready-to-go amounts to us. I'd need to triple-check on the fact that the 'ready-to-claim' tokens are no longer active in the credit account and are thus claimable at any point (not some window).

[steve0xp]

Currently working on custom harvestTrigger() and the details of estimatedTotalAssets(). IMO it is going to involve the GearboxRootVault.epochToPriceForLpTokenD18(_currentEpoch); and price for thelatestRequestEpoch[address yearnStrategy].

TODO - clean up this thread as it is confusing right now

[steve0xp]

See PR #6 - where recent commits have been made as an initial attempt at resolving this and issue #15.

The keys, previously stated here, was used to help map out the sequence for harvest() sequences based on DR changing actions from the vault.

Details of the changes are mainly in:

1. valueOfMellowLPT() - [LINK](https://github.com/umphams/yearn_mellow-gearbox-strategy/blob/sp/draft-v1-strategy/src/Strategy.sol#:~:text=function%20valueOfMellowLPT()%20public%20view%20returns%20(uint256)%20%7B)
2. liquidatePosition() - LINK
3. harvestTrigger() - LINK

[steve0xp]

PR #18 incorporates implementation code related to this issue. Leaving this issue open until we have tests and feel that this has been resolved fully alongside issue #15