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

Add signature-based onBehalf methods and natspec #43

Merged
merged 2 commits into from
Feb 8, 2024
Merged

Add signature-based onBehalf methods and natspec #43

merged 2 commits into from
Feb 8, 2024

Conversation

wildmolasses
Copy link
Collaborator

Resolves #29

@wildmolasses wildmolasses force-pushed the behalfOf branch 6 times, most recently from 18fc9bb to 2bbad9f Compare February 5, 2024 18:44
@wildmolasses wildmolasses force-pushed the behalfOf branch 2 times, most recently from 30978a5 to 399fbb6 Compare February 6, 2024 23:43
@wildmolasses wildmolasses marked this pull request as ready for review February 6, 2024 23:43
test/UniStaker.t.sol Outdated Show resolved Hide resolved
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.

Hey @wildmolasses some comments after an initial pass through the contracts only. Looking forward to a walk through on the tests.

src/UniStaker.sol Outdated Show resolved Hide resolved
src/UniStaker.sol Outdated Show resolved Hide resolved
src/UniStaker.sol Show resolved Hide resolved
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 good, all small comments

totalDeposits[deposit.owner] += _amount;
earningPower[deposit.beneficiary] += _amount;
deposit.balance += _amount;
emit StakeDeposited(_depositId, _amount, deposit.balance);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just a reminder that once this is rebased on main this should have the deposit.owner

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done!

src/UniStaker.sol Show resolved Hide resolved
test/UniStaker.t.sol Show resolved Hide resolved
test/UniStaker.t.sol Show resolved Hide resolved
@wildmolasses wildmolasses force-pushed the behalfOf branch 2 times, most recently from 42a386c to 04259c7 Compare February 7, 2024 22:54
Copy link

github-actions bot commented Feb 7, 2024

Coverage after merging behalfOf into main will be

100.00%

Coverage Report
FileStmtsBranchesFuncsLinesUncovered Lines
src
   UniStaker.sol99.48%96.15%100%100%547
   V3FactoryOwner.sol100%100%100%100%

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.

A couple small things @wildmolasses, but overall this looks great.

src/UniStaker.sol Show resolved Hide resolved
src/UniStaker.sol Outdated Show resolved Hide resolved
src/UniStaker.sol Show resolved Hide resolved
test/UniStaker.t.sol Show resolved Hide resolved
test/UniStaker.t.sol Show resolved Hide resolved
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.

Awesome work on this @wildmolasses

@apbendi apbendi merged commit b278fd8 into main Feb 8, 2024
0 of 4 checks passed
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.

Implement Signature Based onBehalf Methods for Staking Actions
3 participants