-
Notifications
You must be signed in to change notification settings - Fork 18
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 checkpoints to staking contract #25
Conversation
146454a
to
a3bac0b
Compare
52109e9
to
b8a5024
Compare
b8a5024
to
defbde1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess there is missing commit with new optimizer runs or something causing the TokenStaking deployment to fail during tests
Have we not just exceeded 24KB contract size limit with the additions from this PR? |
Yeah, but reducing the number of optimization rounds allows to be right under 24KB |
Co-authored-by: Vicky Zotova <vikki.zta@gmail.com>
3183933
to
61bcfe7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is some infrastructure-level problem with the draft system tests job. Given system tests are still in draft, I am ignoring it and merging.
@@ -697,7 +714,7 @@ contract TokenStaking is Ownable, IStaking { | |||
"Too much to unstake" | |||
); | |||
operatorStruct.nuInTStake -= amount; | |||
|
|||
decreaseStakeCheckpoint(operator, amount); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick but we could keep the same order of operations of emitting event, updating staked amount, and decreasing checkpoint in unstake
, unstakeKeep
, and unstakeNu
.
function delegateVoting(address operator, address delegatee) external { | ||
delegate(operator, delegatee); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if we rename delegate
function in T
token contract to delegateVoting
to keep them aligned?
solidity-contracts/contracts/token/T.sol
Line 79 in fa83b98
function delegate(address delegatee) public virtual { |
We could also rename the function in Checkpoint
so that we do not need to have function delegate(address operator, address delegatee)
a few lines below and just put this logic into delegateVoting
.
expect(await tokenStaking.getVotes(delegatee.address)).to.equal( | ||
expectedAmount | ||
) | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should add a test that it should decrease the total voting power
as well.
Adds checkpoints to staking contract, so that stakers can collect and delegate voting power in proportion to their stake size.
Some important aspects to note: