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

Add BinaryEligibilityOracleEarningPowerCalculator.sol and tests #24

Merged
merged 5 commits into from
Oct 9, 2024
Merged

Add BinaryEligibilityOracleEarningPowerCalculator.sol and tests #24

merged 5 commits into from
Oct 9, 2024

Conversation

garyghayrat
Copy link
Collaborator

closes #13

@garyghayrat garyghayrat marked this pull request as ready for review September 26, 2024 14:05
apbendi
apbendi reviewed Sep 27, 2024
View reviewed changes
Copy link
Collaborator

@apbendi apbendi left a comment

Choose a reason for hiding this comment

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

Seriously excellent work on this @garyghayrat, overall it's looking great. I left some comments on things we can address and/or pull into issues. But this is a really great start—you've gotten a ton done 🚀

src/EarningPowerCalculator.sol Outdated Show resolved Hide resolved
src/EarningPowerCalculator.sol Outdated Show resolved Hide resolved
src/EarningPowerCalculator.sol Outdated Show resolved Hide resolved
src/EarningPowerCalculator.sol Outdated Show resolved Hide resolved
src/EarningPowerCalculator.sol Outdated Show resolved Hide resolved
test/EarningPowerCalculator.t.sol Outdated Show resolved Hide resolved
test/EarningPowerCalculator.t.sol Outdated Show resolved Hide resolved
test/EarningPowerCalculator.t.sol Outdated Show resolved Hide resolved
test/EarningPowerCalculator.t.sol Outdated Show resolved Hide resolved
test/EarningPowerCalculator.t.sol Outdated Show resolved Hide resolved
@garyghayrat garyghayrat force-pushed the earning-power-calculator branch 2 times, most recently from e47e774 to 29fce43 Compare October 1, 2024 21:59
@garyghayrat garyghayrat requested a review from apbendi October 1, 2024 22:08
@garyghayrat garyghayrat force-pushed the earning-power-calculator branch from 29fce43 to e7dff49 Compare October 1, 2024 22:24
@garyghayrat garyghayrat changed the title Add EarningPowerCalculator.sol and tests Add BinaryEligibilityOracleEarningPowerCalculator.sol and tests Oct 2, 2024
alexkeating
alexkeating reviewed Oct 2, 2024
View reviewed changes
Copy link
Collaborator

@alexkeating alexkeating left a comment

Choose a reason for hiding this comment

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

Lots of good stuff in here

src/BinaryEligibilityOracleEarningPowerCalculator.sol Outdated Show resolved Hide resolved
src/BinaryEligibilityOracleEarningPowerCalculator.sol Outdated Show resolved Hide resolved
src/BinaryEligibilityOracleEarningPowerCalculator.sol Outdated Show resolved Hide resolved
src/BinaryEligibilityOracleEarningPowerCalculator.sol Outdated Show resolved Hide resolved
src/BinaryEligibilityOracleEarningPowerCalculator.sol Outdated Show resolved Hide resolved
test/BinaryEligibilityOracleEarningPowerCalculator.t.sol Outdated Show resolved Hide resolved
test/BinaryEligibilityOracleEarningPowerCalculator.t.sol Show resolved Hide resolved
test/BinaryEligibilityOracleEarningPowerCalculator.t.sol Outdated Show resolved Hide resolved
test/BinaryEligibilityOracleEarningPowerCalculator.t.sol Outdated Show resolved Hide resolved
src/BinaryEligibilityOracleEarningPowerCalculator.sol Outdated Show resolved Hide resolved
alexkeating
alexkeating reviewed Oct 2, 2024
View reviewed changes
Copy link
Collaborator

@alexkeating alexkeating 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.

I will give some more suggestions for test names, but figures I should send over what I already have before tomorrow. In general I would recommend describing the scenario tested rather than the return and boolean checks.

src/BinaryEligibilityOracleEarningPowerCalculator.sol Outdated Show resolved Hide resolved
src/BinaryEligibilityOracleEarningPowerCalculator.sol Outdated Show resolved Hide resolved
src/BinaryEligibilityOracleEarningPowerCalculator.sol Outdated
) external view returns (uint256 _newEarningPower, bool _isQualifiedForUpdate) {
// If the oracle has not been updated for more than the stale oracle window, return full earning
// power and is qualified for update.
if (block.timestamp - lastOracleUpdateTime > STALE_ORACLE_WINDOW) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this could probably be enforced on when we set the eligibility delay.

src/BinaryEligibilityOracleEarningPowerCalculator.sol Outdated Show resolved Hide resolved
test/BinaryEligibilityOracleEarningPowerCalculator.t.sol Outdated Show resolved Hide resolved
test/BinaryEligibilityOracleEarningPowerCalculator.t.sol Outdated Show resolved Hide resolved
test/BinaryEligibilityOracleEarningPowerCalculator.t.sol Outdated Show resolved Hide resolved
test/BinaryEligibilityOracleEarningPowerCalculator.t.sol Outdated Show resolved Hide resolved
test/BinaryEligibilityOracleEarningPowerCalculator.t.sol Outdated Show resolved Hide resolved
test/BinaryEligibilityOracleEarningPowerCalculator.t.sol Outdated Show resolved Hide resolved
alexkeating
alexkeating reviewed Oct 3, 2024
View reviewed changes
Copy link
Collaborator

@alexkeating alexkeating left a comment

Choose a reason for hiding this comment

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

Looking great a few more nits and test name suggestions

test/BinaryEligibilityOracleEarningPowerCalculator.t.sol Outdated Show resolved Hide resolved
test/BinaryEligibilityOracleEarningPowerCalculator.t.sol Outdated Show resolved Hide resolved
test/BinaryEligibilityOracleEarningPowerCalculator.t.sol Outdated Show resolved Hide resolved
test/BinaryEligibilityOracleEarningPowerCalculator.t.sol Outdated Show resolved Hide resolved
test/BinaryEligibilityOracleEarningPowerCalculator.t.sol Outdated Show resolved Hide resolved
test/BinaryEligibilityOracleEarningPowerCalculator.t.sol Outdated Show resolved Hide resolved
test/BinaryEligibilityOracleEarningPowerCalculator.t.sol Outdated Show resolved Hide resolved
test/BinaryEligibilityOracleEarningPowerCalculator.t.sol Outdated Show resolved Hide resolved
alexkeating
alexkeating approved these changes Oct 4, 2024
View reviewed changes
Copy link
Collaborator

@alexkeating alexkeating left a comment

Choose a reason for hiding this comment

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

I this looks good, a couple small nits that can be handled in a followup pr

src/BinaryEligibilityOracleEarningPowerCalculator.sol Outdated Show resolved Hide resolved
src/BinaryEligibilityOracleEarningPowerCalculator.sol Outdated Show resolved Hide resolved
test/BinaryEligibilityOracleEarningPowerCalculator.t.sol Outdated Show resolved Hide resolved
@garyghayrat garyghayrat mentioned this pull request Oct 5, 2024
Open
@garyghayrat garyghayrat requested a review from apbendi October 5, 2024 02:26
@garyghayrat
Add EarningPowerCalculator and tests
09d146a 
Make `delegateScoreEligibilityThreshold` updatable

Add missing test and update comment

Merge lockDelegateScore and unlockDelegateScore and update tests

Rename delegate to delegatee

Rename `_isEligibleForUpdate` to `_isQualifiedForUpdate`

Rename `NotScoreOracle` to `Unauthorized`

Rename `EarningPowerCalculator` to `BinaryEligibilityOracleEarningPowerCalculator` and inherit `IEarningPowerCalculator`

Update comments around ScoreOracle, Karma, and reorder internal function

Rename tests

Add `lastDelegateeEligibilityChangeTime` and tests

Use local vars to simplify checking change in eligibility
@garyghayrat
Update naming and comments
f05b475 
Make `getEarningPower` logic more readable

Make `getNewEarningPower` more readable

Use  instead of modifiers and remove duplicate events and errors

Add test case for when owner is set to address 0

Make `STALE_ORACLE_WINDOW` immutable

Update error naming

Use internal functions

Simplify `getNewEarningPower` logic

Update comments and test names

Rename tests

Update natspec and rename vars

Change `lastDelegateeEligibilityChangeTime` to `timeOfIneligibility`

Move tests and correct names

Rename var and remove unused var

Refactor a test
@garyghayrat
Remove vm.assume and update natspec
ae7097b
@garyghayrat
Rename tests
7ea2e99
@garyghayrat garyghayrat force-pushed the earning-power-calculator branch from 38a84df to 7ea2e99 Compare October 8, 2024 01:16
@garyghayrat garyghayrat force-pushed the earning-power-calculator branch from 49bf69d to 8669241 Compare October 8, 2024 17:55
@github-actions GitHub Actions
Copy link

github-actions bot commented Oct 8, 2024

Coverage after merging earning-power-calculator into main will be

99.16%

Coverage Report
FileStmtsBranchesFuncsLinesUncovered Lines
src
   BinaryEligibilityOracleEarningPowerCalculator.sol100%100%100%100%
   DelegationSurrogate.sol100%100%100%100%
   GovernanceStaker.sol98.82%100%97.73%98.96%248–249

@garyghayrat garyghayrat mentioned this pull request Oct 9, 2024
Closed
apbendi
apbendi approved these changes Oct 9, 2024
View reviewed changes
Copy link
Collaborator

@apbendi apbendi left a 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! Make sure we squash all the commits into one with a clean message @garyghayrat.

@garyghayrat garyghayrat merged commit 5e86c54 into main Oct 9, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alexkeating alexkeating alexkeating approved these changes

@apbendi apbendi apbendi approved these changes

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

Successfully merging this pull request may close these issues.

Spec Issue For v1 Earning Power Calculator
3 participants
@garyghayrat @apbendi @alexkeating