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

Rollback ve changes #696

Merged
merged 5 commits into from
Sep 1, 2022
Merged

Rollback ve changes #696

merged 5 commits into from
Sep 1, 2022

Conversation

trizin
Copy link
Contributor

@trizin trizin commented Aug 31, 2022

Changes proposed in this PR:

Rollback changes to ve contracts.

@trizin trizin marked this pull request as ready for review August 31, 2022 14:48
@trentmc
Copy link
Member

trentmc commented Aug 31, 2022

Thanks. Could you show a diff of our code, and the original ve code? Perhaps right before doing the diff, replace CRV with OCEAN.

@trizin
Copy link
Contributor Author

trizin commented Aug 31, 2022

I've updated the veDelegation and veDelegationProxy contracts with the latest from Curve.

We made modifications in veOCEAN.vy and veFeeDistributor.vy.
Here is the veFeeDistributor diff
Here is the veOCEAN diff

@trentmc
Copy link
Member

trentmc commented Aug 31, 2022

We made modifications in veOCEAN.vy and veFeeDistributor.vy. Here is the veFeeDistributor diff Here is the veOCEAN diff

That's the diff between our code before the PR, and our code after the PR. (Or did I miss something?)

I'm interested to see the diff between our code after the PR, and Curve's code.

@trizin
Copy link
Contributor Author

trizin commented Aug 31, 2022

Oh I see. In this PR, for each contract, I copy pasted the code from the Curve repo and replaced CRV with OCEAN in the code comments.

Here are the original contracts from Curve repos:
veOCEAN (Change: A small fix in code comment)
veFeeDistributor (Change: Replaced CRV with OCEAN in comments)
veDelegation (Change: Replaced CRV with OCEAN in comments)
veDelegationProxy (Change: Replaced CRV with OCEAN in comments)

The contracts should be identical, excluding the comments.

@alexcos20
Copy link
Member

Copy link
Member

@alexcos20 alexcos20 left a comment

Choose a reason for hiding this comment

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

lgtm

@alexcos20 alexcos20 merged commit 840c835 into main Sep 1, 2022
@alexcos20 alexcos20 deleted the rollback-ve-changes branch September 1, 2022 12:49
@trizin
Copy link
Contributor Author

trizin commented Sep 1, 2022

@alexcos20 in the previous version I upgraded the contract versions. In the PR, I set them as in original contracts in case there could be a security issue. Maybe I'm being too paranoid.

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.

None yet

3 participants