Refactors the code for gas optimisation. #36
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
hickscorp
reviewed
Aug 19, 2022
| @@ -81,46 +81,45 @@ contract FastFrontendFacet is AFastFacet { | |||
|
|
|||
| function detailedMember(address member) | |||
| public view returns(MemberDetails memory) { | |||
| LibFastToken.Data storage tokenStorage = LibFastToken.data(); | |||
hickscorp
reviewed
Aug 19, 2022
| }); | ||
| } | ||
|
|
||
| function detailedGovernor(address governor) | ||
| public view returns(GovernorDetails memory) { | ||
| LibFastAccess.Data storage accessStorage = LibFastAccess.data(); |
hickscorp
reviewed
Aug 19, 2022
| }); | ||
| } | ||
|
|
||
| function paginateDetailedMembers(uint256 index, uint256 perPage) | ||
| external view returns(MemberDetails[] memory, uint256) { | ||
| LibFastAccess.Data storage accessStorage = LibFastAccess.data(); |
hickscorp
reviewed
Aug 19, 2022
| values[i] = detailedMember(members[i]); | ||
| unchecked { ++i; } |
hickscorp
reviewed
Aug 19, 2022
| } | ||
| return (values, nextCursor); | ||
| } | ||
|
|
||
| function paginateDetailedGovernors(uint256 index, uint256 perPage) | ||
| external view returns(GovernorDetails[] memory, uint256) { | ||
| LibFastAccess.Data storage accessStorage = LibFastAccess.data(); |
hickscorp
reviewed
Aug 19, 2022
| values[i] = detailedGovernor(governors[i]); | ||
| unchecked { ++i; } |
hickscorp
reviewed
Aug 19, 2022
| values[i] = s.transferProofs[collection[cursor + i]]; | ||
| unchecked { ++i; } |
hickscorp
reviewed
Aug 19, 2022
| @@ -29,10 +29,9 @@ contract FastTokenFacet is AFastFacet, IERC20, IERC1404 { | |||
| // We want to make sure that either of these two is true: | |||
| // - The token doesn't have fixed supply. | |||
| // - The token has fixed supply but has no tokens yet (First and only mint). | |||
| require( | |||
hickscorp
reviewed
Aug 19, 2022
| @@ -50,11 +49,17 @@ contract FastTokenFacet is AFastFacet, IERC20, IERC1404 { | |||
| onlyIssuerMember { | |||
| LibFastToken.Data storage s = LibFastToken.data(); | |||
|
|
|||
| require(!FastTopFacet(address(this)).hasFixedSupply(), LibConstants.REQUIRES_CONTINUOUS_SUPPLY); | |||
hickscorp
reviewed
Aug 19, 2022
| @@ -50,11 +49,17 @@ contract FastTokenFacet is AFastFacet, IERC20, IERC1404 { | |||
| onlyIssuerMember { | |||
| LibFastToken.Data storage s = LibFastToken.data(); | |||
|
|
|||
| require(!FastTopFacet(address(this)).hasFixedSupply(), LibConstants.REQUIRES_CONTINUOUS_SUPPLY); | |||
| require(balanceOf(address(0)) >= amount, LibConstants.INSUFFICIENT_FUNDS); | |||
hickscorp
reviewed
Aug 19, 2022
contracts/fast/FastTokenFacet.sol
Outdated
|
|
||
| // Remove the minted amount from the zero address. | ||
| s.balances[address(0)] -= amount; | ||
| unchecked { |
There was a problem hiding this comment.
Now that I think about it, it could be that we should remove all the reverts around bound checking, so that we could not use unchecked and rely on the compiler to throw when overflowing...
There was a problem hiding this comment.
Good point, that's even better probably, I'll double check!
hickscorp
reviewed
Aug 19, 2022
| @@ -154,7 +159,9 @@ contract FastTokenFacet is AFastFacet, IERC20, IERC1404 { | |||
| // If the allowance being queried is from the zero address and the spender | |||
| // is a governor, we want to make sure that the spender has full rights over it. | |||
| if (owner == address(0)) { | |||
| require(FastAccessFacet(address(this)).isGovernor(spender), LibConstants.REQUIRES_FAST_GOVERNORSHIP); | |||
hickscorp
reviewed
Aug 19, 2022
contracts/fast/FastTokenFacet.sol
Outdated
| @@ -228,8 +235,7 @@ contract FastTokenFacet is AFastFacet, IERC20, IERC1404 { | |||
|
|
|||
| function detectTransferRestriction(address from, address to, uint256 amount) | |||
| external view override returns(uint8) { | |||
| LibFastToken.Data storage s = LibFastToken.data(); | |||
hickscorp
reviewed
Aug 19, 2022
contracts/fast/FastTokenFacet.sol
Outdated
| @@ -253,7 +259,7 @@ contract FastTokenFacet is AFastFacet, IERC20, IERC1404 { | |||
| } else if (restrictionCode == LibFastToken.REQUIRES_DIFFERENT_SENDER_AND_RECIPIENT_CODE) { | |||
| return LibConstants.REQUIRES_DIFFERENT_SENDER_AND_RECIPIENT; | |||
| } | |||
| revert(LibConstants.UNKNOWN_RESTRICTION_CODE); | |||
hickscorp
reviewed
Aug 19, 2022
hickscorp
reviewed
Aug 19, 2022
hickscorp
reviewed
Aug 19, 2022
contracts/fast/FastTokenFacet.sol
Outdated
| if(s.balances[p.from] < p.amount){ | ||
| revert ICustomErrors.InsufficientFunds(); | ||
| } | ||
| if(p.amount <= 0){ |
There was a problem hiding this comment.
This should be p.amount == 0 as it can never be less than zero - it's unsigned. Slight gas optimization.
hickscorp
reviewed
Aug 19, 2022
hickscorp
reviewed
Aug 19, 2022
contracts/fast/FastTokenFacet.sol
Outdated
|
|
||
| // If the from account isn't the zero address... | ||
| if (p.from != address(0)) { | ||
| // Decrease allowance. | ||
| uint256 newAllowance = s.allowances[p.from][p.spender] -= p.amount; |
hickscorp
reviewed
Aug 19, 2022
hickscorp
reviewed
Aug 19, 2022
contracts/fast/FastTokenFacet.sol
Outdated
| s.balances[p.from] -= p.amount; | ||
| unchecked { | ||
| s.balances[p.from] -= p.amount; | ||
| } | ||
| s.balances[p.to] += p.amount; |
hickscorp
reviewed
Aug 19, 2022
hickscorp
reviewed
Aug 19, 2022
contracts/fast/FastTokenFacet.sol
Outdated
| if(s.transferCredits < p.amount){ | ||
| revert ICustomErrors.InsufficientTransferCredits(); | ||
| } | ||
| unchecked { |
hickscorp
reviewed
Aug 19, 2022
| @@ -406,17 +415,15 @@ contract FastTokenFacet is AFastFacet, IERC20, IERC1404 { | |||
| if (candidate != address(0)) { | |||
| // FAST is semi-public - the only requirement to hold tokens is to be an marketplace member. | |||
| if (IFast(address(this)).isSemiPublic()) { | |||
| require( | |||
hickscorp
reviewed
Aug 19, 2022
| } | ||
| // FAST is private, the requirement to hold tokens is to be a member of that FAST. | ||
| else { | ||
| require( |
hickscorp
reviewed
Aug 19, 2022
| @@ -37,10 +37,11 @@ contract FastTopFacet is AFastFacet { | |||
| function setIsSemiPublic(bool flag) | |||
| external | |||
| onlyIssuerMember { | |||
| LibFast.Data storage s = LibFast.data(); | |||
There was a problem hiding this comment.
This needs a rework. For now, no cigar :)
hickscorp
reviewed
Aug 19, 2022
| @@ -23,82 +24,74 @@ abstract contract AFastFacet is IFastEvents { | |||
|
|
|||
| /// @dev Ensures that a method can only be called by another facet of the same diamond. | |||
| modifier onlyDiamondFacet() { | |||
| require( | |||
hickscorp
reviewed
Aug 19, 2022
| _; | ||
| } | ||
|
|
||
| /// @dev Ensures that a method can only be called by the owner of this diamond. | ||
| modifier onlyDiamondOwner() { | ||
| require( |
hickscorp
reviewed
Aug 19, 2022
| _; | ||
| } | ||
|
|
||
| /// @dev Ensures that a method can only be called by the singleton deployer contract factory. | ||
| modifier onlyDeployer() { | ||
| require( |
hickscorp
reviewed
Aug 19, 2022
| @@ -9,8 +9,9 @@ library LibPaginate { | |||
| internal view returns(address[] memory, uint256) { | |||
| uint256 length = (perPage > collection.length - cursor) ? collection.length - cursor : perPage; | |||
| address[] memory values = new address[](length); | |||
| for (uint256 i = 0; i < length; i++) { | |||
hickscorp
reviewed
Aug 19, 2022
| @@ -19,8 +20,9 @@ library LibPaginate { | |||
| internal view returns(uint256[] memory, uint256) { | |||
| uint256 length = (perPage > collection.length - cursor) ? collection.length - cursor : perPage; | |||
| uint256[] memory values = new uint256[](length); | |||
| for (uint256 i = 0; i < length; i++) { | |||
hickscorp
reviewed
Aug 19, 2022
| @@ -29,8 +31,9 @@ library LibPaginate { | |||
| internal view returns(LibFastHistory.SupplyProof[] memory, uint256) { | |||
| uint256 length = (perPage > collection.length - cursor) ? collection.length - cursor : perPage; | |||
| LibFastHistory.SupplyProof[] memory values = new LibFastHistory.SupplyProof[](length); | |||
| for (uint256 i = 0; i < length; i++) { | |||
hickscorp
reviewed
Aug 19, 2022
| @@ -39,8 +42,9 @@ library LibPaginate { | |||
| internal view returns(LibFastHistory.TransferProof[] memory, uint256) { | |||
| uint256 length = (perPage > collection.length - cursor) ? collection.length - cursor : perPage; | |||
| LibFastHistory.TransferProof[] memory values = new LibFastHistory.TransferProof[](length); | |||
| for (uint256 i = 0; i < length; i++) { | |||
hickscorp
reviewed
Aug 19, 2022
| @@ -70,7 +71,9 @@ contract MarketplaceAccessFacet is AMarketplaceFacet, IHasMembers, IHasActiveMem | |||
| onlyIssuerMember { | |||
| LibMarketplaceAccess.Data storage s = LibMarketplaceAccess.data(); | |||
| // Ensure that member doesn't have any FAST membership. | |||
| require(s.fastMemberships[member].values.length == 0, LibConstants.REQUIRES_NO_FAST_MEMBERSHIPS); | |||
hickscorp
reviewed
Aug 19, 2022
| @@ -93,27 +96,23 @@ contract MarketplaceAccessFacet is AMarketplaceFacet, IHasMembers, IHasActiveMem | |||
| function memberAddedToFast(address member) | |||
| external { | |||
| // Verify that the given address is in fact a registered FAST contract. | |||
| require( | |||
hickscorp
reviewed
Aug 19, 2022
| // Keep track of the member's FAST membership. | ||
| LibAddressSet.Data storage memberFasts = LibMarketplaceAccess.data().fastMemberships[member]; |
hickscorp
reviewed
Aug 19, 2022
| } | ||
|
|
||
| /** @dev Callback from FAST contracts allowing the Marketplace contract to keep track of FAST memberships. | ||
| * @param member The member for which a FAST membership has been removed. | ||
| */ | ||
| function memberRemovedFromFast(address member) | ||
| external { | ||
| require( |
hickscorp
reviewed
Aug 19, 2022
| // Remove the tracked membership. | ||
| LibAddressSet.Data storage memberFasts = LibMarketplaceAccess.data().fastMemberships[member]; |
hickscorp
reviewed
Aug 19, 2022
| @@ -132,10 +131,9 @@ contract MarketplaceAccessFacet is AMarketplaceFacet, IHasMembers, IHasActiveMem | |||
| onlyIssuerMember | |||
| onlyMember(member) { | |||
| // Guard against attempting to activate an already active member. | |||
| require( | |||
hickscorp
reviewed
Aug 19, 2022
| @@ -153,10 +151,9 @@ contract MarketplaceAccessFacet is AMarketplaceFacet, IHasMembers, IHasActiveMem | |||
| onlyIssuerMember | |||
| onlyMember(member) { | |||
| // Guard against attempting to deactivate an already deactivated member. | |||
| require( | |||
hickscorp
reviewed
Aug 19, 2022
| @@ -22,7 +22,9 @@ contract MarketplaceInitFacet is AMarketplaceFacet { | |||
| external | |||
| onlyDeployer { | |||
| // Make sure we haven't initialized yet. | |||
| require(LibMarketplace.data().version < LibMarketplace.STORAGE_VERSION, LibConstants.ALREADY_INITIALIZED); | |||
hickscorp
reviewed
Aug 19, 2022
| @@ -42,7 +44,6 @@ contract MarketplaceInitFacet is AMarketplaceFacet { | |||
| // ------------------------------------- // | |||
|
|
|||
| // Initialize access storage. | |||
| LibMarketplaceAccess.Data storage accessData = LibMarketplaceAccess.data(); | |||
hickscorp
reviewed
Aug 19, 2022
| @@ -21,31 +22,28 @@ abstract contract AMarketplaceFacet is IMarketplaceEvents { | |||
|
|
|||
| /// @dev Ensures that a method can only be called by the singleton deployer contract factory. | |||
| modifier onlyDeployer() { | |||
| require( | |||
hickscorp
reviewed
Aug 19, 2022
| _; | ||
| } | ||
|
|
||
| /** @dev Requires that the message sender is a member of the linked Issuer. | ||
| */ | ||
| modifier onlyIssuerMember() { | ||
| require( |
hickscorp
reviewed
Aug 19, 2022
| _; | ||
| } | ||
|
|
||
| /** @dev Requires that the given address is a member of the marketplace. | ||
| * @param candidate is the address to be checked. | ||
| */ | ||
| modifier onlyMember(address candidate) { | ||
| require( |
hickscorp
reviewed
Aug 19, 2022
| }, | ||
| optimizer: { | ||
| enabled: true, | ||
| runs: 1000000 |
hickscorp
reviewed
Aug 19, 2022
| MemberDetails[] memory values = new MemberDetails[](members.length); | ||
| for (uint256 i = 0; i < members.length; ++i) { | ||
| uint256 length = members.length; | ||
| for (uint256 i = 0; i < length;) { |
There was a problem hiding this comment.
for (uint256 i; i < length;) { ? using default 0 for uint256... would this be cheaper still? That's getting a bit crazy of course... might not be worth it.
There was a problem hiding this comment.
Well if it's a gas optimisation, @afmfe-iul can have a look.
Just so you know - I'm against some of them.
If we're really after gas optimisations, we shouldn't have the FastFacetHistory - which is what consumes half of the gas during transfers - and could be removed if our users were tech-savy ;)
The unchecked keyword is to be avoided, unless we have the money to hire super-soldiers in solidity.
I only use unchecked in very specific circumstances. For example consider the following:
require(balance >= amount, "Not enough funds or somethn'");
unchecked {
balance -= amount; # Doesn't crash on underflow, but we're protected by the `require`.
}So far so good. But as the complexity increases, and as multiple developers work and solve conflicts on the file, you could end up with:
require(balance >= amount, "Not enough funds or somethin");
balance -= fee;
unchecked {
balance -= amount; # Won't crash on underflow, and there could be one now.
}Or even, why not:
balance -= fee;
unchecked {
balance -= amount; # Won't crash on overflow, and no more require to guard us!
}As much as I like gas optimizations, the compiler isn't smart enough to fence the developer against these problems. So I'd rather have our users spend a bit more gas on the first snippet, or on:
balance -= fee; # Will crash on underflow, even without the `require` which was acting as documentation for misusing the API.If I'm not mistaken, solidity > 0.8 checks for overflow. In this context, lots of the require that we have in the source code add as "documentation" when someone misuses the API. So in a way, as long as we use solidity 0.8+ to compile - yes - we're consuming a bit more gas - but given how poor the compiler is at warning us, I'd rather.
There was a problem hiding this comment.
@jamesduncombe FYI I've started telling @afmfe-iul about being careful around over-optimisations. IMO most of these are over-optimisations. I agree that they are optimisations - but a bit too far on the spectrum of where I consider a codebase "safe to play with" for future developers.
If the solidity compiler reaches a point where it performs static analysis properly, and at least warns you of underflows, I'd be happy using unchecked all over the place. But until then, the risk outweighs the reward IMO - especially if we're targeting private chains / L2.
There was a problem hiding this comment.
Yep, sounds good to me... if we're happy with the cost of leaving some things under optimised it's all good. Readability + security at the cost of few extra cents makes sense 👍
hickscorp
force-pushed
the
features/gas-optimizations
branch
from
1311fbc to
59e3c62
Compare
hickscorp
force-pushed
the
features/gas-optimizations
branch
from
59e3c62 to
001d690
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Results
(all values, except "runs" column, expressed in gas)
Deployments
Function calls
Types and comments about optmisations done
Enabling the Solidity optimizer
Not much to say here, it's advisable to use the optimizer even with the Diamond pattern, but there's room for tweaking the actual value: a smaller one will lead to smaller bytecode (cheaper contract deployments) and a bigger one will lead to cheaper function calls, right now I've used a really big number for cheaper calls, since we didn't exceed the max bytecode size on any deployed contract.
Use of custom errors with revert
Solidity 0.8.4 added custom errors and after reading about it, which cost less gas and open the door for some interesting stuff like having arguments passed to the error or having better testing (soon with TypeChain!). Here are some links that explain it way better than I can:
Unchecked Math where applicable
Mainly for looping and when updating balances after a check was already done. Lots of tips taken from this post
Maybe this one is a better way of doing it, for readability.
Better caching of variables
Stopped storing some values on variables that weren't used more than once, and started storing array lengths on variables before looping.
Extra notes
LibDiamond.solun-touched but there are 14requirestatements there that we could update (the contract was written prior to 0.8.4)