Slither report action, IR optimizer, minor style convention edits #49
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
0xfoobar
added a commit
that referenced
this pull request
Sep 23, 2023
* index event params by vault, delegate, contract * version bump * chore: update links for new deployments in README (#29) * chore: update links for new deployments * fix: sanitize links * fix: formatting * comment spacing * fix event emission * solidity 0.8.18, named mappings * comment cleanup * comment cleanup * forge fmt * bump to solc 0.8.19 * version bump * forge fmt * gas benchmarking * add getDelegationsByVault() method * comment out all the get() tests, need to replace * remove getContractLevelDelegations and getTokenLevelDelegations * rename getDelegationsBy to getDelegationsFor * remove ContractDelegation and TokenDelegation structs * batch delegation * remove extraneous boolean * invert param order on _revokeDelegate * todo notes * get rid of unchecked, not worth the additional illegibility * 4k gas savings from storage cleanup in revokeAllDelegates * gas report for getDelegationsForVault * batch delegation test * cleaner internal variable names * todo * rewrite tests to use new enumerations, refactor into _filterHash() function * remove revokeDelegate(), revokeSelf(), and delegateVersion internal mapping * revokeAllDelegates() -> revokeAllDelegations() * test cleanup, event renaming * forge fmt * remove revokeAllDelegations() * cleanup _lookupHashes() * forge fmt * wip * tests passing with new bytes32 data param * line length 160 * smaller comments * gas optimization * Implemented delegateForBalance idea * Pushing some comments fo reference * Pushing some comments for reference * Changed to ∃ delegatedBalance + return that method * Updated formatting * Ideas for supporting tokenId with balance case * Pushing work on test Airdrop contract * forge install: murky * Minor bug fixes * Started creating end to end airdrop test * Ran forge fmt * Created foundry airdrop test for delegate case * Fixing remappings * Add remappings.txt to .gitignore * Untracking remappings.txt * Descriptive naming, empties delegate airdrop test * Modified Claim event to include claimable * Add claimAmount to calldata params * Inherit delegate logic using DelegateAirdrop * More renaming changes, added NatSpec * Implements "continuous" token balance delegation, updated tests, and airdrop example + test (#34) * Implemented delegateForBalance idea * Pushing some comments fo reference * Pushing some comments for reference * Changed to ∃ delegatedBalance + return that method * Updated formatting * Ideas for supporting tokenId with balance case * Pushing work on test Airdrop contract * forge install: murky * Minor bug fixes * Started creating end to end airdrop test * Ran forge fmt * Created foundry airdrop test for delegate case * Fixing remappings * Add remappings.txt to .gitignore * Untracking remappings.txt * Descriptive naming, empties delegate airdrop test * Modified Claim event to include claimable * Add claimAmount to calldata params * Inherit delegate logic using DelegateAirdrop * More renaming changes, added NatSpec * bump solc to 0.8.20 * rename to ERC20/721/1155 methods * bump dependencies * comment cleanup * rename DelegationRegistry to DelegateRegistry * remove openzeppelin ERC165 dependency, hardcode interface constant * remove bytecode hash, some foundry.toml benchmarking * Resolving conflicts with v2 * Resolved inline TODOs * Committing v2 modifications to v2_wip * Reducing gas by increasing gas for getDelegations * Inverted checks to optimize for ALL delegations * Rights only stored if non-default, removed structs * Collisions between types now impossible * Subdelegations added to checkDelegateForERC20 * Standardized storage positions, remove rights loop * Simplified getDelegationsFromHashes, repeated code * Subdelegation support for all tiers, natspec * Code refactor, added private function section * Removed none type, distinct delegation structs * Added supports interface test * gasreport * Keep NONE type * checkDelegateForERC721 made external * Stopped duplicate hash pushes, 10x enumeration * Removed enable, moved positions to registry * Added getDelegationHashes methods, enables retrieval for vault / delegate hash arrays up to 100k in size and increased getDelegations capacity up to 13k (via infura). Reverted to v1 ordering. * Separated get method gas tests * Subdelegations, airdrop example is now a subdelegation example, up to 30% less gas for delegate methods (#36) DelegateRegistry.sol changes Replaced OpenZeppelin enumerable sets with bytes32[] array in vault and delegate hash mappings Hashes for a particular vault / delegate are now pushed to a bytes32[] array in their hash mappings instead of adding (and removing) them from a set. This reduces gas by > 40k every time a delegation is added to the registry due to the storage writes in the enumerable set backend. Suppose that a vault / delegate accumulates 500 delegation hashes in their array, the increased computation of the getDelegationsFor methods is well within acceptable gas limits for most RPC providers. Consumable methods are unaffected as they now use the _delegations mapping only to establish whether a delegation is valid and enabled / disabled. Changed bytes to bytes32[6] in _delegations mapping Reduces > 20k gas every time a delegation is added since variable sized storage data also need to store their length. Updated delegate methods to write minimal storage and removed _setDelegationValues Delegate methods now only write data if it exists in their type and does not write rights to storage if it's the default "". _computeDelegationLocation(hash) is used to obtain the storage location for a given hash, and writeDelegation(location, StoragePositions.delegate, data); is used to write particular delegation data. For example, this saves > 80k gas for a delegateForAll call with all rights. Storage positions of data types are standardised with the new StoragePositions struct. _setDelegationValues has been removed since these changes make it redundant. Delegation type now encoded in the last byte of the hash This solves both the problem of collisions and being able to identify the type of delegation without writing its type to storage. Check methods now support subdelegations All check methods now accept a bytes32 rights parameter (previously data). If this parameter is not the default "", the check will bubble through the default case and then the rights case unless it returns true before exhausting the search (in the case of erc721, contract, all) or unless it returns type(uint256).max before exhausting the search (in the erc20, erc1155 cases). The _loadDelegationAddress(location, StoragePositions.vault) and _loadDelegationUint(location, StoragePositions.balance) methods are used to load vault and balance values directly from storage. A check on vault is now used to determine if the hash exists and is valid (since it is set to zero when disabled, and set to vault when enabled). Removed occurrences of IDelegationRegistry.[object] These objects are inherited explicitly anyway, and omitting IDelegationRegistry from these object references increases readability of the code. Created Consumable and Private methods sections Mostly a code refactor to keep the write and read sections clean. Added consumable as a section since we have distinct classes of on chain functions that can be consumed on and off chain. IDelegateRegistry.sol changes Removed NONE from DelegationType enum as it's never used. Added StoragePositions enum to standardise storage position of delegation data. Renamed DelegationInfo to Delegation struct to be used for returning an arbitrary delegation. Created BatchDelegation struct to be used for batch delegations Airdrop Example Updated to a Subdelegation Example The airdrop example now accepts sub delegations with rights = "airdrop" in addition to default delegations, configured with a acceptableRight variable in the airdrop contract. New tests Fuzz test for supportsInterface method testVultEnumerationGas tests an edge case where a vault has enabled 500 delegations. * make interface support >=0.8.13, remove comment ramblings * rename balance to amount * move erc165 function around * replace delegateForX method names with delegateX, easier to read on etherscan * Test to isolate write / consumable function gas * Formatting correction * Added multicall, removed batchDelegate (#38) Modest gas benefits, and much cleaner / simpler (smaller function, removes need for BatchDelegation struct). Also allows projects to multicall other registry functions without going through an external multicall contract. Encoding is very simple on the front end as well e.g.: ``` var batch = []; const delegation1 = delegateRegistry.methods.delegateAll([delegate1, "", true]).encodeABI(); const delegation2 = delegateRegistry.methods.delegateERC20([delegate2, contract2, 10, "", true]).encodeABI(); batch.push(delegation1); batch.push(delegation2); try { await delegateRegistry.methods.multicall(batch).send(); } catch(err) { console.log(err); } ``` * Registry harness, integration test for delegateAll * Create new registry in each _delegateAll test call * gasdiff (#39) * gasdiff * touch pr * only run on gasbenchmark * test gasdiff, add nonsense storage write * V2 gasdiff test (#41) * add nonsense storage write to boost delegateAll() gas by 20k * remove storage write * run gasreport on v2 branch * cleanup gasdiff test, port gasreport over to gasbenchmark * Sub-delegation example: IP License Check * Added mappings to registry harness * DelegateForContract integration test * delegateERC721 integration test * Merged singular integration tests into one file * Started creating unit tests for registry functions * Fixed missing hashes if delegation set false first * Loop unit tests: repeated case and amount updates * Vault storage flags, added _validDelegation method (#44) * Vault storage flags, added _validDelegation method * Added explicit internal declaration to storage flags * Unclassified type decoding, multicall unit test * Unit tests for consumables * gas optimizations (#45) * gas optimizations, rename events, add multicall error, return NONE if delegation unknown/existed * add rest of files * clean up storage flag naming * rename DelegationsForDelegate to IncomingDelegations, DelegationsForVault to OutgoingDelegations * forge fmt * rename delegate/vault -> to/from in events and function calls * rename StoragePositions * more renaming * more unchecked * gasreport * more renaming * more renaming * add nonexistent checks to gasbenchmark to highlight double-bubble issue (#46) * linearize checks (#48) * linearize checkContract, shaved 800 gas off failure case * linearize checkERC721, chop off 1k gas there * gasbenchmark * all check functions now external, no bubbling, linearization, shaved 3k gas off erc20/1155 * format * compress increment * rename test file * Slither report action, minor style convention edits (#49) * Moved internal pure to library, ext storage access (#50) * Storage compression, private f(x) > harness, tests (#51) * Storage compression, private f(x) > harness, tests * Additional shifts to clean any dirty bits * fmt * INTERNAL, slither comment, removed whitespace * 10-17% Gas reduction for checks, hash tests (#52) * Storage method library, reduced visual complexity (#53) * Location collision test, storage library tests * Moved HashHarness to benchmark, added more tests * Moved RegistryHarness to test, equivalence tests * style nits * Open zep to v4.9, delete slither report * Slither action, naming conventions, long digits * bump solc to 0.8.21 (#54) * correct comment * Casing nits * fix gas reports * add write permission to PR (#57) * Gasoptvect, bool cleaning rules, storage positions, readSlots, forge update (#58) Follow up to the Vectorized pr / merge into Gasoptvect Solidity compiler stack cleaning rules SolidityCleaningRulespng Source: https://docs.soliditylang.org/en/v0.8.21/internals/variable_cleanup.html Direct returns of booleans and boolean cleaning From what I can see from the solidity docs, the compiler rule for cleaning booleans on the stack is done by setting them to 1 if they contain any dirty bits (see table and reference above). The iszero opcodes used in the direct valid returns and RegistryOps should conform to that cleaning rule. There is some ambiguity as to whether that should be done in assembly blocks when accessing variables, but I didn't manage to find any documentation where cleaning to their last bit or byte, for example, is an appropriate cleaning method. They don't mention in the table that this cleaning rule is going to change, but it will be worth checking when using future compiler versions. tldr. seems fine to do this given the only obvious other method is just to not use assembly for booleans. RegistryHashes.sol The assembly blocks should be implemented according to the solidity memory model and are marked; accordingly, this will allow projects to use these libraries without the downside of globally disabled compiler memory optimisation at a very slight gas cost to the registry. Other important changes Replaced Positions enum in RegistryStorage with POSITIONS_[name] uint256 constants This means we don't have to navigate the correct cleaning (reverting flow) of enums on the stack in the assembly blocks. We were directly using enums in assembly blocks before without cleaning them which was incorrect. uint256 means we don't need to clean at all as the type spans 32 bytes. All the functions that had the positions enum as a parameter now expect a uint256 instead. _writeDelegationAddresses and _loadDelegationAddresses no longer expects positions as a parameter and the packed position constants are hard coded into the function. _loadFrom no longer expects the first packed position as a parameter and the first packed constant is now hard codeded into the function. Tests have been updated to reflect this change as the positions enum was used in several places. readSlots Now has a simplified implementation where assembly is only used for the sload operation. forge update Updated foundry submodule and OpenZeppelin submodule to their latest v4.9.3 release * Payable delegate funcs for possible tip functionality (#59) * make delegate funcs payable * add slither ignore, add singlesig prototype * add slither detector ignore to sweep * two step ownership transfer * error code cleanup * add receive and fallback funcs to singlesig * use revert strings for broad version and chain explorer compat * add test * remove slither error ignore * gasreport * forge fmt * remove delegatecall from singlesig * forge update * Removed enable for fungibles, preserve storage. (#60) Removed enable for fungibles, control of revoking / delegating a fungible is now done entirely through amount; corresponding events and functions updated with enable removed, interface also updated. We are still emitting delegation events regardless of whether the state was actually changed. Delegate functions updated to preserve storage. Before: everything but the from storage flag was deleted. Now: Once a delegation has been created once, it is only possible to toggle the from storage flag and change the amount for fungibles, the rest of the storage is preserved. loadedFrom cached given its extended use in limiting the amount of storage interactions on calling one of the delegate methods. _updateFrom storage helper added to assist with the updates of from whilst preserving the rest of the packed slot. _validateDelegation replaced with _validateFrom since the check on from > address(1) is redundant since DELEGATION_EMPTY = address(0) and DELEGATION_REVOKED = address(1) cannot reasonably write to the registry. Removed redundant checks on validateDelegation in checkDelegateForERC20 and checkDelegateForERC1155 since amount will now always return zero if the fungible delegations are revoked. Added _invalidFrom helper function to reduce repeated code of checking if a from input is DELEGATION_REVOKED or DELEGATION_EMPTY. Moved DELEGATION_EMPTY and DELEGATION_REVOKED flags to RegistryStorage as projects might want to use these when performing manual storage read operations on the registry. Removed CLEAN_LAST12_BYTES_ADDRESS, not actually used anywhere. Updated tests and harness to reflect change in how the fungibles are enabled / disabled. * Skip bubble up in checks for invalidFrom to avoid confusion. (#61) Skips bubble up for _invalidFrom in the checks to avoid confusion with address(0) and / or address(1) having 'delegated' things, and also to be consistent with their omission in the enumeration functions. * wip (#62) * submodule update * C4 audit fixes and mainnet deployment (#63) C4 audit fixes and mainnet deployment to 0x00000000000000447e69651d841bD8D104Bed493 --------- Co-authored-by: lambda-0x <[email protected]> Co-authored-by: Michael Reynolds <[email protected]>
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.
Slither GitHub action:
Excellent static analyser from Trail of Bits, have it set to run on src folder only (not on test or script) and to fail if medium severity issues are detected.
IR optimizer:
Really don't think it's worth ever turning this on since the gas & deployment benefits are microscopic and do not outweigh the potential for bugs.
Minor naming convention edits:
Picked up by Slither actually, convention is for no leading _ for state vars unless private.
Cute cat photo: