Implementation of TimelockedEscrow, ConditionalTokenEscrow, RefundTokenEscrow & TimelockedTokenEscrow #1262
Conversation
* Extracted behavior to a different file
* Updated docs of ConditionalTokenEscrow, RefundTokenEscrow, TimelockedEscrow and TimelockedTokenEscrow contracts
nventuro
added
feature
There was a problem hiding this comment.
Woohoo, great work @tinchoabbate, thanks!
I left a couple comments: mosts of them are test-suite related, but a couple have to do with the design and exploration on this space.
| */ | ||
| function withdrawalAllowed(address _payee) public view returns (bool) { | ||
| // solium-disable-next-line security/no-block-members | ||
| require(block.timestamp >= releaseTime); |
There was a problem hiding this comment.
Do we want to disallow payments after this time has elapsed?
There was a problem hiding this comment.
On the one hand, if we do not disallow payments (i.e. leaving things as they are), once the time has elapsed, the escrow can at least continue working as a regular TokenEscrow. The owner must bare this in mind though. Should we go for this approach, we ought to explicitly describe this behavior somewhere in the contract.
On the other hand, if we do disallow payments, the contract would become useless once all tokens are withdrawn from the escrow.
Even though not entirely convinced yet, I'd go for the second option. It may be stricter, but I find it more predictable. If the owner wants a regular TokenEscrow after the time has elapsed, he/she should go ahead and deploy a new contract.
There was a problem hiding this comment.
Having the contract be useless after the time has elapsed doesn't worry me too much right now: this is part of a bigger issue in the ecosystem, and we haven't yet taken a stance (e.g. providing self-destruct mechanisms).
I also like the second option more, though I guess it all depends on what this sort of contract would be used for. A time-locked payment doesn't make too much sense IMO without the ability to cancel, and in all cancellable scenarios I can think of, I don't see a reason why you'd want to pay after the time has expired.
* Improved withdrawalAllowed to return the condition (removed require) * Specified units of releaseTime in a comment * Improved tests readability with context blocks
* Specified units in releaseTime as a comment * Improved tests readability with context blocks
* Fixed contract formatting * Improved tests readability with describe blocks * Added tests for the state variable
🚀 Description
Further exploring different types of Escrows and possible implementations, I included four new contracts:
ConditionalTokenEscrow: similar to the existingConditionalEscrow, but capable of handling an ERC20 token instead of ether.RefundTokenEscrow: similar to the existingRefundEscrow, but capable of handling an ERC20 token instead of ether.TimelockedEscrow: an implementation of aConditionalEscrowthat prevents the withdrawal of funds that were put in escrow for a certain amount of time.TimelockedTokenEscrow: an implementation of the newConditionalTokenEscrowthat prevents the withdrawal of ERC20 tokens that were put in escrow for a certain amount of time.The implementation of tests for the new
ConditionalTokenEscrowalso led to a refactoring of the existingTokenEscrowtests.npm run lint:all:fix).