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

Wcusdcv3 rewards fix #1016

Merged
merged 6 commits into from
Nov 30, 2023
Merged

Wcusdcv3 rewards fix #1016

merged 6 commits into from
Nov 30, 2023

Conversation

tbrent
Copy link
Collaborator

@tbrent tbrent commented Nov 28, 2023

  • Add regression test for large COMP reward amounts; confirmed fails on previous implementation
  • Implement fix to avoid overflow. I considered changing all uint64 to uint256, but this would break the re-use of comet functions in CometHelper. It would actually be a much larger change to do this, and I don't think it's worth the cost. Just changing the storage type for baseTrackingAccrued is I think the minimal fix that is also slightly more robust than just adding a uint256 cast to the broken multiplication in L:199.

Needs review from everyone

julianmrodri
julianmrodri reviewed Nov 29, 2023
View reviewed changes
Copy link
Contributor

@julianmrodri julianmrodri left a comment •
edited
Loading

Choose a reason for hiding this comment

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

The change LGTM, have doubts if there is not an additional fix needed in L259

contracts/plugins/assets/compoundv3/CusdcV3Wrapper.sol Outdated Show resolved Hide resolved
test/plugins/individual-collateral/compoundv3/CusdcV3Wrapper.test.ts Show resolved Hide resolved
akshatmittal
akshatmittal reviewed Nov 29, 2023
View reviewed changes
Copy link
Member

@akshatmittal akshatmittal left a comment

Choose a reason for hiding this comment

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

Added a few comments, looks good overall.

I think it's also worth changing the RESCALE_FACTOR to a uint256. (GitHub doesn't let me comment on the line itself)

contracts/plugins/assets/compoundv3/CusdcV3Wrapper.sol Show resolved Hide resolved
test/plugins/individual-collateral/compoundv3/CusdcV3Wrapper.test.ts Show resolved Hide resolved
@pmckelvy1 pmckelvy1 merged commit 3a4ac7c into master Nov 30, 2023
9 checks passed
@pmckelvy1 pmckelvy1 deleted the wcusdcv3-rewards branch November 30, 2023 03:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@julianmrodri julianmrodri julianmrodri left review comments

@akshatmittal akshatmittal akshatmittal left review comments

@pmckelvy1 pmckelvy1 pmckelvy1 approved these changes

@jankjr jankjr Awaiting requested review from jankjr

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

Successfully merging this pull request may close these issues.
5 participants
@tbrent @akshatmittal @pmckelvy1 @jankjr @julianmrodri