Reduce staking contract size, extract Authorizations contract #1867
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This lets us save 65 bytes in contract size (23904 -> 23839)
We were not returning anything from this function and await instruction when calling it was doing nothing.
We were not returning a promise from stakeDelegate function (fixed in the previous commit) and as a result, all await instructions for stakeDelegate functions were useless. Now we are actually waiting for stakeDelegate to complete in tests and because of that I had to correct groupActiveTime in test to include the time for 2 additional awaits when setting up a test.
It's been done to save some space in TokenStaking contract. We went down from 23839 to 23663 bytes (-176) this way.
There is no reason to keep them public, we have functions allowing to access their content. This change allows to reduce TokenStaking contract size from 23663 to 23039 (-624) bytes.
This change allows to reduce TokenStaking contract size from 23039 to 22802 (-237) bytes.
StakeDelegateable is not interested in those two fields. Moving them to TokenStaking reduces TokenStaking contract size by two bytes (22802->22800).
First contants, then public uints, then internal contract references and mappings.
Some libraries are needed in TokenStaking but not in StakeDelegateable and some others are not needed at all.
The new Authorizations contract has all the operator contract authorization code and TokenStaking inherits from it. We will need the functionality from StakeDelegateable and Authorizations to implement ETH-only staking and we may not need all the other functionality from TokenStaking.
nkuba
reviewed
Jun 19, 2020
nkuba
approved these changes
Jun 19, 2020
nkuba
added a commit
to keep-network/keep-ecdsa
that referenced
this pull request
Jun 22, 2020
As TokenStaking contract has been refactored in threshold-network/keep-core#1867 we can use Authorizations and StakeDelegatable contracts calls as these contracts define the functions we use in bonding. In this commit we use separately Authorizations and StakeDelegatable instead of TokenStaking to be more flexible and able to use common code from AbstractBonding for both KEEP token and ETH bonding.
nkuba
added a commit
to keep-network/keep-ecdsa
that referenced
this pull request
Aug 11, 2020
As TokenStaking contract has been refactored in threshold-network/keep-core#1867 we can use Authorizations and StakeDelegatable contracts calls as these contracts define the functions we use in bonding. In this commit we use separately Authorizations and StakeDelegatable instead of TokenStaking to be more flexible and able to use common code from AbstractBonding for both KEEP token and ETH bonding.
nkuba
added a commit
to keep-network/keep-ecdsa
that referenced
this pull request
Aug 11, 2020
As TokenStaking contract has been refactored in threshold-network/keep-core#1867 we can use Authorizations and StakeDelegatable contracts calls as these contracts define the functions we use in bonding. In this commit we use separately Authorizations and StakeDelegatable instead of TokenStaking to be more flexible and able to use common code from AbstractBonding for both KEEP token and ETH bonding.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There were two motivations for changes in this PR: make ETH-only staking implementation easier and reduce contract size of staking contract. We were operating very close to the limit and adding any functionality to
TokenStakingwas problematic because of out-of-gas problems during deployment. In this PR we reduceTokenStakingcontract size from23904to22775(-1129). Hopefully, this will be enough to implement top-ups in the future.One unrelated change is a fix to
stakeDelegatefunction. We were not returning anything from this function andawaitinstructions were doing nothing. Now we return a promise and no tests should fail because of timing issues related to this function.The summary of changes with
TokenStakingsize diff:TokenStakingcontract:23904 -> 23839 (-65).23839 -> 23663 bytes (-176).StakeDelegateable'sownerOperatorsanoperatorsmappings made internal:23663 -> 23039 (-624).TokenStakingtokenandregistryreferences made internal:23039 -> 22802 (-237).initializationPeriodandundelegationPeriodmoved toTokenStaking:22802->22800 (-2).Additionally, cleaned up unused libraries, added KEEP ASCII logo and extracted contract authorization code to a separate contract (
Authorizations.sol). We will need the functionality fromStakeDelegateableandAuthorizationsto implement ETH-only staking and we may not need all the other functionality fromTokenStaking. This change reducedTokenStakingcontract size as well22800 -> 22775 = (-25).