Zero out tokenApprovals using assembly

[cygaar]

Use assembly to zero out the _tokenApprovals[tokenId] value. A couple of units of gas saved here: https://www.diffchecker.com/W75Q27C1

[Vectorized]

All for the 3 gas. Kekw.

<inserts_optimizoor_meme_here>

[Vectorized]

Nit: should have leading underscore for private function. For consistency.

[Vectorized]

Nit: Some people prefer hexadecimal (i.e. '0x00', '0x20', '0x40') for memory addresses and memory related constants. The Yul docs use hex for the memory addresses.

[Vectorized]

I just thought of an approach that may cut down even more gas. ;)

/**
 * @dev Returns the variables required for approval check.
 */
function _getApproved(
    uint256 tokenId, 
    address from, 
    address msgSender
) private view returns (bool isApprovedOrOwner, uint256 approvedAddressSlot, address approvedAddress) {
    mapping(uint256 => address) storage tokenApprovalsPtr = _tokenApprovals;
    assembly {
        mstore(0x00, tokenId)
        mstore(0x20, tokenApprovalsPtr.slot)
        approvedAddressSlot := keccak256(0x00, 0x40)
        approvedAddress := sload(approvedAddressSlot)
        isApprovedOrOwner := or(eq(msgSender, from), eq(msgSender, approvedAddress))
    }
}

Then, we'll need the following on the _transfer and _burn functions:

(bool isApprovedOrOwner, uint256 approvedAddressSlot, address approvedAddress) = 
    _getApproved(tokenId, from, _msgSenderERC721A());

isApprovedOrOwner = isApprovedOrOwner || isApprovedForAll(from, _msgSenderERC721A());

if (!isApprovedOrOwner) revert TransferCallerNotOwnerNorApproved();
if (to == address(0)) revert TransferToZeroAddress();

_beforeTokenTransfers(from, to, tokenId, 1);

// Clear approvals from the previous owner.
assembly {
    if approvedAddress {
        sstore(approvedAddressSlot, 0)    
    }
}

We can't put if statements in the function, otherwise else the compiler won't inline it.

Also, we can copy paste the entire _transfer function into the transferFrom function.

[cygaar]

Just tested this snippet:

bool test;
address sender = _msgSenderERC721A();
assembly {
    test := eq(sender, from)
}
console.log('from      :', from);
console.log('msg sender:', sender);
console.log('Are they equal?', test);

And got this output:

from      : 0xf39fd6e51aad88f6f4ce6ab8827279cfffb92266
msg sender: 0xf39fd6e51aad88f6f4ce6ab8827279cfffb92266
Are they equal? false

[cygaar]

I'm wondering if this is caused the by the other bits in the storage slot having dirty values

[cygaar]

Looks like it is. Here's my code to mask with an address mask:

bool test;
uint256 mask = (1 << 160) - 1;
address sender = _msgSenderERC721A();
assembly {
    test := eq(and(sender, mask), and(from, mask))
}
console.log('from      :', address(uint160(uint256(uint160(from)) & mask)));
console.log('msg sender:', address(uint160(uint256(uint160(sender)) & mask)));
console.log('Are they equal?', test);

This is the result

from      : 0xf39fd6e51aad88f6f4ce6ab8827279cfffb92266
msg sender: 0xf39fd6e51aad88f6f4ce6ab8827279cfffb92266
Are they equal? true

[fulldecent]

Not worth it.

If this is really to be pursued, this should please be raised in the Solidity project as a way to improve their compiler optimization step.

[cygaar]

I'll look into raising the issue with the solidity compiler, but I'm going to merge this in for the time being since the actual change here is pretty straightforward and gives us some gas savings.

[fulldecent]

If things like this are being accepted then it would probably be in-scope of this project to rewrite everything in YUL.

FYI, you can take a YUL smart contract and paste the whole thing into Solidity with a line or two of wrapper and it will compile.

[cygaar]

Our project's goal is "to provide a fully compliant implementation of IERC721 with significant gas savings for minting multiple NFTs in a single transaction". The change here as well as our other snippets of assembly code are meant to improve gas efficiency which is in the project scope. In a perfect world we wouldn't need to write any assembly, but right now there are gains to be had from doing so.

I don't see why it's "probably in-scope of this project to rewrite everything in YUL", but if we find it greatly efficient to do so, then maybe we'll consider it.