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

V2 optimize #55

Merged
merged 26 commits into from
Aug 10, 2023
Merged
Changes from 19 commits
Commits
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
28 changes: 14 additions & 14 deletions gasbenchmark10mil
Original file line number Diff line number Diff line change
@@ -1,24 +1,24 @@
No files changed, compilation skipped

Running 1 test for test/GasBenchmark.t.sol:GasBenchmark
[PASS] testGas(address,bytes32) (runs: 256, μ: 14540532, ~: 14540813)
Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 289.94ms
[PASS] testGas(address,bytes32) (runs: 256, μ: 12586431, ~: 12586524)
Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 492.11ms
| src/DelegateRegistry.sol:DelegateRegistry contract | | | | | |
|----------------------------------------------------|-----------------|--------|--------|--------|---------|
| Deployment Cost | Deployment Size | | | | |
| 2124658 | 10644 | | | | |
| 1799691 | 9021 | | | | |
| Function Name | min | avg | median | max | # calls |
| checkDelegateForAll | 2980 | 3183 | 3183 | 3386 | 2 |
| checkDelegateForContract | 5473 | 5895 | 5895 | 6317 | 2 |
| checkDelegateForERC1155 | 7990 | 8685 | 8685 | 9380 | 2 |
| checkDelegateForERC20 | 7927 | 8610 | 8610 | 9293 | 2 |
| checkDelegateForERC721 | 7991 | 8644 | 8644 | 9297 | 2 |
| delegateAll | 135861 | 135861 | 135861 | 135861 | 2 |
| delegateContract | 114469 | 125419 | 125419 | 136369 | 2 |
| delegateERC1155 | 159393 | 170343 | 170343 | 181293 | 2 |
| delegateERC20 | 136942 | 147892 | 147892 | 158842 | 2 |
| delegateERC721 | 136936 | 147886 | 147886 | 158836 | 2 |
| multicall | 689696 | 689696 | 689696 | 689696 | 1 |
| checkDelegateForAll | 2841 | 3010 | 3010 | 3180 | 2 |
| checkDelegateForContract | 5270 | 5621 | 5621 | 5973 | 2 |
| checkDelegateForERC1155 | 7707 | 8280 | 8280 | 8854 | 2 |
| checkDelegateForERC20 | 7646 | 8211 | 8211 | 8777 | 2 |
| checkDelegateForERC721 | 7730 | 8277 | 8277 | 8824 | 2 |
| delegateAll | 135880 | 135880 | 135880 | 135880 | 2 |
| delegateContract | 114464 | 125414 | 125414 | 136364 | 2 |
| delegateERC1155 | 159363 | 170313 | 170313 | 181263 | 2 |
| delegateERC20 | 136903 | 147853 | 147853 | 158803 | 2 |
| delegateERC721 | 136903 | 147853 | 147853 | 158803 | 2 |
| multicall | 689608 | 689608 | 689608 | 689608 | 1 |



26 changes: 13 additions & 13 deletions hashbenchmark10mil
Original file line number Diff line number Diff line change
@@ -1,24 +1,24 @@
No files changed, compilation skipped

Running 1 test for test/HashBenchmark.t.sol:HashBenchmark
[PASS] testHashGas(address,bytes32,address,uint256,address,bytes32) (runs: 256, μ: 20187, ~: 20187)
Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 26.78ms
[PASS] testHashGas(address,bytes32,address,uint256,address,bytes32) (runs: 256, μ: 19737, ~: 19737)
Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 54.85ms
| test/HashBenchmark.t.sol:HashHarness contract | | | | | |
|-----------------------------------------------|-----------------|-----|--------|-----|---------|
| Deployment Cost | Deployment Size | | | | |
| 396030 | 2010 | | | | |
| 275918 | 1410 | | | | |
| Function Name | min | avg | median | max | # calls |
| allHash | 679 | 679 | 679 | 679 | 1 |
| allLocation | 724 | 724 | 724 | 724 | 1 |
| contractHash | 813 | 813 | 813 | 813 | 1 |
| contractLocation | 851 | 851 | 851 | 851 | 1 |
| allHash | 666 | 666 | 666 | 666 | 1 |
| allLocation | 694 | 694 | 694 | 694 | 1 |
| contractHash | 759 | 759 | 759 | 759 | 1 |
| contractLocation | 797 | 797 | 797 | 797 | 1 |
| decodeType | 371 | 371 | 371 | 371 | 1 |
| erc1155Hash | 821 | 821 | 821 | 821 | 1 |
| erc1155Location | 920 | 920 | 920 | 920 | 1 |
| erc20Hash | 792 | 792 | 792 | 792 | 1 |
| erc20Location | 831 | 831 | 831 | 831 | 1 |
| erc721Hash | 866 | 866 | 866 | 866 | 1 |
| erc721Location | 888 | 888 | 888 | 888 | 1 |
| erc1155Hash | 776 | 776 | 776 | 776 | 1 |
| erc1155Location | 875 | 875 | 875 | 875 | 1 |
| erc20Hash | 738 | 738 | 738 | 738 | 1 |
| erc20Location | 766 | 766 | 766 | 766 | 1 |
| erc721Hash | 821 | 821 | 821 | 821 | 1 |
| erc721Location | 843 | 843 | 843 | 843 | 1 |
| location | 384 | 384 | 384 | 384 | 1 |


