Feat/rename waterfall to OptimisticWithdrawalRecipient #62
Conversation
…ename vars in OWR
| seed = keccak256(abi.encodePacked(seed)); | ||
| principal = address(bytes20(seed)); | ||
|
|
||
| seed = keccak256(abi.encodePacked(seed)); | ||
| reward = address(bytes20(seed)); |
There was a problem hiding this comment.
Won't this result in principal and reward being the same address if they are deterministically generated from the same seed? And will that not mess up tests?
There was a problem hiding this comment.
they are different addresses the first seed = ... changes the seed for the second call
|
|
||
| owrFactoryModule = new OptimisticWithdrawalRecipientFactory(); | ||
|
|
||
| nonOWRRecipient = makeAddr("nonOWRRecipient"); |
There was a problem hiding this comment.
Super pointless nitpick: R is present in the acronym and Recipient, expands to nonOptimisticWithdrawalRecipientRecipient.
You can probably ignore this, its literally free text for ad address creator :)
|
|
||
| nonOWRRecipient = makeAddr("nonOWRRecipient"); | ||
| (principalRecipient, rewardRecipient) = generateTrancheRecipients(10); | ||
| threshold = ETH_STAKE; |
There was a problem hiding this comment.
Where does this constant come from? (just because its non obvious to me) And does it matter that threshold is declared as uint256 here, while we are going to have to bitpack it into uint96 when we call the factory with it?
There was a problem hiding this comment.
I see now that the arg is uint256 but that it will revert if the number is bigger than uint96, is that annoying/a good design?
There was a problem hiding this comment.
a couple of constants are declared in the OWRTestHelper contract which is inherited by the test suite
There was a problem hiding this comment.
I see now that the arg is uint256 but that it will revert if the number is bigger than uint96, is that annoying/a good design?
I'm not sure which is cheaper, doing the type check manually or the EVM doing it. Also, we are able to set a custom error if we do the check ourselves
| /// @return owr Address of new OptimisticWithdrawalRecipient clone | ||
| function createOWRecipient( | ||
| address token, | ||
| address nonOWRecipient, |
There was a problem hiding this comment.
Do we maybe want to call this recoveryAddress? We should also include in the natspec an explainer like:
"If this address is 0x0, recovery of unrelated tokens can be completed by either the principal or reward recipients. If this address is set, only this address can recover tokens (or ether) that are not the focus of this OWRecipient contract"
There was a problem hiding this comment.
Also I would probably make it the second last arg in the function, just because it feels right to specify the important ones first. And I assume if we made it the last arg we might end up with weird word sizes or something.
| event WaterfallFunds( | ||
| address[] recipients, uint256[] payouts, uint256 pullFlowFlag | ||
| event DistributeFunds( | ||
| uint256[] payouts, uint256 pullFlowFlag |
There was a problem hiding this comment.
Can the second arg here be a boolean rather than uint256?
There was a problem hiding this comment.
Or like a smaller uint? That might neccessitate the payouts array being a uint smaller than 256 as well, but I think I am overthinking things here, might not be worth it.
There was a problem hiding this comment.
events arguments take an entire word size in the evm 32 bytes, so using bool or uint256 doesn't really matter
I'd think uint256 will be cheaper cos the EVM doesn't have to do additional expansion
| } | ||
|
|
||
| /// Get waterfall tranche `i` | ||
| /// Get OWR tranche `i` | ||
| /// @dev emulates to uint256[] internal immutable tranche; | ||
| function _getTranche(uint256 i) internal pure returns (uint256) { |
There was a problem hiding this comment.
I assume this is a requirement for a CloneWithImmutableArgs design, but it seems weird to me. Its internal so I guess it can't be abused too much either, but I assume if you give anything other than 0 or 1 to this method, you'll get a nonsensical response for a tranche, yes? Is that a risk?
There was a problem hiding this comment.
I believe it's not possible to pass anything other than 0 or 1 to the method because it's an internal method.
| /// ----------------------------------------------------------------------- | ||
| /// storage - mutables | ||
| /// ----------------------------------------------------------------------- | ||
|
|
||
| /// Amount of distributed waterfall token | ||
| /// Amount of distributed OWRecipient token | ||
| /// @dev ERC20s with very large decimals may overflow & cause issues |
There was a problem hiding this comment.
Should we clarify that this contract will break beyond uint96?
| /// @dev can PUSH or PULL funds to recipients | ||
| function _waterfallFunds(uint256 pullFlowFlag) internal { | ||
| function _distributeFunds(uint256 pullFlowFlag) internal { |
There was a problem hiding this comment.
Any gas to be saved by changing the param down to boolean or uint16 or something?
Fixes #60