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

Split Test Case #164

Open
wants to merge 61 commits into
base: master
Choose a base branch
from
Open

Split Test Case #164

wants to merge 61 commits into from

Conversation

coburncoburn
Copy link
Contributor

No description provided.

TylerEther and others added 30 commits July 28, 2021 13:46
- Adds compBorrowSpeeds and compSupplySpeeds to ComptrollerV6Storage
- Change _setCompSpeed to _setCompSpeeds - one function call to update distribution speeds for multiple markets
- Change signature of setCompSpeedInternal - compSpeed split up into two parts: compSupplySpeed and compBorrowSpeed
…ompBorrowIndex

- compSupplySpeeds and compBorrowSpeeds were flipped
- These tests have been manually verified to fail when they are expected to fail => they work well
- Supply state and borrow state now initialized when a market is added, ensuring market state indices are always set
- Now that market state indices are non lazily set, we can simply update market states when updating COMP speeds
- Adds changes from PR #173
- Second mint with comp accrued: reduced by 695
- Claim comp: reduced by 1268
- Old scen test used ComptrollerG3
- Removed test for removed function which never actually worked
- We need to call the upgrade function using the delegator address to use the storage at that contract address
- Asserts that the upgrade proposal passes
- Asserts that old comp speeds are deleted
- Asserts that new comp speeds are the same as they were previously
- Asserts that market state indices are the same post upgrade, with 0 values initialized to 1e36
- Asserts that the 2 comp speed bugs were fixed
- Asserts that the new _setCompSpeeds function works
- Asserts that comp rewards accrual works
- Ensures supply and borrow states properly initialized
- Ensures COMP is accrued correctly when COMP rewards are added (after market activation), removed, then added again
  - This test covers the COMP speed bug identified on August 09, 2021
- The logic didn't flow properly
- Underflow occurs when calling distributeBorrowerComp on markets whose borrowState.index didn't start at 1e36.
- The COMP rewards on those markets work fine, so we just need to make a fix for people borrowing cMKR, cAAVE, and the other new markets whose indices haven't been set yet, which this commit does.
- The change to distributeBorrowerComp to account for borrowers with uninitialized state indices increased the gas costs unfortunately
- Ensures new COMP speeds apply to both prior borrowers+suppliers and later borrowers+suppliers correctly
- Ensures new COMP speeds apply to both prior borrowers+suppliers and later borrowers+suppliers correctly w/ uninitialized prior borrower/supplier state indices
- Oof, the necessary change to distributeSupplierComp increased the gas cost of claim
- Lastly, gm
- The failing test of the prior commit is now passing
- There are COMP accrual values we need to fix before we can fully allow this
- Allows COMP to be transferred to only the users which haven't interacted with the problematic markets
@hayesgm hayesgm changed the title Cob/split speed scen Split Test Case Oct 1, 2021
TylerEther and others added 7 commits October 1, 2021 16:21
- Ensures proposal 64 does what it's expected to do
- Please use the current mainnet configurations for this to run properly
This patch simply adds a test-case to show how some accounts interact with the current Comptroller.
@hayesgm
Copy link
Collaborator

hayesgm commented Oct 4, 2021

Note: this patch now adds a check on #165 with a new scenario that tests how the upgrade would affect a variety of accounts.

Assert Equal (Erc20 COMP TokenBalance Geoff) 2000

-- Start with G8, upgrade to G9 (bug), then upgrade to latest
Only "Claim COMP across upgrade"
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be only?

"seventh": 6
"seventh": 6,

"tyler": 7,
Copy link
Contributor

Choose a reason for hiding this comment

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

😎

@jflatow
Copy link
Contributor

jflatow commented Nov 16, 2021

Did the changes here already get included in merged changes?

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.

6 participants