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

Mechanics refactor #51

Merged
merged 9 commits into from
Feb 7, 2024
Merged

Mechanics refactor #51

merged 9 commits into from
Feb 7, 2024

Conversation

apbendi
Copy link
Collaborator

@apbendi apbendi commented Feb 6, 2024 •
edited
Loading

closes #41

@apbendi apbendi marked this pull request as ready for review February 6, 2024 15:46
alexkeating
alexkeating reviewed Feb 6, 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.

Looks good, naming nits and I have to think about the rounding a little more

src/UniStaker.sol Outdated Show resolved Hide resolved
src/UniStaker.sol Outdated Show resolved Hide resolved
src/UniStaker.sol Show resolved Hide resolved
test/UniStaker.t.sol Outdated Show resolved Hide resolved
test/UniStaker.t.sol Show resolved Hide resolved
@apbendi apbendi force-pushed the mechanics-refactor branch 2 times, most recently from e162830 to 0438d29 Compare February 7, 2024 01:26
wildmolasses
wildmolasses reviewed Feb 7, 2024
View reviewed changes
Copy link
Collaborator

@wildmolasses wildmolasses 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 mid-review comments, gonna keep reviewing in a bit

src/UniStaker.sol Outdated Show resolved Hide resolved
src/UniStaker.sol Outdated Show resolved Hide resolved
test/UniStaker.t.sol Show resolved Hide resolved
wildmolasses
wildmolasses reviewed Feb 7, 2024
View reviewed changes
src/UniStaker.sol Outdated Show resolved Hide resolved
wildmolasses
wildmolasses reviewed Feb 7, 2024
View reviewed changes
src/UniStaker.sol Outdated Show resolved Hide resolved
wildmolasses
wildmolasses reviewed Feb 7, 2024
View reviewed changes
src/UniStaker.sol Outdated Show resolved Hide resolved
wildmolasses
wildmolasses reviewed Feb 7, 2024
View reviewed changes
src/UniStaker.sol Outdated Show resolved Hide resolved
wildmolasses
wildmolasses reviewed Feb 7, 2024
View reviewed changes
src/UniStaker.sol Outdated Show resolved Hide resolved
alexkeating
alexkeating approved these changes Feb 7, 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.

Looks good, interested to hear Ed's thoughts on some of the naming issues, but they shouldn't hold up approval. Also, I don't think the rounding precision should ever not favor the contract.

@apbendi apbendi force-pushed the mechanics-refactor branch from 0438d29 to 646cf2f Compare February 7, 2024 21:28
@apbendi
Update natspec to use checkpoint instead of snapshot
26f88e9
@apbendi
Split and rename the internal method for checkpointing global & accou…
171c9ec 
…nt rewards
@apbendi
Avoid overwriting timestamp twice in claim rewards method
7e48d2a
@apbendi
Move the scale factor into the reward rate for greater precision
4d974ba 
We had to update the way percentages were calculated in our tests,
and also fix a test, to avoid introducing too much precision error
in our expectations.
@apbendi
Rename vars and methods associated with staking mechanics for clarity
56ad600 
We move away from some of the variable and method names we inherited from
the synthetix staking contract in favor of more explicit and clear names
where possible. In particular, we introduce language around checkpointing
and accumulators to make it more obvious how the mechanism works. We also
add clarity around reward related methods to make it clear the rewards in
question are only unclaimed rewards.
@apbendi
Move, rename, and fix test to ensure rewards earned are less than rec…
e18c418 
…eived
@apbendi
Bump SCALE_FACTOR used by UniStaker to 1e36
536cbde
@apbendi apbendi force-pushed the mechanics-refactor branch from 646cf2f to 4a740e6 Compare February 7, 2024 21:35
wildmolasses
wildmolasses approved these changes Feb 7, 2024
View reviewed changes
Copy link
Collaborator

@wildmolasses wildmolasses left a comment

Choose a reason for hiding this comment

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

lgtm, a couple small things i caught

src/UniStaker.sol Outdated Show resolved Hide resolved
src/UniStaker.sol Outdated Show resolved Hide resolved
src/UniStaker.sol Outdated Show resolved Hide resolved
@apbendi apbendi force-pushed the mechanics-refactor branch from 91124c3 to 3355b40 Compare February 7, 2024 22:03
@github-actions GitHub Actions
Copy link

github-actions bot commented Feb 7, 2024

Coverage after merging mechanics-refactor into main will be

100.00%

Coverage Report
FileStmtsBranchesFuncsLinesUncovered Lines
src
   UniStaker.sol99.32%95.45%100%100%389
   V3FactoryOwner.sol100%100%100%100%

@apbendi apbendi merged commit 38f42e3 into main Feb 7, 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

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

Improvements to Reward Mechanics Code
3 participants
@apbendi @wildmolasses @alexkeating