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

Write pseudo code for mellow-gearbox_wETH strategy to start discussions #6

Closed
wants to merge 11 commits into from

Conversation

steve0xp
Copy link
Owner

@steve0xp steve0xp commented Jan 29, 2023

Primary Goals of PR

Write pseudo code to define scope of implementation details:

TODO:

  • Research Mellow contracts && protocol architecture
    • Ask questions as needed (see Figjam)
  • Write pseudo code (comments && rough code) for Val John to peer review
  • Discuss pseudo code && open issues async w/ Val John && other stakeholder
  • Troubleshoot scope clarification && issues && compiler errors
  • Discuss and set up new PR scope (for tests and more)

NOTES:

  • Val John, please see the TODOs commented throughout the Strategy.sol. Also, note that I kept a couple of aspects from the Angle protocol strategy you had sent me as a reference with the anticipation to change it based on peer review discussions.
  • Questions answered by Mellow team have defined some aspects & raised some design questions to keep in mind. These are outlined in current issues (some postponed until later) 1 through 5. The most pertinent ones are:
  1. Issue Set Up 1-Tx Withdrawals #1
  2. Issue Holistic strategy design wrt to harvestTrigger() conditions & functionality #4
  3. Issue Where does the strategy measure health metrics from: Yearn (internal calcs), Mellow, or Gearbox? #5
  • Discussions were had using a figjam early on as well: WIP Figma This may be expanded on to show the architecture of strategy too.

src/Strategy.sol Outdated Show resolved Hide resolved
src/Strategy.sol Outdated Show resolved Hide resolved
Copy link
Collaborator

@0xValJohn 0xValJohn left a comment

Choose a reason for hiding this comment

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

Initial set of comments.

src/Strategy.sol Outdated Show resolved Hide resolved
src/Strategy.sol Outdated Show resolved Hide resolved
src/Strategy.sol Outdated Show resolved Hide resolved
src/Strategy.sol Outdated Show resolved Hide resolved
src/Strategy.sol Outdated Show resolved Hide resolved
src/Strategy.sol Outdated Show resolved Hide resolved
src/Strategy.sol Outdated
function _withdrawSome(uint256 _amount) internal {
uint256 _lptToBurn = Math.min(wantToMelloToken(_amount), balanceOfMellowToken()); // see dev comment above

gearboxRootVault.registerWithdrawal(_lptToBurn); // queues up withdrawals for current epoch. Also closes out any hanging withdrawals from before, so may have more wantToken in the strategy then we wanted from this.
Copy link
Collaborator

Choose a reason for hiding this comment

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

We probably want to have a setter function here to manually request a withdrawal.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Are you suggesting a function that simply calls registerWithdrawal() whenever we want to manually do so? This would be in addition to this internal function _withdrawSome()?

Copy link
Owner Author

Choose a reason for hiding this comment

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

The only reason I can think of for two functions atm, is permissions. withdrawSome() seems to be an internal function usually, and manualWithdraw() sounds like a permissioned, onlyStrategist function call

Copy link
Collaborator

Choose a reason for hiding this comment

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

Correct, we should have both the internal function and a permissionned function that can be called by sms.

Copy link
Owner Author

Choose a reason for hiding this comment

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

I actually think I need to coordinate this with the harvest() sequences that we decide are going to be used with this strategy. Will update this as we sort this out in issue #7

returns (address[] memory)
// from angle strategy: wantToken is transferred by the base contract's migrate function

// TODO - possibly transfer CRV
Copy link
Collaborator

Choose a reason for hiding this comment

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

I was under the impression that all rewards were handled by Mellow, and we would only get the yield in term of want token. Please clarify if there are cases where we could end up with CRV/CVX tokens.

Copy link
Owner Author

Choose a reason for hiding this comment

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

We have received clarification that all rewards are handled by Mellow. There may be moments where the rewards are still in their original form, CRV/CVX tokens, but that would only happen in circumstances where the Mellow bot hasn't swapped && reinvested the rewards tokens due to the adjustPosition associated to the marginFactor not being triggered.

TODO - I need to confirm this moment though with Mellow. If this is what happens, the estimatedTotalAssets() function and any other querying functions will need to keep this circumstance in mind.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Would CRV / CVX tokens be transferred to the strategy? If that's a possibility, we need the yswaps logic, else it can be removed completely.

Copy link
Owner Author

Choose a reason for hiding this comment

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

From a conversation with Mellow Protocol Team:

The CRV/CVX rewards are never given to the user in form of CRV/CVX, they exist in CRV/CVX only as a part of the internal workflow of the strategy. So, I think you could see it as a strategy fully operating with users in WETH only.

I think I am still trying to understand how one could carry out fairly accurate accounting at any point with this strategy. It may be down to just checking all possible locations for rewards tokens too & converting it to wantTokens. @0xValJohn what do you think? Do we need to go that far?

Copy link
Owner Author

Choose a reason for hiding this comment

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

This thread basically hinges on my assumptions being right or wrong:

A common yearn workflow, IIRC, is to check accounting for strategies per vault. That said, there may come times when the strategy calls for an accounting report from the strategy, and thus we will have to calculate estimatedTotalAssets within the strategy. From the sounds of it, the Mellow credit account may have reward tokens at certain times until the Mellow bot rebalances (via swapping rewards for further wantTokens). So we would simply have to ensure we check contracts for any value when calculating ´estimatedTotallAssets´. Does this sound like overkill or fair?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it's better to simplify things here and only account for rewards that are "claimable" and converted back to want.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Sounds good to me!

src/Strategy.sol Outdated Show resolved Hide resolved
src/Strategy.sol Outdated Show resolved Hide resolved
@steve0xp
Copy link
Owner Author

steve0xp commented Feb 5, 2023

Made some changes as per requests within this draft PR. The remaining tasks are sorted into a few issues (#7, #8, #15 specifically). I will work on them in the next commit on this PR.

I left change requests/conversations unresolved for the most part so you can confirm the changes. Thanks!

@steve0xp
Copy link
Owner Author

steve0xp commented Feb 8, 2023

Recent commits have been made as an initial attempt at resolving issue #7 and issue #15.

The keys, previously stated #7, 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:

valueOfMellowLPT() - LINK
liquidatePosition() - LINK
harvestTrigger() - LINK

There are TODOs still within the Strategy.sol and integration / other tests are not working right now. @0xValJohn please let me know if you think these changes make sense. If they do, I believe we are in the phase of getting the standard tests working and investigating tests custom to this strategy.

@steve0xp steve0xp mentioned this pull request Feb 11, 2023
@steve0xp
Copy link
Owner Author

Closing this PR as it is continued in PR #18. Val John had started this new PR since there were some issues with this one (minor), and it seemed easier to deal with in a new PR. Please take a look at this new PR for other discussion topics and use this one for the early development context.

@steve0xp steve0xp closed this Feb 13, 2023
@steve0xp steve0xp deleted the sp/draft-v1-strategy branch February 13, 2023 15:53
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.

2 participants