Skip to content
This repository has been archived by the owner on Jan 24, 2022. It is now read-only.

Reduce asm in Proxy.sol #1487

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Reduce asm in Proxy.sol #1487

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2a9a5b1
Reduce asm in Proxy.sol
k06a Feb 29, 2020
f34a36b
Fix revert reason format
k06a Mar 9, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 5 additions & 16 deletions packages/lib/contracts/upgradeability/Proxy.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,23 +28,12 @@ contract Proxy {
* @param implementation Address to delegate.
*/
function _delegate(address implementation) internal {
(bool success, bytes memory data) = implementation.delegatecall(msg.data);
assembly {
// Copy msg.data. We take full control of memory in this inline assembly
// block because it will not return to Solidity code. We overwrite the
// Solidity scratch pad at memory position 0.
calldatacopy(0, 0, calldatasize)

// Call the implementation.
// out and outsize are 0 because we don't know the size yet.
let result := delegatecall(gas, implementation, 0, calldatasize, 0, 0)

// Copy the returned data.
returndatacopy(0, 0, returndatasize)

switch result
// delegatecall returns 0 on error.
case 0 { revert(0, returndatasize) }
default { return(0, returndatasize) }
switch success
// delegatecall returns 0 on error.
case 0 { revert(add(data, 32), returndatasize) }
default { return(add(data, 32), returndatasize) }
Comment on lines +35 to +36
Copy link
Contributor

Choose a reason for hiding this comment

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

A small optimization, although the compiler may catch this too.

Suggested change
case 0 { revert(add(data, 32), returndatasize) }
default { return(add(data, 32), returndatasize) }
let data_start := add(data, 32)
case 0 { revert(data_start, returndatasize) }
default { return(data_start, returndatasize) }

I also considered using mload(data) instead of returndatasize but it seems to be more expensive.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried mload(data), but tests failed

Copy link
Contributor

Choose a reason for hiding this comment

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

That's interesting. Did you understand why?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@frangio not yet, will check

}
}

Expand Down