Reducing Safe Contract Code Size #726
Conversation
Pull Request Test Coverage Report for Build 7503450091
💛 - Coveralls |
| @@ -292,7 +292,7 @@ contract Safe is | |||
| // Load threshold to avoid multiple storage loads | |||
| uint256 _threshold = threshold; | |||
| // Check that a threshold is set | |||
| require(_threshold > 0, "GS001"); | |||
| if (_threshold == 0) revertWithError("GS001"); | |||
There was a problem hiding this comment.
This is such a surprise to me that changing require to if affects the code size...
There was a problem hiding this comment.
I see now, its because of how the require was encoding the revert error.
| } else if (v > 30) { | ||
| // If v > 30 then default va (27,28) has been adjusted for eth_sign flow | ||
| // To support eth_sign and similar we adjust v and hash the messageHash with the Ethereum message prefix before applying ecrecover | ||
| currentOwner = ecrecover(keccak256(abi.encodePacked("\x19Ethereum Signed Message:\n32", dataHash)), v - 4, r, s); | ||
| currentOwner = ecrecover(keccak256(abi.encodePacked("\x19Ethereum Signed Message:\n32", dataHash)), uint8(v - 4), r, s); |
There was a problem hiding this comment.
Nice! I've also noticed that keeping uintN for N < 256 tends to generate more code.
There was a problem hiding this comment.
Learned from the best: #707 (comment)
| * @author Shebin John - @remedcu | ||
| * @notice The aim is to save gas using assembly to revert with custom error message. | ||
| */ | ||
| abstract contract ErrorMessage { |
There was a problem hiding this comment.
Personally, I'm fine with this assembly, it is fairly simple, and removing all the additional string encoding saves non-trivial amount of space.
We should probably have a discussion with @rmeissner about what our appetite for assembly in the contracts is.
There was a problem hiding this comment.
I think the benefits of the approach outweigh the cognitive overhead of assembly code
TODO:
uint256forv. (Saved around 9 bytes)ifwithrevertinstead ofrequirewhere possible. (Saved around 20 bytes)revertinSafe.soland inherited files. (Saved around1900 bytes2700 bytes)Closes #713