solmate icon indicating copy to clipboard operation
solmate copied to clipboard

:zap: ERC1155: Use storage pointers on batch functions

Open joshieDo opened this issue 3 years ago • 5 comments

Description

Use storage pointers on batch functions before loops.

Checklist

Ensure you completed all of the steps below before submitting your pull request:

  • [x] Ran forge snapshot?
  • [x] Ran npm run lint?
  • [x] Ran forge test?

Pull requests with an incomplete checklist will be thrown out.

joshieDo avatar Apr 21 '22 10:04 joshieDo

why in gods name does this save gas

transmissions11 avatar Apr 21 '22 15:04 transmissions11

solidity does not optimize the access into the slot.

doesn't feel much different than caching array.length no ?

joshieDo avatar Apr 21 '22 17:04 joshieDo

its just very gross lol

transmissions11 avatar Apr 21 '22 17:04 transmissions11

but its def effective...

transmissions11 avatar Apr 21 '22 17:04 transmissions11

one could take it a step further... ~~and assuming that users approve without uint256.max which is not true~~:

ERC20 & ERC4626.

diff --git a/.gas-snapshot b/.gas-snapshot
index 80ae19b..b77e761 100644
--- a/.gas-snapshot
+++ b/.gas-snapshot
@@ -15,8 +15,8 @@ AuthTest:testSetOwnerAsOwner() (gas: 15298)
 AuthTest:testSetOwnerWithPermissiveAuthority() (gas: 127884)
 Bytes32AddressLibTest:testFillLast12Bytes() (gas: 223)
 Bytes32AddressLibTest:testFromLast20Bytes() (gas: 191)
-CREATE3Test:testDeployERC20() (gas: 852410)
-CREATE3Test:testFailDoubleDeployDifferentBytecode() (gas: 9079256848778914164)
+CREATE3Test:testDeployERC20() (gas: 846045)
+CREATE3Test:testFailDoubleDeployDifferentBytecode() (gas: 9079256848778914066)
 CREATE3Test:testFailDoubleDeploySameBytecode() (gas: 9079256848778906218)
 DSTestPlusTest:testBound() (gas: 16777)
 DSTestPlusTest:testFailBoundMinBiggerThanMax() (gas: 309)
@@ -65,26 +65,26 @@ ERC20Test:testFailPermitBadDeadline() (gas: 36924)
 ERC20Test:testFailPermitBadNonce() (gas: 36874)
 ERC20Test:testFailPermitPastDeadline() (gas: 8589)
 ERC20Test:testFailPermitReplay() (gas: 66285)
-ERC20Test:testFailTransferFromInsufficientAllowance() (gas: 80882)
-ERC20Test:testFailTransferFromInsufficientBalance() (gas: 81358)
+ERC20Test:testFailTransferFromInsufficientAllowance() (gas: 80885)
+ERC20Test:testFailTransferFromInsufficientBalance() (gas: 81286)
 ERC20Test:testFailTransferInsufficientBalance() (gas: 52806)
-ERC20Test:testInfiniteApproveTransferFrom() (gas: 89793)
+ERC20Test:testInfiniteApproveTransferFrom() (gas: 89798)
 ERC20Test:testMint() (gas: 53746)
 ERC20Test:testPermit() (gas: 63193)
 ERC20Test:testTransfer() (gas: 60272)
-ERC20Test:testTransferFrom() (gas: 83777)
-ERC4626Test:testFailDepositWithNoApproval() (gas: 13351)
-ERC4626Test:testFailDepositWithNotEnoughApproval() (gas: 86987)
+ERC20Test:testTransferFrom() (gas: 83721)
+ERC4626Test:testFailDepositWithNoApproval() (gas: 13354)
+ERC4626Test:testFailDepositWithNotEnoughApproval() (gas: 86990)
 ERC4626Test:testFailDepositZero() (gas: 7774)
-ERC4626Test:testFailMintWithNoApproval() (gas: 13296)
+ERC4626Test:testFailMintWithNoApproval() (gas: 13299)
 ERC4626Test:testFailRedeemWithNoShareAmount() (gas: 32339)
-ERC4626Test:testFailRedeemWithNotEnoughShareAmount() (gas: 203632)
+ERC4626Test:testFailRedeemWithNotEnoughShareAmount() (gas: 203562)
 ERC4626Test:testFailRedeemZero() (gas: 7961)
 ERC4626Test:testFailWithdrawWithNoUnderlyingAmount() (gas: 32292)
-ERC4626Test:testFailWithdrawWithNotEnoughUnderlyingAmount() (gas: 203615)
-ERC4626Test:testMintZero() (gas: 54598)
-ERC4626Test:testMultipleMintDepositRedeemWithdraw() (gas: 411940)
-ERC4626Test:testVaultInteractionsForSomeoneElse() (gas: 286247)
+ERC4626Test:testFailWithdrawWithNotEnoughUnderlyingAmount() (gas: 203545)
+ERC4626Test:testMintZero() (gas: 54528)
+ERC4626Test:testMultipleMintDepositRedeemWithdraw() (gas: 411716)
+ERC4626Test:testVaultInteractionsForSomeoneElse() (gas: 286135)
 ERC4626Test:testWithdrawZero() (gas: 52465)
 ERC721Test:testApprove() (gas: 78427)
 ERC721Test:testApproveAll() (gas: 31063)
@@ -207,7 +207,7 @@ SafeTransferLibTest:testTransferETH() (gas: 34637)
 SafeTransferLibTest:testTransferFromWithMissingReturn() (gas: 49153)
 SafeTransferLibTest:testTransferFromWithNonContract() (gas: 3036)
 SafeTransferLibTest:testTransferFromWithReturnsTooMuch() (gas: 49812)
-SafeTransferLibTest:testTransferFromWithStandardERC20() (gas: 47623)
+SafeTransferLibTest:testTransferFromWithStandardERC20() (gas: 47567)
 SafeTransferLibTest:testTransferWithMissingReturn() (gas: 36668)
 SafeTransferLibTest:testTransferWithNonContract() (gas: 2990)
 SafeTransferLibTest:testTransferWithReturnsTooMuch() (gas: 37093)
diff --git a/src/mixins/ERC4626.sol b/src/mixins/ERC4626.sol
index 704e0d4..2b82e75 100644
--- a/src/mixins/ERC4626.sol
+++ b/src/mixins/ERC4626.sol
@@ -78,9 +78,11 @@ abstract contract ERC4626 is ERC20 {
         shares = previewWithdraw(assets); // No need to check for rounding error, previewWithdraw rounds up.
 
         if (msg.sender != owner) {
-            uint256 allowed = allowance[owner][msg.sender]; // Saves gas for limited approvals.
+            mapping(address => uint256) storage allowanceOwner = allowance[owner];
 
-            if (allowed != type(uint256).max) allowance[owner][msg.sender] = allowed - shares;
+            uint256 allowed = allowanceOwner[msg.sender]; // Saves gas for limited approvals.
+
+            if (allowed != type(uint256).max) allowanceOwner[msg.sender] = allowed - shares;
         }
 
         beforeWithdraw(assets, shares);
@@ -98,9 +100,11 @@ abstract contract ERC4626 is ERC20 {
         address owner
     ) public virtual returns (uint256 assets) {
         if (msg.sender != owner) {
-            uint256 allowed = allowance[owner][msg.sender]; // Saves gas for limited approvals.
+            mapping(address => uint256) storage allowanceOwner = allowance[owner];
+
+            uint256 allowed = allowanceOwner[msg.sender]; // Saves gas for limited approvals.
 
-            if (allowed != type(uint256).max) allowance[owner][msg.sender] = allowed - shares;
+            if (allowed != type(uint256).max) allowanceOwner[msg.sender] = allowed - shares;
         }
 
         // Check for rounding error since we round down in previewRedeem.
diff --git a/src/tokens/ERC20.sol b/src/tokens/ERC20.sol
index 110314b..48d6dc2 100644
--- a/src/tokens/ERC20.sol
+++ b/src/tokens/ERC20.sol
@@ -92,9 +92,11 @@ abstract contract ERC20 {
         address to,
         uint256 amount
     ) public virtual returns (bool) {
-        uint256 allowed = allowance[from][msg.sender]; // Saves gas for limited approvals.
+        mapping(address => uint256) storage allowanceFrom = allowance[from];
+        
+        uint256 allowed = allowanceFrom[msg.sender]; // Saves gas for limited approvals.
 
-        if (allowed != type(uint256).max) allowance[from][msg.sender] = allowed - amount;
+        if (allowed != type(uint256).max) allowanceFrom[msg.sender] = allowed - amount;
 
         balanceOf[from] -= amount;
 

I think that block of code of ERC4626 isn't being covered by the tests though.

joshieDo avatar Apr 23 '22 17:04 joshieDo