48 changes: 37 additions & 11 deletions src/DelegateRegistry.sol
Original file line number Diff line number Diff line change
@@ -4,6 +4,7 @@ pragma solidity ^0.8.21;
import {IDelegateRegistry as IDelegateRegistry} from "./IDelegateRegistry.sol";
import {RegistryHashes as Hashes} from "./libraries/RegistryHashes.sol";
import {RegistryStorage as Storage} from "./libraries/RegistryStorage.sol";
import {RegistryOps as Ops} from "./libraries/RegistryOps.sol";

/**
* @title DelegateRegistry
@@ -133,25 +134,37 @@ contract DelegateRegistry is IDelegateRegistry {
/// @inheritdoc IDelegateRegistry
function checkDelegateForAll(address to, address from, bytes32 rights) external view override returns (bool valid) {
valid = _validateDelegation(Hashes.allLocation(from, "", to), from);
if (rights != "" && !valid) valid = _validateDelegation(Hashes.allLocation(from, rights, to), from);
if (!Ops.or(rights == "", valid)) valid = _validateDelegation(Hashes.allLocation(from, rights, to), from);
assembly ("memory-safe") {
mstore(0x00, iszero(iszero(valid)))
Copy link
Contributor

Choose a reason for hiding this comment

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

This still assumes valid is free of dirty bits, which the compiler doesn't guarantee. A valid with dirty bits will incorrectly return false under iszero even if the last 8 bits are set to zero. If we're going to use assembly here, we need to clean valid, then store it in memory for return. Compiler will do this under the hood, so if we want to skip a bit of gas on the return we need to ensure it's going to give the same result as if there were no assembly block overriding the return of valid.

return(0x00, 0x20) // Direct return. Skips Solidity's redundant copying to save gas.
}
}

/// @inheritdoc IDelegateRegistry
function checkDelegateForContract(address to, address from, address contract_, bytes32 rights) external view override returns (bool valid) {
valid = _validateDelegation(Hashes.allLocation(from, "", to), from) || _validateDelegation(Hashes.contractLocation(from, "", to, contract_), from);
if (rights != "" && !valid) {
if (!Ops.or(rights == "", valid)) {
valid = _validateDelegation(Hashes.allLocation(from, rights, to), from) || _validateDelegation(Hashes.contractLocation(from, rights, to, contract_), from);
}
assembly ("memory-safe") {
mstore(0x00, iszero(iszero(valid)))
Copy link
Contributor

Choose a reason for hiding this comment

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

See above.

return(0x00, 0x20) // Direct return. Skips Solidity's redundant copying to save gas.
}
}

/// @inheritdoc IDelegateRegistry
function checkDelegateForERC721(address to, address from, address contract_, uint256 tokenId, bytes32 rights) external view override returns (bool valid) {
valid = _validateDelegation(Hashes.allLocation(from, "", to), from) || _validateDelegation(Hashes.contractLocation(from, "", to, contract_), from)
|| _validateDelegation(Hashes.erc721Location(from, "", to, tokenId, contract_), from);
if (rights != "" && !valid) {
if (!Ops.or(rights == "", valid)) {
valid = _validateDelegation(Hashes.allLocation(from, rights, to), from) || _validateDelegation(Hashes.contractLocation(from, rights, to, contract_), from)
|| _validateDelegation(Hashes.erc721Location(from, rights, to, tokenId, contract_), from);
}
assembly ("memory-safe") {
mstore(0x00, iszero(iszero(valid)))
Copy link
Contributor

Choose a reason for hiding this comment

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

See above.

return(0x00, 0x20) // Direct return. Skips Solidity's redundant copying to save gas.
}
}

/// @inheritdoc IDelegateRegistry
@@ -160,12 +173,16 @@ contract DelegateRegistry is IDelegateRegistry {
amount = (_validateDelegation(Hashes.allLocation(from, "", to), from) || _validateDelegation(Hashes.contractLocation(from, "", to, contract_), from))
? type(uint256).max
: (_validateDelegation(location, from) ? _loadDelegationUint(location, Storage.Positions.amount) : 0);
if (rights != "" && amount != type(uint256).max) {
if (!Ops.or(rights == "", amount == type(uint256).max)) {
location = Hashes.erc20Location(from, rights, to, contract_);
uint256 rightsBalance = (
_validateDelegation(Hashes.allLocation(from, rights, to), from) || _validateDelegation(Hashes.contractLocation(from, rights, to, contract_), from)
) ? type(uint256).max : (_validateDelegation(location, from) ? _loadDelegationUint(location, Storage.Positions.amount) : 0);
amount = rightsBalance > amount ? rightsBalance : amount;
amount = Ops.max(rightsBalance, amount);
}
assembly ("memory-safe") {
mstore(0x00, amount)
return(0x00, 0x20) // Direct return. Skips Solidity's redundant copying to save gas.
}
}

@@ -175,12 +192,16 @@ contract DelegateRegistry is IDelegateRegistry {
amount = (_validateDelegation(Hashes.allLocation(from, "", to), from) || _validateDelegation(Hashes.contractLocation(from, "", to, contract_), from))
? type(uint256).max
: (_validateDelegation(location, from) ? _loadDelegationUint(location, Storage.Positions.amount) : 0);
if (rights != "" && amount != type(uint256).max) {
if (!Ops.or(rights == "", amount == type(uint256).max)) {
location = Hashes.erc1155Location(from, rights, to, tokenId, contract_);
uint256 rightsBalance = (
_validateDelegation(Hashes.allLocation(from, rights, to), from) || _validateDelegation(Hashes.contractLocation(from, rights, to, contract_), from)
) ? type(uint256).max : (_validateDelegation(location, from) ? _loadDelegationUint(location, Storage.Positions.amount) : 0);
amount = rightsBalance > amount ? rightsBalance : amount;
amount = Ops.max(rightsBalance, amount);
}
assembly ("memory-safe") {
mstore(0x00, amount)
return(0x00, 0x20) // Direct return. Skips Solidity's redundant copying to save gas.
}
}

@@ -215,7 +236,7 @@ contract DelegateRegistry is IDelegateRegistry {
for (uint256 i = 0; i < hashes.length; ++i) {
bytes32 location = Hashes.location(hashes[i]);
address from = _loadFrom(location, Storage.Positions.firstPacked);
if (from == DELEGATION_EMPTY || from == DELEGATION_REVOKED) {
if (Ops.or(from == DELEGATION_EMPTY, from == DELEGATION_REVOKED)) {
delegations_[i] = Delegation({type_: DelegationType.NONE, to: address(0), from: address(0), rights: "", amount: 0, contract_: address(0), tokenId: 0});
} else {
(, address to, address contract_) = _loadDelegationAddresses(location, Storage.Positions.firstPacked, Storage.Positions.secondPacked);
@@ -259,7 +280,7 @@ contract DelegateRegistry is IDelegateRegistry {
/// @param interfaceId The interface identifier
/// @return valid Whether the queried interface is supported
function supportsInterface(bytes4 interfaceId) external pure returns (bool) {
return interfaceId == type(IDelegateRegistry).interfaceId || interfaceId == 0x01ffc9a7;
return Ops.or(interfaceId == type(IDelegateRegistry).interfaceId, interfaceId == 0x01ffc9a7);
}

/**
@@ -384,7 +405,12 @@ contract DelegateRegistry is IDelegateRegistry {
}

/// @dev Helper function to establish whether a delegation is enabled
function _validateDelegation(bytes32 location, address from) internal view returns (bool) {
return (_loadFrom(location, Storage.Positions.firstPacked) == from && from > DELEGATION_REVOKED);
function _validateDelegation(bytes32 location, address from) internal view returns (bool result) {
uint256 loaded = uint256(uint160(_loadFrom(location, Storage.Positions.firstPacked)));
uint256 revoked = uint256(uint160(DELEGATION_REVOKED));
uint256 fromCasted = uint256(uint160(from));
assembly {
result := and(eq(fromCasted, loaded), gt(fromCasted, revoked))
}
mireynolds marked this conversation as resolved.
Show resolved Hide resolved
}
}
Loading