Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Should bytes calldata be replaced with bytes memory in IERC721 to prevent compilation errors? #5201

Open
Tracked by #5202
saniksin opened this issue Sep 15, 2024 · 4 comments

Comments

@saniksin
Copy link

Summary

When working with the safeTransferFrom function in the IERC721 interface, there are instances where the use of bytes calldata leads to compilation errors, especially when trying to pass empty values (e.g., safeTransferFrom(from, to, tokenId, "")). In contrast, using bytes memory resolves the issue and prevents such errors from occurring.

Detailed Description

Currently, the safeTransferFrom function in IERC721 defines the data parameter as bytes calldata, which causes issues when trying to pass empty byte arrays or when making certain calls within the implementation. In practice, this often leads to Solidity compilation errors when empty values are used, such as "", since it can't implicitly convert between string literals and bytes calldata.

Switching the argument type from calldata to memory in the IERC721 interface and its associated functions resolves the issue without causing any functional or performance drawbacks. Additionally, it aligns with the practical usage patterns seen in many ERC-721 implementations, where memory is already frequently used.

Question

Is it possible or appropriate to update the IERC721 interface to use bytes memory instead of bytes calldata for the data parameter in safeTransferFrom?

Expected Behavior

  • The IERC721 interface would be updated to use bytes memory in the safeTransferFrom function to prevent compilation errors when passing empty values or making internal calls.

Context

This issue primarily affects developers who encounter errors when trying to pass empty byte arrays in contracts implementing the ERC-721 standard. Changing to bytes memory resolves these errors and allows for smoother development, especially in cases where safeTransferFrom is called without data.

Before and After Example

Before:

function safeTransferFrom(address from, address to, uint256 tokenId, bytes calldata data) external;

After:

function safeTransferFrom(address from, address to, uint256 tokenId, bytes memory data) external;
@saniksin saniksin changed the title Title: Should bytes calldata be replaced with bytes memory in IERC721 to prevent compilation errors? Should bytes calldata be replaced with bytes memory in IERC721 to prevent compilation errors? Sep 15, 2024
@Swapnapratim
Copy link

  • When you pass "" (an empty string) to a function expecting bytes memory, Solidity allows implicit conversion between string and bytes in memory.
    This means that "" (a string literal) can be automatically converted to bytes memory without requiring an explicit cast.

  • safeTransferFrom with bytes calldata, Solidity enforces stricter type checks because calldata is used for external function calls and is an immutable region, so implicit conversions aren't allowed.

In conclusion, if you want to pass data as empty object, just cast it into a bytes type : bytes("")

Try doing this, you should not receive any compilation issue.

@Amxx
Copy link
Collaborator

Amxx commented Sep 16, 2024

Hello @saniksin

IERC721 is just an interface. It is common to declare interface using external functions and calldata. When you call a contract by casting an address using an interface (doing IERC721(someaddress).someFunction(...)), you are going to do a call with some calldata anyway.

When you implement the interface, you are free to change that:

  • making the funciton public instead of external
  • making the function arguments memory

Example

I'm failling to see how the current interface definition is causing any issues.

@Swapnapratim
Copy link

Hello @saniksin

IERC721 is just an interface. It is common to declare interface using external functions and calldata. When you call a contract by casting an address using an interface (doing IERC721(someaddress).someFunction(...)), you are going to do a call with some calldata anyway.

When you implement the interface, you are free to change that:

  • making the funciton public instead of external
  • making the function arguments memory

Example

I'm failling to see how the current interface definition is causing any issues.

@Amxx The issue is whenever you are trying to pass "" inside the data parameter of safeTransferFrom instead of bytes("") , the compilation fails. Its quite obvious because Solidity does not allow implicit conversion between string and bytes in calldata.

@saniksin
Copy link
Author

Hello @saniksin
IERC721 is just an interface. It is common to declare interface using external functions and calldata. When you call a contract by casting an address using an interface (doing IERC721(someaddress).someFunction(...)), you are going to do a call with some calldata anyway.
When you implement the interface, you are free to change that:

  • making the funciton public instead of external
  • making the function arguments memory

Example
I'm failling to see how the current interface definition is causing any issues.

@Amxx The issue is whenever you are trying to pass "" inside the data parameter of safeTransferFrom instead of bytes("") , the compilation fails. Its quite obvious because Solidity does not allow implicit conversion between string and bytes in calldata.

It even causes a compilation error with bytes(""). This is a bit confusing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants