Gas Optimization Bounty submission #1 (#452)

* NFT Stake optimizations (#417)

* Update Struct `Staker`

* uint256 amountStake => uint64 amountStaked
* uint256 conditionIdOflastUpdate => uint64 conditionIdOflastUpdate
* uint256 timeOfLastUpdate => uint128 timeOfLastUpdate
* Other associated changes to facilitate use of updated struct

* Remove `stakersArray` and reorder varibles

`stakersArray` does not appear to serve any purpose other than to allow
for easy off-chain retreival of all staked users.

Moving the declaration of indexArray below the non-dynamic variables
allows for more efficient variable packing.
This saves ~2K gas on the staking function.

* Remove ownership and approval check from `_stake`

The removed checks are included in all modern ERC721 implementations.
If a project removes them from their ERC721 contract,
they should override the `_stake` function and include them there.
However, in most instances, it is redundant to include them in `_stake`.

* linting

* run yarn gas

* update gas report to ensure accuracy

* Re-add stakers array

* yarn gas

---------

Co-authored-by: WhiteOakKong <92236155+WhiteOakKong@users.noreply.github.com>

Author: nkrishang

contracts/extension/Staking721.sol

@@ -17,18 +17,18 @@ abstract contract Staking721 is ReentrancyGuard, IStaking721 {
     ///@dev Address of ERC721 NFT contract -- staked tokens belong to this contract.
     address public stakingToken;
 
+    /// @dev Flag to check direct transfers of staking tokens.
+    uint8 internal isStaking = 1;
+
+    ///@dev Next staking condition Id. Tracks number of conditon updates so far.
+    uint64 private nextConditionId;
+
     ///@dev List of token-ids ever staked.
     uint256[] public indexedTokens;
 
     /// @dev List of accounts that have staked their NFTs.
     address[] public stakersArray;
 
-    /// @dev Flag to check direct transfers of staking tokens.
-    uint8 internal isStaking = 1;
-
-    ///@dev Next staking condition Id. Tracks number of conditon updates so far.
-    uint256 private nextConditionId;
-
     ///@dev Mapping from token-id to whether it is indexed or not.
     mapping(uint256 => bool) public isIndexed;
 
@@ -175,7 +175,7 @@ abstract contract Staking721 is ReentrancyGuard, IStaking721 {
 
     /// @dev Staking logic. Override to add custom logic.
     function _stake(uint256[] calldata _tokenIds) internal virtual {
-        uint256 len = _tokenIds.length;
+        uint64 len = uint64(_tokenIds.length);
         require(len != 0, "Staking 0 tokens");
 
         address _stakingToken = stakingToken;
@@ -184,17 +184,10 @@ abstract contract Staking721 is ReentrancyGuard, IStaking721 {
             _updateUnclaimedRewardsForStaker(_stakeMsgSender());
         } else {
             stakersArray.push(_stakeMsgSender());
-            stakers[_stakeMsgSender()].timeOfLastUpdate = block.timestamp;
+            stakers[_stakeMsgSender()].timeOfLastUpdate = uint128(block.timestamp);
             stakers[_stakeMsgSender()].conditionIdOflastUpdate = nextConditionId - 1;
         }
         for (uint256 i = 0; i < len; ++i) {
-            require(
-                IERC721(_stakingToken).ownerOf(_tokenIds[i]) == _stakeMsgSender() &&
-                    (IERC721(_stakingToken).getApproved(_tokenIds[i]) == address(this) ||
-                        IERC721(_stakingToken).isApprovedForAll(_stakeMsgSender(), address(this))),
-                "Not owned or approved"
-            );
-
             isStaking = 2;
             IERC721(_stakingToken).safeTransferFrom(_stakeMsgSender(), address(this), _tokenIds[i]);
             isStaking = 1;
@@ -214,7 +207,7 @@ abstract contract Staking721 is ReentrancyGuard, IStaking721 {
     /// @dev Withdraw logic. Override to add custom logic.
     function _withdraw(uint256[] calldata _tokenIds) internal virtual {
         uint256 _amountStaked = stakers[_stakeMsgSender()].amountStaked;
-        uint256 len = _tokenIds.length;
+        uint64 len = uint64(_tokenIds.length);
         require(len != 0, "Withdrawing 0 tokens");
         require(_amountStaked >= len, "Withdrawing more than staked");
 
@@ -249,7 +242,7 @@ abstract contract Staking721 is ReentrancyGuard, IStaking721 {
 
         require(rewards != 0, "No rewards");
 
-        stakers[_stakeMsgSender()].timeOfLastUpdate = block.timestamp;
+        stakers[_stakeMsgSender()].timeOfLastUpdate = uint128(block.timestamp);
         stakers[_stakeMsgSender()].unclaimedRewards = 0;
         stakers[_stakeMsgSender()].conditionIdOflastUpdate = nextConditionId - 1;
 
@@ -271,7 +264,7 @@ abstract contract Staking721 is ReentrancyGuard, IStaking721 {
     function _updateUnclaimedRewardsForStaker(address _staker) internal virtual {
         uint256 rewards = _calculateRewards(_staker);
         stakers[_staker].unclaimedRewards += rewards;
-        stakers[_staker].timeOfLastUpdate = block.timestamp;
+        stakers[_staker].timeOfLastUpdate = uint128(block.timestamp);
         stakers[_staker].conditionIdOflastUpdate = nextConditionId - 1;
     }
 

contracts/extension/Staking721Upgradeable.sol

@@ -17,18 +17,18 @@ abstract contract Staking721Upgradeable is ReentrancyGuardUpgradeable, IStaking7
     ///@dev Address of ERC721 NFT contract -- staked tokens belong to this contract.
     address public stakingToken;
 
+    /// @dev Flag to check direct transfers of staking tokens.
+    uint8 internal isStaking = 1;
+
+    ///@dev Next staking condition Id. Tracks number of conditon updates so far.
+    uint64 private nextConditionId;
+
     ///@dev List of token-ids ever staked.
     uint256[] public indexedTokens;
 
     /// @dev List of accounts that have staked their NFTs.
     address[] public stakersArray;
 
-    /// @dev Flag to check direct transfers of staking tokens.
-    uint8 internal isStaking = 1;
-
-    ///@dev Next staking condition Id. Tracks number of conditon updates so far.
-    uint256 private nextConditionId;
-
     ///@dev Mapping from token-id to whether it is indexed or not.
     mapping(uint256 => bool) public isIndexed;
 
@@ -177,7 +177,7 @@ abstract contract Staking721Upgradeable is ReentrancyGuardUpgradeable, IStaking7
 
     /// @dev Staking logic. Override to add custom logic.
     function _stake(uint256[] calldata _tokenIds) internal virtual {
-        uint256 len = _tokenIds.length;
+        uint64 len = uint64(_tokenIds.length);
         require(len != 0, "Staking 0 tokens");
 
         address _stakingToken = stakingToken;
@@ -186,17 +186,10 @@ abstract contract Staking721Upgradeable is ReentrancyGuardUpgradeable, IStaking7
             _updateUnclaimedRewardsForStaker(_stakeMsgSender());
         } else {
             stakersArray.push(_stakeMsgSender());
-            stakers[_stakeMsgSender()].timeOfLastUpdate = block.timestamp;
+            stakers[_stakeMsgSender()].timeOfLastUpdate = uint128(block.timestamp);
             stakers[_stakeMsgSender()].conditionIdOflastUpdate = nextConditionId - 1;
         }
         for (uint256 i = 0; i < len; ++i) {
-            require(
-                IERC721(_stakingToken).ownerOf(_tokenIds[i]) == _stakeMsgSender() &&
-                    (IERC721(_stakingToken).getApproved(_tokenIds[i]) == address(this) ||
-                        IERC721(_stakingToken).isApprovedForAll(_stakeMsgSender(), address(this))),
-                "Not owned or approved"
-            );
-
             isStaking = 2;
             IERC721(_stakingToken).safeTransferFrom(_stakeMsgSender(), address(this), _tokenIds[i]);
             isStaking = 1;
@@ -216,7 +209,7 @@ abstract contract Staking721Upgradeable is ReentrancyGuardUpgradeable, IStaking7
     /// @dev Withdraw logic. Override to add custom logic.
     function _withdraw(uint256[] calldata _tokenIds) internal virtual {
         uint256 _amountStaked = stakers[_stakeMsgSender()].amountStaked;
-        uint256 len = _tokenIds.length;
+        uint64 len = uint64(_tokenIds.length);
         require(len != 0, "Withdrawing 0 tokens");
         require(_amountStaked >= len, "Withdrawing more than staked");
 
@@ -251,7 +244,7 @@ abstract contract Staking721Upgradeable is ReentrancyGuardUpgradeable, IStaking7
 
         require(rewards != 0, "No rewards");
 
-        stakers[_stakeMsgSender()].timeOfLastUpdate = block.timestamp;
+        stakers[_stakeMsgSender()].timeOfLastUpdate = uint128(block.timestamp);
         stakers[_stakeMsgSender()].unclaimedRewards = 0;
         stakers[_stakeMsgSender()].conditionIdOflastUpdate = nextConditionId - 1;
 
@@ -273,7 +266,7 @@ abstract contract Staking721Upgradeable is ReentrancyGuardUpgradeable, IStaking7
     function _updateUnclaimedRewardsForStaker(address _staker) internal virtual {
         uint256 rewards = _calculateRewards(_staker);
         stakers[_staker].unclaimedRewards += rewards;
-        stakers[_staker].timeOfLastUpdate = block.timestamp;
+        stakers[_staker].timeOfLastUpdate = uint128(block.timestamp);
         stakers[_staker].conditionIdOflastUpdate = nextConditionId - 1;
     }
 

contracts/extension/interface/IStaking721.sol

@@ -31,10 +31,10 @@ interface IStaking721 {
      *  @param conditionIdOflastUpdate  Condition-Id when rewards were last updated for user.
      */
     struct Staker {
-        uint256 amountStaked;
-        uint256 timeOfLastUpdate;
+        uint64 amountStaked;
+        uint64 conditionIdOflastUpdate;
+        uint128 timeOfLastUpdate;
         uint256 unclaimedRewards;
-        uint256 conditionIdOflastUpdate;
     }
 
     /**

gasreport.txt

@@ -1,98 +1,98 @@
 Compiling 70 files with 0.8.12
-Solc 0.8.12 finished in 136.28s
+Solc 0.8.12 finished in 133.05s
 Compiler run successful
 
 Running 3 tests for src/test/benchmark/EditionStakeBenchmark.t.sol:EditionStakeBenchmarkTest
 [PASS] test_benchmark_editionStake_claimRewards() (gas: 69252)
 [PASS] test_benchmark_editionStake_stake() (gas: 214287)
 [PASS] test_benchmark_editionStake_withdraw() (gas: 49469)
-Test result: ok. 3 passed; 0 failed; finished in 399.92ms
+Test result: ok. 3 passed; 0 failed; finished in 375.95ms
 
 Running 2 tests for src/test/benchmark/MultiwrapBenchmark.t.sol:MultiwrapBenchmarkTest
 [PASS] test_benchmark_multiwrap_unwrap() (gas: 92404)
 [PASS] test_benchmark_multiwrap_wrap() (gas: 475314)
-Test result: ok. 2 passed; 0 failed; finished in 399.85ms
+Test result: ok. 2 passed; 0 failed; finished in 376.62ms
 
-Running 5 tests for src/test/benchmark/SignatureDropBenchmark.t.sol:SignatureDropBenchmarkTest
-[PASS] test_bechmark_signatureDrop_claim_five_tokens() (gas: 141456)
-[PASS] test_bechmark_signatureDrop_setClaimConditions() (gas: 73774)
-[PASS] test_benchmark_signatureDrop_lazyMint() (gas: 124639)
-[PASS] test_benchmark_signatureDrop_lazyMint_for_delayed_reveal() (gas: 226228)
-[PASS] test_benchmark_signatureDrop_reveal() (gas: 9183)
-Test result: ok. 5 passed; 0 failed; finished in 402.05ms
+Running 3 tests for src/test/benchmark/PackBenchmark.t.sol:PackBenchmarkTest
+[PASS] test_benchmark_pack_addPackContents() (gas: 219449)
+[PASS] test_benchmark_pack_createPack() (gas: 1425232)
+[PASS] test_benchmark_pack_openPack() (gas: 170278)
+Test result: ok. 3 passed; 0 failed; finished in 378.83ms
 
 Running 4 tests for src/test/benchmark/TokenERC1155Benchmark.t.sol:TokenERC1155BenchmarkTest
 [PASS] test_benchmark_tokenERC1155_burn() (gas: 5744)
 [PASS] test_benchmark_tokenERC1155_mintTo() (gas: 121028)
 [PASS] test_benchmark_tokenERC1155_mintWithSignature_pay_with_ERC20() (gas: 263757)
 [PASS] test_benchmark_tokenERC1155_mintWithSignature_pay_with_native_token() (gas: 292255)
-Test result: ok. 4 passed; 0 failed; finished in 411.39ms
+Test result: ok. 4 passed; 0 failed; finished in 377.39ms
+
+Running 5 tests for src/test/benchmark/SignatureDropBenchmark.t.sol:SignatureDropBenchmarkTest
+[PASS] test_bechmark_signatureDrop_claim_five_tokens() (gas: 141456)
+[PASS] test_bechmark_signatureDrop_setClaimConditions() (gas: 73774)
+[PASS] test_benchmark_signatureDrop_lazyMint() (gas: 124639)
+[PASS] test_benchmark_signatureDrop_lazyMint_for_delayed_reveal() (gas: 226228)
+[PASS] test_benchmark_signatureDrop_reveal() (gas: 9183)
+Test result: ok. 5 passed; 0 failed; finished in 378.21ms
 
 Running 1 test for src/test/benchmark/AirdropERC20Benchmark.t.sol:AirdropERC20BenchmarkTest
 [PASS] test_benchmark_airdropERC20_airdrop() (gas: 32173710)
-Test result: ok. 1 passed; 0 failed; finished in 416.85ms
+Test result: ok. 1 passed; 0 failed; finished in 394.74ms
 
 Running 1 test for src/test/benchmark/AirdropERC1155Benchmark.t.sol:AirdropERC1155BenchmarkTest
 [PASS] test_benchmark_airdropERC1155_airdrop() (gas: 38084693)
-Test result: ok. 1 passed; 0 failed; finished in 432.26ms
+Test result: ok. 1 passed; 0 failed; finished in 398.96ms
 
 Running 1 test for src/test/benchmark/AirdropERC721Benchmark.t.sol:AirdropERC721BenchmarkTest
 [PASS] test_benchmark_airdropERC721_airdrop() (gas: 43898701)
-Test result: ok. 1 passed; 0 failed; finished in 465.71ms
+Test result: ok. 1 passed; 0 failed; finished in 416.83ms
+
+Running 3 tests for src/test/benchmark/NFTStakeBenchmark.t.sol:NFTStakeBenchmarkTest
+[PASS] test_benchmark_nftStake_claimRewards() (gas: 68826)
+[PASS] test_benchmark_nftStake_stake_five_tokens() (gas: 549421)
+[PASS] test_benchmark_nftStake_withdraw() (gas: 39648)
+Test result: ok. 3 passed; 0 failed; finished in 156.09ms
+
+Running 4 tests for src/test/benchmark/TokenERC721Benchmark.t.sol:TokenERC721BenchmarkTest
+[PASS] test_benchmark_tokenERC721_burn() (gas: 10411)
+[PASS] test_benchmark_tokenERC721_mintTo() (gas: 149977)
+[PASS] test_benchmark_tokenERC721_mintWithSignature_pay_with_ERC20() (gas: 259382)
+[PASS] test_benchmark_tokenERC721_mintWithSignature_pay_with_native_token() (gas: 283724)
+Test result: ok. 4 passed; 0 failed; finished in 160.13ms
 
 Running 3 tests for src/test/benchmark/TokenStakeBenchmark.t.sol:TokenStakeBenchmarkTest
 [PASS] test_benchmark_tokenStake_claimRewards() (gas: 75694)
 [PASS] test_benchmark_tokenStake_stake() (gas: 181796)
 [PASS] test_benchmark_tokenStake_withdraw() (gas: 55541)
-Test result: ok. 3 passed; 0 failed; finished in 182.91ms
+Test result: ok. 3 passed; 0 failed; finished in 166.98ms
 
 Running 3 tests for src/test/benchmark/TokenERC20Benchmark.t.sol:TokenERC20BenchmarkTest
 [PASS] test_benchmark_tokenERC20_mintTo() (gas: 118533)
 [PASS] test_benchmark_tokenERC20_mintWithSignature_pay_with_ERC20() (gas: 181738)
 [PASS] test_benchmark_tokenERC20_mintWithSignature_pay_with_native_token() (gas: 206150)
-Test result: ok. 3 passed; 0 failed; finished in 192.16ms
-
-Running 3 tests for src/test/benchmark/NFTStakeBenchmark.t.sol:NFTStakeBenchmarkTest
-[PASS] test_benchmark_nftStake_claimRewards() (gas: 68479)
-[PASS] test_benchmark_nftStake_stake_five_tokens() (gas: 617449)
-[PASS] test_benchmark_nftStake_withdraw() (gas: 39121)
-Test result: ok. 3 passed; 0 failed; finished in 210.22ms
-
-Running 4 tests for src/test/benchmark/TokenERC721Benchmark.t.sol:TokenERC721BenchmarkTest
-[PASS] test_benchmark_tokenERC721_burn() (gas: 10411)
-[PASS] test_benchmark_tokenERC721_mintTo() (gas: 149977)
-[PASS] test_benchmark_tokenERC721_mintWithSignature_pay_with_ERC20() (gas: 259382)
-[PASS] test_benchmark_tokenERC721_mintWithSignature_pay_with_native_token() (gas: 283724)
-Test result: ok. 4 passed; 0 failed; finished in 187.04ms
+Test result: ok. 3 passed; 0 failed; finished in 160.61ms
 
-Running 3 tests for src/test/benchmark/PackBenchmark.t.sol:PackBenchmarkTest
-[PASS] test_benchmark_pack_addPackContents() (gas: 219449)
-[PASS] test_benchmark_pack_createPack() (gas: 1425232)
-[PASS] test_benchmark_pack_openPack() (gas: 170278)
-Test result: ok. 3 passed; 0 failed; finished in 173.34ms
+Running 3 tests for src/test/benchmark/PackVRFDirectBenchmark.t.sol:PackVRFDirectBenchmarkTest
+[PASS] test_benchmark_packvrf_createPack() (gas: 1391866)
+[PASS] test_benchmark_packvrf_openPack() (gas: 119981)
+[PASS] test_benchmark_packvrf_openPackAndClaimRewards() (gas: 3621)
+Test result: ok. 3 passed; 0 failed; finished in 153.03ms
 
 Running 2 tests for src/test/benchmark/DropERC20Benchmark.t.sol:DropERC20BenchmarkTest
 [PASS] test_bechmark_dropERC20_claim() (gas: 230441)
 [PASS] test_bechmark_dropERC20_setClaimConditions_five_conditions() (gas: 501045)
-Test result: ok. 2 passed; 0 failed; finished in 765.73ms
+Test result: ok. 2 passed; 0 failed; finished in 706.23ms
 
 Running 3 tests for src/test/benchmark/DropERC1155Benchmark.t.sol:DropERC1155BenchmarkTest
 [PASS] test_bechmark_dropERC1155_claim() (gas: 186668)
 [PASS] test_bechmark_dropERC1155_setClaimConditions_five_conditions() (gas: 492331)
 [PASS] test_benchmark_dropERC1155_lazyMint() (gas: 123873)
-Test result: ok. 3 passed; 0 failed; finished in 784.83ms
-
-Running 3 tests for src/test/benchmark/PackVRFDirectBenchmark.t.sol:PackVRFDirectBenchmarkTest
-[PASS] test_benchmark_packvrf_createPack() (gas: 1391866)
-[PASS] test_benchmark_packvrf_openPack() (gas: 119981)
-[PASS] test_benchmark_packvrf_openPackAndClaimRewards() (gas: 3621)
-Test result: ok. 3 passed; 0 failed; finished in 127.97ms
+Test result: ok. 3 passed; 0 failed; finished in 713.16ms
 
 Running 5 tests for src/test/benchmark/DropERC721Benchmark.t.sol:DropERC721BenchmarkTest
 [PASS] test_bechmark_dropERC721_claim_five_tokens() (gas: 211967)
 [PASS] test_bechmark_dropERC721_setClaimConditions_five_conditions() (gas: 500859)
 [PASS] test_benchmark_dropERC721_lazyMint() (gas: 124544)
 [PASS] test_benchmark_dropERC721_lazyMint_for_delayed_reveal() (gas: 226124)
 [PASS] test_benchmark_dropERC721_reveal() (gas: 8800)
-Test result: ok. 5 passed; 0 failed; finished in 875.25ms
+Test result: ok. 5 passed; 0 failed; finished in 359.38ms
 

src/test/sdk/extension/StakingExtension.t.sol

@@ -198,7 +198,7 @@ contract StakingExtensionTest is DSTest, Test {
         _tokenIds[0] = 6;
 
         vm.prank(stakerOne);
-        vm.expectRevert("Not owned or approved");
+        vm.expectRevert("ERC721: transfer from incorrect owner");
         ext.stake(_tokenIds);
     }
 

src/test/staking/NFTStake.t.sol

@@ -151,7 +151,7 @@ contract NFTStakeTest is BaseTest {
         _tokenIds[0] = 6;
 
         vm.prank(stakerOne);
-        vm.expectRevert("Not owned or approved");
+        vm.expectRevert("ERC721: transfer from incorrect owner");
         stakeContract.stake(_tokenIds);
     }
 

src/test/staking/NFTStake_EthReward.t.sol

@@ -160,7 +160,7 @@ contract NFTStakeEthRewardTest is BaseTest {
         _tokenIds[0] = 6;
 
         vm.prank(stakerOne);
-        vm.expectRevert("Not owned or approved");
+        vm.expectRevert("ERC721: transfer from incorrect owner");
         stakeContract.stake(_tokenIds);
     }