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

refactor(ERC165Checker) replaced assembly code with staticcall() #1829

Merged
merged 8 commits into from
Sep 11, 2019

Conversation

yopereir
Copy link
Contributor

@yopereir yopereir commented Jul 20, 2019

Fixes #1761

Replaced assembly code in ERC165Checker with staticcall method

@yopereir yopereir changed the title refactor(ERC165Checker) replaced assembly code with function refactor(ERC165Checker) replaced assembly code with staticcall() Jul 20, 2019
@yopereir
Copy link
Contributor Author

yopereir commented Jul 22, 2019

6 tests are failing giving the same error :Error: Returned error: VM Exception while processing transaction: revert. The remaining 11 tests that call this function pass. Any idea what the cause is? @nventuro , @frangio

@frangio
Copy link
Contributor

frangio commented Jul 29, 2019

@yopereir The problem is that with abi.decode you're trying to decode bool out of an empty bytes array, in the cases where there wasn't a return value or the call reverted. If the bytes array is empty, we should assume it decodes as false. There is actually a bug in the EIP that I just saw, but what I said seems to be the intended behavior.

@yopereir yopereir force-pushed the refactor/replaceAssembly#1761 branch from b36d7c6 to f9066ad Compare July 29, 2019 19:04
@frangio
Copy link
Contributor

frangio commented Aug 9, 2019

Thanks @yopereir!

I'm holding off on merging this for a few more weeks, to give Solidity 0.5.10 a little bit of time to mature.

@frangio frangio added the on hold Put on hold for some reason that must be specified in a comment. label Aug 9, 2019
@yopereir
Copy link
Contributor Author

ok

@nventuro
Copy link
Contributor

nventuro commented Sep 6, 2019

Given that 0.5.11 has already come out, I think we can resume this.

@nventuro nventuro removed the on hold Put on hold for some reason that must be specified in a comment. label Sep 6, 2019
Copy link
Contributor

@frangio frangio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One last comment that I had reviewing this again. Then it's ready to be merged.

result := mload(output) // Load the result
}
(bool success, bytes memory result) = account.staticcall.gas(30000)(encodedParams);
if (result.length == 0) return (false, false);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just in case, I think we should change this to make sure we're replicating the exact same behavior as before. Otherwise abi.decode could fail.

Suggested change
if (result.length == 0) return (false, false);
if (result.length < 32) return (false, false);

@frangio frangio merged commit 5ab6b99 into OpenZeppelin:master Sep 11, 2019
@frangio
Copy link
Contributor

frangio commented Sep 11, 2019

Thanks @yopereir!

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

Successfully merging this pull request may close these issues.

Replace inline assembly with Solidity in ERC165Checker
3 participants