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

feat: Earner Manager #100

Open
wants to merge 1 commit into
base: feat/set-claim-recipient
Choose a base branch
from
Open

feat: Earner Manager #100

wants to merge 1 commit into from

Conversation

deluca-mike
Copy link
Collaborator

No description provided.

@deluca-mike deluca-mike added the enhancement New feature or request label Jan 13, 2025
@deluca-mike deluca-mike self-assigned this Jan 13, 2025
@github-actions GitHub Actions
Copy link

github-actions bot commented Jan 13, 2025 •
edited
Loading

LCOV of commit 848f90c during Forge Coverage #535

Summary coverage rate:
  lines......: 99.4% (334 of 336 lines)
  functions..: 100.0% (69 of 69 functions)
  branches...: 92.0% (46 of 50 branches)

Files changed coverage rate:
                                 |Lines       |Functions  |Branches    
  Filename                       |Rate     Num|Rate    Num|Rate     Num
  =====================================================================
  src/EarnerManager.sol          |18.8%     80| 133%    15|    -      0
  src/WrappedMToken.sol          |20.9%    249|57.7%    52|    -      0
  src/WrappedMTokenMigratorV1.sol|40.0%      5| 0.0%     2|    -      0

@deluca-mike deluca-mike force-pushed the feat/set-claim-recipient branch from 869827e to 4c1d530 Compare January 13, 2025 21:59
@deluca-mike deluca-mike force-pushed the feat/set-claim-recipient branch from 4c1d530 to 2c7cfc7 Compare January 14, 2025 18:43
@deluca-mike deluca-mike force-pushed the feat/set-claim-recipient branch from 2c7cfc7 to afbf5fc Compare January 14, 2025 19:17
PierrickGT
PierrickGT reviewed Jan 15, 2025
View reviewed changes
script/DeployProduction.s.sol Outdated Show resolved Hide resolved
script/DeployProduction.s.sol Outdated Show resolved Hide resolved
src/EarnerManager.sol
EarnerDetails storage details_ = _earnerDetails[account_];

// NOTE: Not using `isInAdministratedEarnersList(account_)` here to avoid redundant storage reads.
return _isValidAdmin(details_.admin) ? (true, details_.feeRate, details_.admin) : (false, 0, address(0));
Copy link
Member

Choose a reason for hiding this comment

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

Does this mean that if an admin is removed from the registrar, _isValidAdmin(details_.admin) will return false and now anyone can call stopEarningFor() on Wrapped M?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes. If the only reason an account is earning is because of an admin, and that admin is not longer approved, the that account is no longer approved instantly.

src/interfaces/IEarnerManager.sol Outdated Show resolved Hide resolved
src/interfaces/IEarnerManager.sol Outdated Show resolved Hide resolved
src/interfaces/IEarnerManager.sol Outdated Show resolved Hide resolved
src/interfaces/IEarnerManager.sol
/**
* @notice Sets the status for `account` to `status`.
* @notice If approving an earner that is already earning, but was recently removed from the Registrar earners list,
* call `wrappedM.stopEarning(account)` before calling this, then call `wrappedM.startEarning(account)`.
Copy link
Member

Choose a reason for hiding this comment

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

Is this required because hasEarnerDetails wouldn't be set otherwise?
Does that mean that the earner would still earn but no fee would be taken from the yield since hasEarnerDetails will be false?

Copy link
Collaborator Author

@deluca-mike deluca-mike Jan 16, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Correct. Earning WM is a stateful status that needs to be explicitly stopped. Directing a fee to the admin is a "claim time" process based on the details at that time. Note that hasEarnerDetails will not be false if the originally approving admin is no longer valid, but rather, the _getEarnerDetails query will return a zero feeRate and zero admin.

toninorair
toninorair approved these changes Jan 15, 2025
View reviewed changes
Copy link
Contributor

@toninorair toninorair 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, any extra changes comparing to the last approved/merged/audited version ?

@deluca-mike deluca-mike force-pushed the feat/set-claim-recipient branch 2 times, most recently from 455dd19 to 430773c Compare January 16, 2025 17:47
@deluca-mike deluca-mike force-pushed the feat/set-claim-recipient branch from 430773c to d3acb5a Compare January 16, 2025 17:49
@deluca-mike
Copy link
Collaborator Author

looks good, any extra changes comparing to the last approved/merged/audited version ?

Other than more tests, there should not be.

@deluca-mike deluca-mike force-pushed the feat/set-claim-recipient branch from d3acb5a to 7d256ec Compare January 16, 2025 20:40
@deluca-mike deluca-mike force-pushed the feat/set-claim-recipient branch from 7d256ec to 8d7134c Compare January 16, 2025 21:49
@deluca-mike deluca-mike force-pushed the feat/earner-manager branch 3 times, most recently from 821a060 to 1d2c2b0 Compare January 17, 2025 15:56
@deluca-mike deluca-mike force-pushed the feat/set-claim-recipient branch from 8d7134c to 2c9338d Compare January 17, 2025 19:04
@deluca-mike deluca-mike force-pushed the feat/set-claim-recipient branch from 2c9338d to dfb97c4 Compare January 17, 2025 19:39
@deluca-mike deluca-mike force-pushed the feat/set-claim-recipient branch from dfb97c4 to 72915e7 Compare January 17, 2025 19:46
@deluca-mike deluca-mike force-pushed the feat/set-claim-recipient branch from 72915e7 to d9b89fd Compare January 20, 2025 17:14
@deluca-mike deluca-mike force-pushed the feat/set-claim-recipient branch from d9b89fd to c1d9903 Compare January 21, 2025 18:57
@deluca-mike deluca-mike force-pushed the feat/set-claim-recipient branch from c1d9903 to 99fe3ce Compare January 21, 2025 19:02
@deluca-mike deluca-mike force-pushed the feat/set-claim-recipient branch 2 times, most recently from be37ff9 to a044c21 Compare January 21, 2025 20:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@toninorair toninorair toninorair approved these changes

@0xIryna 0xIryna Awaiting requested review from 0xIryna

@Oighty Oighty Awaiting requested review from Oighty

@PierrickGT PierrickGT Awaiting requested review from PierrickGT

Assignees

@deluca-mike deluca-mike

Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@deluca-mike @toninorair @PierrickGT