Add dispatcher components #66
Conversation
nonergodic
force-pushed
the
dispatcher-components
branch
from
0cf8677 to
69ec4b2
Compare
src/TransferUtils.sol
Outdated
| /** | ||
| * Payment to the target failed. | ||
| */ |
src/TransferUtils.sol
Outdated
There was a problem hiding this comment.
The file name is awkward. I think it's good not to just pile all the code into a single util file, but having awkwardly named Utils.sol files next to a generic Utils.sol is not the answer. The answer that scales and is clean is to create a utils subdirectory and have the individual files live in there and then expose their totality through a Utils.sol in the root directory.
| /** | ||
| * Dispatch an execute function. Execute functions almost always modify contract state. | ||
| */ |
|
|
||
| function cancelOwnershipTransfer() external { | ||
| AccessControlState storage state = accessControlState(); | ||
| if (state.owner != msg.sender) |
There was a problem hiding this comment.
unconventional order comparison. Should be msg.sender != state.owner to be consistent and match convention.
|
|
||
| function senderHasAuth() view returns (Role) { | ||
| Role role = senderRole(); | ||
| if (Role.None == role) |
There was a problem hiding this comment.
unconventional order comparison. Should be role == Role.None to match expectation.
|
|
||
| function _acquireOwnership() internal { | ||
| AccessControlState storage state = accessControlState(); | ||
| if (state.pendingOwner != msg.sender) |
There was a problem hiding this comment.
Inconsistent order comparison.
| /** | ||
| * Dispatch a query function. Query functions never modify contract state. | ||
| */ | ||
| function dispatchQueryAccessControl(bytes calldata data, uint256 offset, uint8 query) view internal returns (bool, bytes memory, uint256) { |
There was a problem hiding this comment.
inconsistent use of uint256 and uint.
| /** | ||
| * Dispatch an execute function. Execute functions almost always modify contract state. | ||
| */ | ||
| function dispatchExecAccessControl(bytes calldata data, uint256 offset, uint8 command) internal returns (bool, uint256) { |
There was a problem hiding this comment.
inconsistent use of uint256 and uint. In the original code uint is used for essentially unbounded uint values, akin to size_t while uint256 puts an emphasis on the importance of the datatype (like in a wire format, or for a value that is defined to be 256 bit).
src/TransferUtils.sol
Outdated
| } | ||
|
|
||
| function _transferEth(address to, uint256 amount) { | ||
| if (amount == 0) return; |
There was a problem hiding this comment.
Was there a deliberate choice to short-circuit zero value native token transfers but allow them for ERC20s? Do we rely anywhere on an ERC20 Transfer event even for zero value transfers?
No description provided.