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

feat: integrate convex finance #1

Closed
wants to merge 14 commits into from
Closed

Conversation

niklr
Copy link
Contributor

@niklr niklr commented Aug 26, 2021

Issue

@samitannir @dhruvinparikh

OpenDeFi/Hackathon#5

Name

OptyFi Bounty #1: Convex Finance Adapter

Description

Build an integration with Convex Finance’s liquidity pools using OptyFi's DeFi Adapter interface. With this integration, OptyFi users should be able to deposit CRV, CVX and Curve LP tokens into OptyFi vaults which will in turn deploy them into Convex to earn yield boosted by CRV and CVX rewards. The OptyFi Protocol should also be able to deposit CRV and Curve LP tokens acquired from other strategies involving our direct Curve integration.

The overall goal is to maximize the liquidity, utility and profitability of CRV and other Curve assets in DeFi.

Video walkthrough

https://youtu.be/1iNkQ_7gzNg

Deployment address on Rinkeby

0x72F4165890E4A1d3f7C1f7771c24B6442DcE0a00

@niklr
Copy link
Contributor Author

niklr commented Aug 26, 2021

Would love to get some feedback as early as possible in the development process. Let me know which pools offered by Convex Finance should be prioritized or if you would like to support them all. The integration should be otherwise complete, just missing the pool addresses.

@dhruvinparikh
Copy link
Collaborator

Would love to get some feedback as early as possible in the development process. Let me know which pools offered by Convex Finance should be prioritized or if you would like to support them all. The integration should be otherwise complete, just missing the pool addresses.

I did skimmed the code.

  • Please cover remaining functions in the test suite to make it ~ 100% covered.
  • I found that there are 41 liquidity pools as I am replying. Please fill up the json with the same.
  • Let us run the test suite against all of them to neutralize edge cases if any.

Over all, it looks good. Thank you for contributing.

@niklr
Copy link
Contributor Author

niklr commented Aug 26, 2021

I did skimmed the code.

* Please cover remaining functions in the test suite to make it ~ 100% covered.

* I found that there are 41 liquidity pools as I am replying. Please fill up the json with the same.

* Let us run the test suite against all of them to neutralize edge cases if any.

Over all, it looks good. Thank you for contributing.

@dhruvinparikh Thanks for the fast reply! I just realized that all pools are using the same deposit contract address. This makes it impossible to map from deposit pool to staking vault. Maybe you know a workaround to solve this dilemma?

What about extending the interface, so that an optional pool identifier is passed in combination with the _liquidityPool address?

@dhruvinparikh
Copy link
Collaborator

I did skimmed the code.

* Please cover remaining functions in the test suite to make it ~ 100% covered.

* I found that there are 41 liquidity pools as I am replying. Please fill up the json with the same.

* Let us run the test suite against all of them to neutralize edge cases if any.

Over all, it looks good. Thank you for contributing.

@dhruvinparikh Thanks for the fast reply! I just realized that all pools are using the same deposit contract address. This makes it impossible to map from deposit pool to staking vault. Maybe you know a workaround to solve this dilemma?

What about extending the interface, so that an optional pool identifier is passed in combination with the _liquidityPool address?

Possible work around could be to :

  • hardcode booster contract.
  • Map underlying token to the staking pool instead of mapping booster to staking pool.
  • You might want to keep _liquidityPool and _liquidityPoolToken args with same addresses.(This hack is fine provided you make similar changes in the pool data .json)
  • Also the feeToken() is not the underlyingToken. You can get the underlyingToken by accessing PoolInfo.lpToken(). And the lpToken would be PoolInfo.token() and staking vault would be PoolInfo.crvRewards() aka BaseRewardPool in terms of Convex Finance

@niklr
Copy link
Contributor Author

niklr commented Aug 26, 2021

Sounds good @dhruvinparikh However, not all interface functions provide the underlying token as parameter e.g.

IAdapterHarvestReward.sol

  • getClaimRewardTokenCode

IAdapterStaking.sol

  • getStakeSomeCodes
  • getUnstakeSomeCodes
  • getUnstakeAllCodes
  • getLiquidityPoolTokenBalanceStake

IAdapter.sol

  • getRewardToken
  • canStake

Not exactly sure what you mean with:

You might want to keep _liquidityPool and _liquidityPoolToken args with same addresses.(This hack is fine provided you make similar changes in the pool data .json)

I assume the getUnderlyingTokens(address _liquidityPool, address _liquidityPoolToken) would just return the _liquidityPoolToken which has the value of PoolInfo.lpToken()?

@dhruvinparikh
Copy link
Collaborator

I assume the getUnderlyingTokens(address _liquidityPool, address _liquidityPoolToken) would just return the _liquidityPoolToken which has the value of PoolInfo.lpToken()?

image
My understanding is depicted here in terms of IAdapter

@niklr
Copy link
Contributor Author

niklr commented Sep 8, 2021

As part of the Gitcoin submission requirements, these are the links to the video and the deployed contract:

Video walkthrough: https://youtu.be/1iNkQ_7gzNg

Deployment address on Rinkeby: 0x72F4165890E4A1d3f7C1f7771c24B6442DcE0a00

@dhruvinparikh
Copy link
Collaborator

Closing this PR in favour of #14

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