-
Notifications
You must be signed in to change notification settings - Fork 98
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
OA tx : Balancer pfei accounting fix + remove PCVDeposits with bad debts #776
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. I'd probably add a unit test to validate the fix, but overall looks great
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
proposals/description/oa_cr_fix.ts
Outdated
target: 'collateralizationOracle', | ||
values: '0', | ||
method: 'swapDeposit(address,address)', | ||
arguments: ['{balancerLensBpt30Fei70Weth}', '{balancerLensBpt30Fei70WethFixed}'], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
now is a good time to rethink our naming conventions. The arbitrary suffix "fixed" is not a super scalable convention.
If it has the same ABI I'd prefer making the fixed one called balancerLensBpt30Fei70Weth
and naming the old one "old" or
"v1" or something
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah would prefer version numbers over "fixed"
Reminds me of how I used to name files in folders for school assignments. "Assignment1Fixed", "Assignment1FixedFinal", "Assignment1FixedFinalFinalSubmitThis.doc"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
renamed the old one to balancerLensBpt30Fei70WethOld
and use the balancerLensBpt30Fei70Weth
name for the new one.
Updated the PR description / screenshot / calldata, I think we're good to ship |
OA Action to fix CR accounting
Accounted PCV loss :
Accounted protocol FEI becoming circulating :
About the Balancer lens fix :
It used to report all the FEI in the B-70WETH-30FEI pool as protocol-owned (28M) and not only the FEI that is within the LP tokens held by the protocol. The protocol owns ~70% of the pool, so it used to count 28M porotocol FEI (the whole pool) instead of 19M (70% of total).
New lens address : https://etherscan.io/address/0x673f7dfa863b611de657759aede629b260f4e682