Tokemak / tokemak-smart-contracts-public

A public repo of Tokemak's currently deployed contracts.
66 stars 29 forks source link

Ideas on gas saving of the Staking.sol contract #10

Closed nijynot closed 2 years ago

nijynot commented 2 years ago

Here are some ideas on how it'd be possible to decrease the gas costs, and averaging the gas costs out a bit.


1. Variable packing on the user's data stored

The StakingDetails struct can be variable packed. As the max amount of TOKE is 100M, this means that the least amount of bits required that will fit with Solidity will be uint88. But, it won't be possible to fit all variables in a single slot, we default to uint128 instead. As a block.timestamp is a unix timestamp, can we can use uint32 and it'll never be a problem for the foreseeable future.

// StakingDetails with var. packing
struct StakingDetails {
  uint128 initial;
  uint128 withdrawn;
  uint128 slashed;
  uint32 started;
  uint32 scheduleIx;
  // 8 bytes unused
}

2. Cumulative values in StakingDetails struct

Instead of having each pushed element in userStakings mapping be separated, it can be cumulative instead - I think. See ERC20Votes.sol contract by OpenZeppelin as reference, or example of the gist of the implementation idea. https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC20/extensions/ERC20Votes.sol

The idea is that on every deposit, you read the previous element in userStaking's StakingDetails[] array, and then aggregate the values and push the new element (which will then be the only relevant element). The logic should still be the same and preserves the history that is needed to keep track on how many tokens are active. This is preserved because you can get the difference by checking the previous elements (length - 2/3/...) of StakingDetails[].

This also means that you won't need for loops for when requesting withdrawal. You would not need to loop over all the userStaking's StakingDetails[] - and, you also skip another loop when running withdraw which is a similar loop which exists in requestWithdrawal. Reading the same variables in a for loop on every requestWithdrawal and withdrawal is quite expensive in the long run. And if accounts compound their TOKE, then this could get out-of-hand quite fast (this applies to both contracts and users).

3. to parameter for withdraw

In the withdraw function, it'd be great if it was possible to withdraw to a specific address. Seems fairly safe to do this change. Example of the changed lines below.

function withdraw(address to, uint256 amount) external override nonReentrant whenNotPaused {
  ...
  tokeToken.safeTransfer(to, amount);
  ...
}
nijynot commented 2 years ago

Also, happy to create a draft PR or provide more sample code of these changes if it's of interest.