I'm not totally sure why some function cost has increased, but please note that ownerOf and transferFrom are mostly used internally, so the increase of cost for them probably relevant only for those cases and doesn't affect the overall cost of the external functions.
Gas saved: up to 4K of gas units for each of the following functions:
fillOrder
exercise
withdraw
The idea here is just to use the original order instead of cloning it.
Flip the 'isLong' field and then flip it back after it has been hashed.
Since the hashOrder function doesn't modify the order, it's safe to pass the original order to it.
I've noticed also some change in the gas usage of internal functions (e.g. onERC721Received increased by 8 units, cancel which is used internally in acceptCounterOffer decreased by ~50). I'm not sure why is that.
function hashOppositeOrder(Order memory order) public view returns (bytes32 orderHash) {
- // use decode/encode to get a copy instead of reference
- Order memory oppositeOrder = abi.decode(abi.encode(order), (Order));
-
- // get the opposite side of the order (short/long)
- oppositeOrder.isLong = !order.isLong;
- orderHash = hashOrder(oppositeOrder);
+ // reverse `isLong` to create the opposite order
+ order.isLong = !order.isLong;
+ orderHash = hashOrder(order);
+ // reverse it back to original state
+ order.isLong = !order.isLong;
}
If for any reason the sponsor isn't comfortable with temporarily-changing the original order, the following optimization can be used instead. It's a bit more expensive but still a huge optimization:
(notice that the arrays aren't cloned, but referenced)
function hashOppositeOrder(Order memory order) public view returns (bytes32 orderHash) {
// get a copy instead of reference
Order memory oppositeOrder = Order(
order.maker,
order.isCall,
!order.isLong, // get the opposite side of the order (short/long)
order.baseAsset,
order.strike,
order.premium,
order.duration,
order.expiration,
order.nonce,
order.whitelist,
order.floorTokens,
order.erc20Assets,
order.erc721Assets
);
orderHash = hashOrder(oppositeOrder);
}
Gas saved: up to 200 gas units at fillOrder, and up to 60 at withdraw.
In order to save condition checks, it's better to use if-else instead of multiple if-s.
At the fillOrder function, I've also 'compressed' the premium-transfer and asset-transfer to one block in order to save an additional if-else.
if (order.isLong) {
ERC20(order.baseAsset).safeTransferFrom(order.maker, msg.sender, order.premium);
+
+ // filling long call: transfer assets from taker to contract
+ if (order.isCall) {
+ _transferERC20sIn(order.erc20Assets, msg.sender);
+ _transferERC721sIn(order.erc721Assets, msg.sender);
+ _transferFloorsIn(order.floorTokens, floorAssetTokenIds, msg.sender);
+ return positionId;
+ }
+ // filling long put: transfer strike from taker to contract
+ else {
+ // handle the case where the taker uses native ETH instead of WETH to deposit the strike
+ if (weth == order.baseAsset && msg.value > 0) {
+ // check enough ETH was sent to cover the strike
+ require(msg.value == order.strike, "Incorrect ETH amount sent");
+
+ // convert ETH to WETH
+ // we convert the strike ETH to WETH so that the logic in exercise() works
+ // - because exercise() assumes an ERC20 interface on the base asset.
+ IWETH(weth).deposit{value: msg.value}();
+ } else {
+ ERC20(order.baseAsset).safeTransferFrom(msg.sender, address(this), order.strike);
+ }
+
+ return positionId;
+ }
} else {
// handle the case where the user uses native ETH instead of WETH to pay the premium
if (weth == order.baseAsset && msg.value > 0) {
@@ -337,45 +362,18 @@ contract PuttyV2 is PuttyV2Nft, EIP712("Putty", "2.0"), ERC721TokenReceiver, Own
} else {
ERC20(order.baseAsset).safeTransferFrom(msg.sender, order.maker, order.premium);
}
- }
- // filling short put: transfer strike from maker to contract
- if (!order.isLong && !order.isCall) {
- ERC20(order.baseAsset).safeTransferFrom(order.maker, address(this), order.strike);
- return positionId;
- }
-
- // filling long put: transfer strike from taker to contract
- if (order.isLong && !order.isCall) {
- // handle the case where the taker uses native ETH instead of WETH to deposit the strike
- if (weth == order.baseAsset && msg.value > 0) {
- // check enough ETH was sent to cover the strike
- require(msg.value == order.strike, "Incorrect ETH amount sent");
-
- // convert ETH to WETH
- // we convert the strike ETH to WETH so that the logic in exercise() works
- // - because exercise() assumes an ERC20 interface on the base asset.
- IWETH(weth).deposit{value: msg.value}();
- } else {
- ERC20(order.baseAsset).safeTransferFrom(msg.sender, address(this), order.strike);
+ // filling short call: transfer assets from maker to contract
+ if (order.isCall) {
+ _transferERC20sIn(order.erc20Assets, order.maker);
+ _transferERC721sIn(order.erc721Assets, order.maker);
+ return positionId;
+ }
+ // filling short put: transfer strike from maker to contract
+ else {
+ ERC20(order.baseAsset).safeTransferFrom(order.maker, address(this), order.strike);
+ return positionId;
}
-
- return positionId;
- }
-
- // filling short call: transfer assets from maker to contract
- if (!order.isLong && order.isCall) {
- _transferERC20sIn(order.erc20Assets, order.maker);
- _transferERC721sIn(order.erc721Assets, order.maker);
- return positionId;
- }
-
- // filling long call: transfer assets from taker to contract
- if (order.isLong && order.isCall) {
- _transferERC20sIn(order.erc20Assets, msg.sender);
- _transferERC721sIn(order.erc721Assets, msg.sender);
- _transferFloorsIn(order.floorTokens, floorAssetTokenIds, msg.sender);
- return positionId;
}
}
@@ -504,9 +502,8 @@ contract PuttyV2 is PuttyV2Nft, EIP712("Putty", "2.0"), ERC721TokenReceiver, Own
return;
}
// transfer assets from putty to owner if put is exercised or call is expired
- if ((order.isCall && !isExercised) || (!order.isCall && isExercised)) {
+ else {
_transferERC20sOut(order.erc20Assets);
_transferERC721sOut(order.erc721Assets);
Having less public functions can reduce the deployment size + save some gas on other functions (Less function hashes to check against at the beginning )
I think the following functions don't have a use as public functions.
hashOrder is used by one of the test (and may be also beneficial as public to the users), so I haven't internalized it.
- function isWhitelisted(address[] memory whitelist, address target) public pure returns (bool) {
+ function isWhitelisted(address[] memory whitelist, address target) internal pure returns (bool) {
@@ -722,7 +722,7 @@ contract PuttyV2 is PuttyV2Nft, EIP712("Putty", "2.0"), ERC721TokenReceiver, Own
- function encodeERC20Assets(ERC20Asset[] memory arr) public pure returns (bytes memory encoded) {
+ function encodeERC20Assets(ERC20Asset[] memory arr) internal pure returns (bytes memory encoded) {
@@ -736,7 +736,7 @@ contract PuttyV2 is PuttyV2Nft, EIP712("Putty", "2.0"), ERC721TokenReceiver, Own
- function encodeERC721Assets(ERC721Asset[] memory arr) public pure returns (bytes memory encoded) {
+ function encodeERC721Assets(ERC721Asset[] memory arr) internal pure returns (bytes memory encoded) {
Gas-report diff:
This reduces about 20 units per function, but has increased the cost of ownerOf(by 1), setBaseURI and setFee (40 units each).
Considering that the set function are rarely used - I think the optimization is worth it.
@@ -490,7 +490,7 @@ contract PuttyV2 is PuttyV2Nft, EIP712("Putty", "2.0"), ERC721TokenReceiver, Own
/* ~~~ INTERACTIONS ~~~ */
// transfer strike to owner if put is expired or call is exercised
- if ((order.isCall && isExercised) || (!order.isCall && !isExercised)) {
+ if (order.isCall == isExercised) {
// send the fee to the admin/DAO if fee is greater than 0%
uint256 feeAmount = 0;
if (fee > 0) {
The first check of isExercised is much cheaper than the timestamp check. So in case isExercised is true, this is going to save some gas.
(In case that it's false it might increase the cost by ~4 units, but considering that both scenarios are about equally likely to happen, we should prefer saving gas on one scenario at the expense of a small increase of the other.)
// check long position has either been exercised or is expired
- require(block.timestamp > positionExpirations[longPositionId] || isExercised, "Must be exercised or expired");
+ require(isExercised || block.timestamp > positionExpirations[longPositionId], "Must be exercised or expired");
/* ~~~ EFFECTS ~~~ */
At the functions exercise and withdraw there's an ownership check done at the beginning, but this one is unnecessary because ownership is checked while transferring the NFT to the 0xdead address.
Removing this is going to increase gas cost for reverted calls (since the check is done later. This is probably the reason the avg for withdraw has increased), but considering that reverted calls are more rare than normal calls I think it's worth it.
This is also going to change some revert errors ('WRONG FROM' instead of 'NOT MINTED' etc.). But I guess/hope this is not a problem for the sponsor.
bytes32 orderHash = hashOrder(order);
- // check user owns the position
- require(ownerOf(uint256(orderHash)) == msg.sender, "Not owner");
-
// check position is long
require(order.isLong, "Can only exercise long positions");
@@ -468,8 +465,6 @@ contract PuttyV2 is PuttyV2Nft, EIP712("Putty", "2.0"), ERC721TokenReceiver, Own
bytes32 orderHash = hashOrder(order);
- // check msg.sender owns the position
- require(ownerOf(uint256(orderHash)) == msg.sender, "Not owner");
uint256 longPositionId = uint256(hashOppositeOrder(order));
bool isExercised = exercisedPositions[longPositionId];
The following tests are failing:
3 of them is because the order of checks have changed, and the other is because the revert msg has changed.
Failed tests:
[FAIL. Reason: Can only exercise long positions] testItCannotExercisePositionThatDoesNotExist() (gas: 30915)
[FAIL. Reason: Can only exercise long positions] testItCannotExercisePositionYouDoNotOwn() (gas: 160124)
[FAIL. Reason: WRONG_FROM] testItCannotWithdrawPositionThatDoesNotExist() (gas: 37344)
[FAIL. Reason: Must be exercised or expired] testItCannotWithdrawPositionThatYouDontOwn() (gas: 160961)
Replace string error msgs with custom errors
This is another common one, all of the requires can be replaced with an if-revert with a custom error.
That's going to save some deployment size and some gas on reverted calls.
Summary
Optimizations:
Detailed gas comparison before and after
Comparison summary:
Use the same order instead of cloning
Lines: PuttyV2.sol#L685-L689
Gas saved: up to 4K of gas units for each of the following functions:
The idea here is just to use the original order instead of cloning it. Flip the 'isLong' field and then flip it back after it has been hashed. Since the hashOrder function doesn't modify the order, it's safe to pass the original order to it.
I've noticed also some change in the gas usage of internal functions (e.g. onERC721Received increased by 8 units, cancel which is used internally in acceptCounterOffer decreased by ~50). I'm not sure why is that.
Gas-report diff:
If for any reason the sponsor isn't comfortable with temporarily-changing the original order, the following optimization can be used instead. It's a bit more expensive but still a huge optimization:
(notice that the arrays aren't cloned, but referenced)
Using if-else instead of multiple if-s
Lines:
Gas saved: up to 200 gas units at fillOrder, and up to 60 at withdraw.
In order to save condition checks, it's better to use if-else instead of multiple if-s. At the fillOrder function, I've also 'compressed' the premium-transfer and asset-transfer to one block in order to save an additional if-else.
Gas-report diff:
Internalize unnecessary public functions
Lines:
Having less public functions can reduce the deployment size + save some gas on other functions (Less function hashes to check against at the beginning )
I think the following functions don't have a use as public functions.
hashOrder
is used by one of the test (and may be also beneficial as public to the users), so I haven't internalized it.Gas-report diff:
This reduces about 20 units per function, but has increased the cost of ownerOf(by 1), setBaseURI and setFee (40 units each). Considering that the set function are rarely used - I think the optimization is worth it.
Logical expressions
order.isCall == isExercised
is equivalent to the one it replaces, but is cheaper:Lines: PuttyV2.sol#L495
Gas saved: ~30-50 units
isExercised
is much cheaper than the timestamp check. So in caseisExercised
is true, this is going to save some gas.(In case that it's false it might increase the cost by ~4 units, but considering that both scenarios are about equally likely to happen, we should prefer saving gas on one scenario at the expense of a small increase of the other.)
Lines: PuttyV2.sol#L495
Gas saved: up to 70 units
'Compressing' a variable used only once
Gas saved: 10-13 units
Since the oppositeOrderHash variable is used only once, we can insert it into the place where it used and to save some gas.
Loop index increase + remove default init
Gas saved: Depends on arrays size, up to 400 units in tests
Lines:
This is a few common optimization:
i++
uncheckedi++
to++i
to save some extra gasRemoving duplicate ownership check
Gas saved: ~200-350 per function
Lines:
At the functions
exercise
andwithdraw
there's an ownership check done at the beginning, but this one is unnecessary because ownership is checked while transferring the NFT to the 0xdead address.Removing this is going to increase gas cost for reverted calls (since the check is done later. This is probably the reason the avg for withdraw has increased), but considering that reverted calls are more rare than normal calls I think it's worth it. This is also going to change some revert errors ('WRONG FROM' instead of 'NOT MINTED' etc.). But I guess/hope this is not a problem for the sponsor.
The following tests are failing: 3 of them is because the order of checks have changed, and the other is because the revert msg has changed.
Replace string error msgs with custom errors
This is another common one, all of the requires can be replaced with an if-revert with a custom error. That's going to save some deployment size and some gas on reverted calls.
List of requires:
PuttyV2.sol#L214 PuttyV2.sol#L241 PuttyV2.sol#L278 PuttyV2.sol#L281 PuttyV2.sol#L284 PuttyV2.sol#L287 PuttyV2.sol#L290 PuttyV2.sol#L293 PuttyV2.sol#L297 PuttyV2.sol#L298 PuttyV2.sol#L329 PuttyV2.sol#L353 PuttyV2.sol#L395 PuttyV2.sol#L398 PuttyV2.sol#L401 PuttyV2.sol#L405 PuttyV2.sol#L406 PuttyV2.sol#L429 PuttyV2.sol#L470 PuttyV2.sol#L475 PuttyV2.sol#L481 PuttyV2.sol#L527 PuttyV2.sol#L551 PuttyV2.sol#L552 PuttyV2.sol#L598 PuttyV2.sol#L599 PuttyV2.sol#L765
PuttyV2Nft.sol#L12 PuttyV2Nft.sol#L13 PuttyV2Nft.sol#L26 PuttyV2Nft.sol#L27 PuttyV2Nft.sol#L28 PuttyV2Nft.sol#L41
Detailed comparison of gas before and after
When comparing the gas report before and after doing all the changes (except the custom errors), this is the results: