lukso-network / lsp-smart-contracts

The reference implementation for universal profiles smart contracts
https://www.npmjs.com/package/@lukso/lsp-smart-contracts
Apache License 2.0
76 stars 50 forks source link

Discussion of using `abi.encodePacked` vs `abi.encode` in LSP7 and LSP8 #195

Closed YamenMerhi closed 2 years ago

YamenMerhi commented 2 years ago

What is this issue about?

Currently, we use abi.encodePacked to send the token transfer data to the universalReceiver(..) function on the sender and receiver profiles to inform them about the transfer.

Code Snippet:

    function _notifyTokenSender(
        address from,
        address to,
        uint256 amount,
        bytes memory data
    ) internal virtual {
        if (ERC165Checker.supportsERC165Interface(from, _INTERFACEID_LSP1)) {
            bytes memory packedData = abi.encodePacked(from, to, amount, data);
            ILSP1UniversalReceiver(from).universalReceiver(_TYPEID_LSP7_TOKENSSENDER, packedData);
        }
    }

A developer from the community asked the following:

should we use abi.encode instead of abi.encodePacked ? I am not sure how it is expected to decode parameters even if we know the type ?

After testing

Turns out that using abi.encode instead of abi.encodePacked will be more expensive by a minimum of 771 gas units (Could increase significantly if optional data was sent during the transfer). Since we will inform the sender and the receiver, it will increase with a minimum of 1,542 gas units.

Slicing parameter:

While using abi.encodePacked, it's easy to get the parameters using any slicing library, as the data sent is: (address, address, uint256, bytes): (fixedLength, fixedLength, fixedLength, dynamicLength).

Since the first three parameters are fixed length we can get all the parameters: Slicing Method: Slicing(starting from, length)

Decoding

While using abi.encode, we should use abi.decode which is more expensive by ~1.5-2k gas units than Slicing.


The combination of abi.encode and abi.decode will make the transfer gas cost much higher. We should enforce using abi.encodePacked in the specs of LSP7 & LSP8 contracts.

frozeman commented 2 years ago

As abi.encodePacked is cheaper and can easily be sliced into its different pieces, as the last part is only dynamic its the preferred way for LSP7 and LSP8.

Closing this as its now documented on the spec: https://github.com/lukso-network/LIPs/blob/main/LSPs/LSP-7-DigitalAsset.md#_notifytokensender