Skip to content

Conversation

@Flocqst
Copy link
Contributor

@Flocqst Flocqst commented Jul 2, 2024

Remove the staking cooldown for the stakingv2 contracts.

Description

  • Remove staking cooldown from contracts/StakingRewardsV2.sol, contracts/RewardEscrowV2.sol and contracts/EscrowMigrator.sol
  • Update tests to reflect removal of staking cooldown

Motivation and Context

KIP-127: Staking V2 Upgrade

@Flocqst Flocqst requested a review from etn0m July 2, 2024 15:04
@Flocqst Flocqst changed the title Staking cooldown Staking cooldown removal Jul 2, 2024
Copy link
Contributor

@tommyrharper tommyrharper left a comment

Choose a reason for hiding this comment

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

Nice work @Flocqst 💪 🔥 🎉 - glad to see this getting ripped out! Left a few comments here and there, nothing major :)

Copy link
Contributor

@JaredBorders JaredBorders left a comment

Choose a reason for hiding this comment

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

preliminary review - just waiting for our sync to go further

@JaredBorders
Copy link
Contributor

please add your name/email/gh handle as author to contracts you modified.

@Flocqst
Copy link
Contributor Author

Flocqst commented Jul 17, 2024

Thanks for the review @tommyrharper, took into account almost every comment you had except for the suggestion i did not include because of the incoming #252.

Copy link
Contributor

@JaredBorders JaredBorders left a comment

Choose a reason for hiding this comment

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

Logic-wise, everything looks good to me. Nice work 💪

The only point of concern:
Removing the cooldown could potentially lead to governance attacks, depending on the voting mechanism implementation.

For instance, with snapshot voting, a malicious actor could utilize a flashloan to artificially inflate voting power if they knew the exact block of the snapshot. This risk should be documented in the interface or another appropriate location.

Copy link
Contributor

@tommyrharper tommyrharper left a comment

Choose a reason for hiding this comment

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

Nice work, thanks for addressing all my concerns!

@Flocqst Flocqst closed this Sep 3, 2024
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.

4 participants