Possible enhancements for our contracts

[EmperorOrokuSaki]

This is a list of some contract enhancements and improvements we can consider to implement. From this list some of the ideas might be already implemented, these ideas are just in the list to be double-checked (they are mainly related to gas optimization). And finally, some of the ideas might not be actually worth it for the MVP launch, those can be discussed as well.

Gas Optimization

Storage Packing

Storage read and write operations are very costly and by making sure we get the most usage out of each storage slot, we can reduce gas costs in read and write operations.

Bad example:

// This example uses three storage slots with each slot containing 256 bytes of data. The first and third slots are not fully used.
contract BadExample {
    uint128 a;
    uint256 b;
    uint128 c;
}

Good example:

// This example only uses two slots. Packing two uint128 types together reduces storage usage.
contract GoodExample {
    uint128 a;
    uint128 c;
    uint256 b;
}

Struct Packing

The same concept that was discussed before applies to structs as well.

Minimize SLOAD and SSTORE calls

Local variable declaration: Avoiding read calls to the storage by declaring local variables to cache the value by just one read can significantly reduce gas costs in some cases.

Using memory instead of storage: Wherever possible, we should stay away from storage variable assignments. In some cases declaring the variable as a memory variable is enough. The difference between memory and storage gas costs is great.

A simple read-and-write operation on a storage variable:

1. Declaring the variable and assigning the first value to it: 20000 (first write) + 2100 (cold slot) = 22100 gas units
2. Reading the value by loading it in a function triggered by an external transaction: 2100 (cold slot) = 24200 gas units
3. Reading the value again: 100 (warm slot) = 25200 gas units

Total gas units spent: 25,200

Memory variable:

1. Declaring the variable and assigning the first value to it: 3 (static cost) + 3 (memory expansion cost, if the offset change is from 0 to one) = 6 gas units)
2. Reading the value by loading it in a function triggered by an external transaction: 3 (static cost) = 9 gas units)
3. Reading the value again: 3 (static cost) = 12 gas units

Total gas units spent: 12

Doing unchecked operations when confident about overflowing/underflowing:

Since Solidity v0.8.0 arithmetic checks have been added (which is why we removed the Counter library #129 ). In cases where we are sure neither overflowing nor underflowing happens, we can do unchecked operations to save gas.

An example from the FleekERC721 contract would be the _appIds variable. We only increment it when a new token is minted and we never decrement it. So, it is safe to do the incrementations in unchecked {} blocks.

Using calldata instead of memory

If a function parameter is never modified, it's generally cheaper to load it from calldata instead of storing it and loading it from memory.

All setToken* functions in the FleekERC721 contract can be improved this way.

In case of facing bytecode size issues, change modifiers to call local functions

Modifiers are copied everywhere they're used and because of this, the bytecode size increases. If needed, we can modify them to call a local function (with the same logic as the modifier) to reduce bytecode size.

Use _safeMint instead of _mint

The _safeMint function calls the _checkOnERC721Received function to check if the receiving address can handle ERC721 tokens or not. Contracts need to implement functions to handle ERC721 tokens and if a token is transferred to a contract that cannot handle it, the token is possibly lost forever.

Using names for our mapping keys and values

This won't have any effect on the functionality of the contract or the gas price users pay. Since the release of Solidity v0.8.19 we can set names for mapping keys and values. The names will be included in the ABI and also give the code better readability: mapping(uint256 tokenId => address verifier)

Storing IPFS hash as bytes32 values

https://ethereum.stackexchange.com/questions/17094/how-to-store-ipfs-hash-using-bytes32/17112#17112

From #253 (comment)

[zoruka]

That's an awesome discussion and I agree with all the points mentioned. Maybe we could have to look for all these improvements and implement everything that is possible before leaving to QA team.

[EmperorOrokuSaki]

That sounds like a good idea.

[EmperorOrokuSaki]

Looking at OZ's ERC721 contract, I noticed they use memory for some data parameters they handle in the _checkOnERC721Received function. That's interesting because Calldata should theoretically work there. I'm going to look into this one.