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: setClaimRecipient #99

Open
wants to merge 1 commit into
base: v2
Choose a base branch
from
Open

feat: setClaimRecipient #99

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 a044c21 during Forge Coverage #534

Summary coverage rate:
  lines......: 100.0% (231 of 231 lines)
  functions..: 100.0% (54 of 54 functions)
  branches...: 91.7% (22 of 24 branches)

Files changed coverage rate:
                       |Lines       |Functions  |Branches    
  Filename             |Rate     Num|Rate    Num|Rate     Num
  ===========================================================
  src/WrappedMToken.sol|23.0%    226|46.2%    52|    -      0

@deluca-mike deluca-mike force-pushed the feat/set-claim-recipient branch 2 times, most recently 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
toninorair
toninorair reviewed Jan 15, 2025
View reviewed changes
src/WrappedMToken.sol Outdated
uint256(_getFromRegistrar(keccak256(abi.encode(CLAIM_OVERRIDE_RECIPIENT_KEY_PREFIX, account_))))
)
);
(_accounts[account_].hasClaimRecipient)
Copy link
Contributor

@toninorair toninorair Jan 15, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

consider adding this logic here [suggestion, can lead to cleaner message for user]

address claimOverrideRecipient_ = claimRecipientFor(account_);
address claimRecipient_ = claimOverrideRecipient_ == address(0) ? account_ : claimOverrideRecipient_;```

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, let's confirm the order once again whether the governance has first priority, then owner of account, then default.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Governance does not have first priority because:

  • it would allow governance to retroactively steal unclaimed yield
  • it costs more gas

toninorair
toninorair reviewed 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, let's confirm order of setting claimant with product/Greg once again

@deluca-mike deluca-mike force-pushed the feat/set-claim-recipient branch 3 times, most recently from 430773c to d3acb5a Compare January 16, 2025 17:49
@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/v2-prep branch 2 times, most recently from ce4261b to 07f4299 Compare January 17, 2025 18:31
@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
Base automatically changed from feat/v2-prep to v2 January 21, 2025 19:00
@deluca-mike deluca-mike force-pushed the feat/set-claim-recipient branch 2 times, most recently from 99fe3ce to be37ff9 Compare January 21, 2025 20:23
@deluca-mike deluca-mike force-pushed the feat/set-claim-recipient branch 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

@PierrickGT PierrickGT PierrickGT approved these changes

@0xIryna 0xIryna Awaiting requested review from 0xIryna

@Oighty Oighty Awaiting requested review from Oighty

@toninorair toninorair Awaiting requested review from toninorair

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