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

Replace abi.encodeWithSelector by abi.encodeCall #3693

Closed
wiasliaw opened this issue Sep 8, 2022 · 6 comments · Fixed by #4293
Closed

Replace abi.encodeWithSelector by abi.encodeCall #3693

wiasliaw opened this issue Sep 8, 2022 · 6 comments · Fixed by #4293
Labels
breaking change Changes that break backwards compatibility of the public API.

Comments

@wiasliaw
Copy link
Contributor

wiasliaw commented Sep 8, 2022

🧐 Motivation
Since 0.8.11, abi.encodeCall provide type-safe encode utility comparing with abi.encodeWithSelector.

📝 Details
Example below and detail are from DevTools Summit | Topics in Solidity - Harikrishnan Mulackal.

abi.encodeWithSelector can use with interface.<function>.selector to prevent typo error, but it doesn't provide type checking.

interface miniERC20 {
  function transfer(address to, uint256 value) external;
}

// works successfully
function transferData(uint256 to, uint256 value)
    public
    pure
    returns (bytes memory)
{
    return abi.encodeWithSelector(miniERC20.transfer.selector, to, value);
}

abi.encodeCall provide type checking during compile time.

function transferData(uint256 to, uint256 value)
    public
    pure
    returns (bytes memory)
{
    return abi.encodeCall(miniERC20.transfer, (to, value));
}

result

error[5407]: TypeError: Cannot implicitly convert component at position 0 from "uint256" to "address".
@frangio frangio added the breaking change Changes that break backwards compatibility of the public API. label Sep 8, 2022
@frangio frangio added this to the 5.0 milestone Sep 8, 2022
@frangio
Copy link
Contributor

frangio commented Sep 8, 2022

We should definitely do this, but it requires bumping the Solidity pragma and this is a breaking change, so it should be done in our next major.

@ashwinYardi
Copy link
Contributor

ashwinYardi commented May 3, 2023

Hey guys! If this is gonna be the part of v5.0 milestone , I would love to draft a PR for this :)

@0xad1onchain
Copy link

To implement this change, the pragma upgrade at min must be >= 0.8.13, since the original implementation was bugged
Ref to blogpost describing bug

@frangio
Copy link
Contributor

frangio commented May 30, 2023

@balajipachai Do you want to help with this issue?

@balajipachai
Copy link
Contributor

balajipachai commented May 31, 2023

@frangio Sure

@balajipachai
Copy link
Contributor

@frangio It is done, could you please direct me to some other open issues with deadlines, I will get those done well within the deadline.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change Changes that break backwards compatibility of the public API.
Projects
None yet
5 participants