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.

Sign up for GitHub

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

Jump to bottom

Add transfer vault capability for crab v2 #518

Closed
wants to merge 10 commits into from
Closed

Add transfer vault capability for crab v2 #518

wants to merge 10 commits into from

Conversation

KMKoushik
Copy link
Contributor

@KMKoushik KMKoushik commented Jun 14, 2022 •
edited

Task:

Add vault transferring capability to crabV2

I am getting Contract code size exceeds 24576 bytes so I disabled it for now and we have to fix it later

Description

  • Add transferVault event
  • Set Cap to 0
  • Add tests for transfer vault
  • Add time lock contract
  • Add tests for timelock

@vercel
Copy link

vercel bot commented Jun 14, 2022 •
edited

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
continuouscall ✅ Ready (Inspect) Visit Preview Jun 17, 2022 at 9:13AM (UTC)

@KMKoushik KMKoushik changed the title Add transfer vault capability Add transfer vault capability for crab v2 Jun 14, 2022
aparnakr
aparnakr approved these changes Jun 14, 2022
View reviewed changes
Copy link
Contributor

@aparnakr aparnakr left a comment

Choose a reason for hiding this comment

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

Can you also add a simple test for this?

@aparnakr
Copy link
Contributor

Do we want to add a timelock as well? If so you could look at https://github.com/compound-finance/compound-protocol/blob/master/contracts/Timelock.sol

@KMKoushik
Copy link
Contributor Author

Do we want to add a timelock as well? If so you could look at https://github.com/compound-finance/compound-protocol/blob/master/contracts/Timelock.sol

@aparnakr How bout making a separate PR for this?

@KMKoushik KMKoushik marked this pull request as ready for review June 15, 2022 11:13
aleone
aleone approved these changes Jun 15, 2022
View reviewed changes
Copy link
Contributor

@aleone aleone left a comment •
edited

Choose a reason for hiding this comment

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

Generally lgtm, few small comments. Ok doing the timelock in separate PR, but do think it may change things a decent amount / larger scope. Fine with either way of doing it.

TLDR:

  • Think easier to use optimizer to fix contract bytecode size for now, reverted later
  • add missing natspec
  • could consider public vs external/internal pattern especially if hit bytecode limit later

packages/hardhat/contracts/strategy/CrabStrategyV2.sol Show resolved Hide resolved
packages/hardhat/hardhat.config.ts Outdated Show resolved Hide resolved
packages/hardhat/contracts/strategy/CrabStrategyV2.sol Show resolved Hide resolved
@KMKoushik KMKoushik changed the title Add transfer vault capability for crab v2 WIP: Add transfer vault capability for crab v2 Jun 16, 2022
@KMKoushik
Copy link
Contributor Author

@aparnakr @aleone Lot's of changes needed in vault transfer logic to implement timelock. So going to use the same PR. Not ready yet

@KMKoushik KMKoushik changed the title WIP: Add transfer vault capability for crab v2 Add transfer vault capability for crab v2 Jun 17, 2022
KMKoushik
KMKoushik commented Jun 17, 2022
View reviewed changes
packages/hardhat/contracts/strategy/timelock/SafeMath.sol
@@ -0,0 +1,191 @@
// SPDX-License-Identifier: BSD-3-Clause
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Don't want to change any code from the compound, so I copied math lib too

KMKoushik
KMKoushik commented Jun 17, 2022
View reviewed changes
packages/hardhat/hardhat.config.ts
@@ -212,7 +212,18 @@ const config: HardhatUserConfig = {
solidity: {
compilers: [
UNISWAP_SETTING,
]
],
overrides: {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Timelock related contracts need 0.8.10 as it's used by compound

@KMKoushik KMKoushik requested a review from aleone June 17, 2022 09:14
aparnakr
aparnakr approved these changes Jun 17, 2022
View reviewed changes
Copy link
Contributor

@aparnakr aparnakr left a comment

Choose a reason for hiding this comment

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

Are the tests also mostly from Compound? It might be fine to just port over and edit their tests.

@aparnakr
Copy link
Contributor

Is the contract size below the limit now?

aleone
aleone approved these changes Jun 17, 2022
View reviewed changes
Copy link
Contributor

@aleone aleone left a comment

Choose a reason for hiding this comment

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

lgtm

@KMKoushik
Copy link
Contributor Author

Are the tests also mostly from Compound? It might be fine to just port over and edit their tests.

That's what I did almost

@KMKoushik
Copy link
Contributor Author

Is the contract size below the limit now?

Yep

@KMKoushik
Copy link
Contributor Author

Will be merged in #530

@KMKoushik KMKoushik closed this Jun 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aleone aleone aleone approved these changes

@aparnakr aparnakr aparnakr approved these changes

@sanandnarayan sanandnarayan Awaiting requested review from sanandnarayan

Assignees
No one assigned
Labels
Crab v2
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@KMKoushik @aparnakr @aleone