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

Refactor Staking contract (met dragons - eip170) #448

Merged
merged 358 commits into from
Feb 13, 2023

Conversation

tjcloa
Copy link
Contributor

@tjcloa tjcloa commented Jul 23, 2022

  • Optimized modular system introduced
  • Staking contract refactored
    • split into modules
    • tests and deployment scripts updated to the new modular structure
  • Refactored stakeBySchedule
  • Restored function Staking.stakeWithApproval
    Note: original Staking contract and all the inherited ones are left for review/comparison at the moment and will be removed except for IStaking which is adapted to the new structure
  • Restored require/revert messages to their full text
  • Fixed governance Team Vesting cancelling out of gas issue
  • Fixed VestingRegistry func name typo isVestingAdress -> isVestingAddress
  • Fixed issue with CI caused by one of packages
  • Fixed Cross-contracts reentrancy vulnerability in staking contracts
  • Fixed Voting Power manipulation vulnerability in staking contracts

@tjcloa tjcloa marked this pull request as ready for review July 30, 2022 00:01
@tjcloa tjcloa requested review from cwsnt and ororopickpocket July 30, 2022 00:02
Copy link
Contributor

@ororopickpocket ororopickpocket 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! The modules make sense, documentation is clear. There are only a few things I noticed.

  • The stakeBySchedule function was not refactored as expected (details in comment). I recommend refactoring this in a separate PR.
  • The error messages were shortened to error codes in the past due to the contract size issue. i would prefer for them to be restored to their full length, because it makes debugging a lot easier.

contracts/governance/Staking/Staking.sol Outdated Show resolved Hide resolved
contracts/governance/Staking/Staking.sol Outdated Show resolved Hide resolved
contracts/utils/SafeOwnable.sol Outdated Show resolved Hide resolved
@tjcloa tjcloa self-assigned this Sep 1, 2022
@tjcloa
Copy link
Contributor Author

tjcloa commented Sep 1, 2022

Looks good! The modules make sense, documentation is clear. There are only a few things I noticed.

* The stakeBySchedule function was not refactored as expected (details in comment). I recommend refactoring this in a separate PR.

* The error messages were shortened to error codes in the past due to the contract size issue. i would prefer for them to be restored to their full length, because it makes debugging a lot easier.

All done

Copy link
Contributor

@jameshowlett977 jameshowlett977 left a comment

Choose a reason for hiding this comment

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

Restored require/revert messages to their full text
I have only minor comments about error messages.

tjcloa and others added 3 commits February 13, 2023 14:19
…ainnet-deployment-scripts-tested-to-staking-contracts-eip-170

Staking mainnet deployment scripts tested on both testnet and a forked mainnet
@tjcloa tjcloa merged commit 1772e37 into development Feb 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants