Skip to content

Conversation

@pata-eth
Copy link

@pata-eth pata-eth commented May 17, 2022

FWIW, my 'arbitrum-2crv' branch was created from the 'arbitrum-tricrypto' branch, but I noticed that your 'arbitrum-2crv' branch was created from main. I didn't notice any material differences in the 'diff'. I look forward to your feedback. Thank you!

Issue: https://github.com/yearn/yearn-strategies/issues/276

@pata-eth pata-eth marked this pull request as draft May 19, 2022 02:20
@pata-eth
Copy link
Author

converted this one to draft until we sort out the reward claim process for all new curve gauges.

@pata-eth pata-eth changed the title Arbitrum 2crv ready for review Curve 2crv Strat: first release to Arbitrum May 19, 2022
@pata-eth pata-eth marked this pull request as ready for review May 20, 2022 03:44
@pata-eth
Copy link
Author

@poolpitako and @dudesahn, given the material changes of the commit I just pushed, we may want to have poolpi review its diff so that he can confirm he still approves the PR as it currently stands. Thank you both!

@pata-eth
Copy link
Author

Screen Shot 2022-05-23 at 10 05 41 PM
@poolpitako and @dudesahn, I've updated this PR in line with the feedback I received for ftm-tricrypto. Please let me know when you are ready to involve security. Thank you

@pata-eth
Copy link
Author

Thanks again, for the feedback @dudesahn ! I've fixed my linter and updated the code according to your feedback. We still have a few open conversations unresolved, but I think this is ready for @poolpitako's final review. I may submit one more change based on how we resolve our pending items. Thank you

@pata-eth
Copy link
Author

pata-eth commented Jun 7, 2022

Screen Shot 2022-06-07 at 4 50 08 PM
@dudesahn, I added keepCRV back. I think I've now taken care of all the requests. Please validate at your earliest convenience. Thank you

@dudesahn
Copy link
Owner

dudesahn commented Jun 9, 2022

https://github.com/pata-eth/StrategyCurveTemplate/blob/16c2cbd3ae25ad38b42e400202ed54df5abedc95/contracts/StrategyCurveTwoPool.sol#L222

you can delete this line since you don't call that function anywhere

@dudesahn
Copy link
Owner

dudesahn commented Jun 9, 2022

non-blocking, but in the future I would encourage you to use natspec for your comments of functions/state vars—that way they show up nicely on etherscan. I did this for my most recent iteration of strategies and am pretty happy with it.

@dudesahn
Copy link
Owner

dudesahn commented Jun 9, 2022

https://github.com/pata-eth/StrategyCurveTemplate/blob/16c2cbd3ae25ad38b42e400202ed54df5abedc95/contracts/StrategyCurveTwoPool.sol#L16

Also, I think you can delete lines 16 and 18, as these are already inherited from the baseStrategy. again, not a big deal tho.

@dudesahn
Copy link
Owner

dudesahn commented Jun 9, 2022

LGTM as my comments are non-blocking

@pata-eth
Copy link
Author

pata-eth commented Jun 9, 2022

non-blocking, but in the future I would encourage you to use natspec for your comments of functions/state vars—that way they show up nicely on etherscan. I did this for my most recent iteration of strategies and am pretty happy with it.

Absolutely. In my new projects I'm already incorporating natspec. So much better!

@pata-eth
Copy link
Author

pata-eth commented Jun 9, 2022

https://github.com/pata-eth/StrategyCurveTemplate/blob/16c2cbd3ae25ad38b42e400202ed54df5abedc95/contracts/StrategyCurveTwoPool.sol#L16

Also, I think you can delete lines 16 and 18, as these are already inherited from the baseStrategy. again, not a big deal tho.

Do you know why we can remove these lines but not lines 6 and 7?

@dudesahn
Copy link
Owner

dudesahn commented Jun 9, 2022

you should be able to remove those as well :)

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