Escrows #1014

nventuro · 2018-06-15T21:44:33Z

Fixes #901

🚀 Description

Adds an Escrow and reimplements PullPayment using it. The totalPayments variable was removed after being deemed useless
Adds a ConditionalEscrow abstract contract, and a RefundEscrow derived contract (similar to RefundVault, but the beneficiary must call withdraw after close is called instead of it being automatic)
Removes RefundVault and updates RefundableCrowdsale to use RefundEscrow

📘 I've reviewed the OpenZeppelin Contributor Guidelines
✅ I've added tests where applicable to test my new functionality.
📖 I've made sure that my contracts are well-documented.
🎨 I've run the JS/Solidity linters and fixed any issues (npm run lint:all:fix).

frangio

Awesome @nventuro! I love this "Escrow" name, it's right on the money.

Here's a first round of feedback.

frangio · 2018-06-16T16:25:44Z

contracts/crowdsale/distribution/RefundableCrowdsale.sol

@@ -3,31 +3,31 @@ pragma solidity ^0.4.24;

 import "../../math/SafeMath.sol";
 import "./FinalizableCrowdsale.sol";
-import "./utils/RefundVault.sol";
+import "../../payment/RefundEscrow.sol";


I think RefundEscrow is very much crowdsale-specific so it should be with the crowdsale stuff. Potentially in this same file. Thoughts?

Disagree: a RefundEscrow is a payment method, a crowdsale typically using it doesn't make it crowdsale-specific. As an example, platforms like Amazon or eBay also offer refunds and act as an escrow themselves, and could extend their payment systems to add support for multiple payers.

frangio · 2018-06-16T16:26:00Z

contracts/payment/PullPayment.sol

@@ -10,34 +9,34 @@ import "../math/SafeMath.sol";
 * contract and use asyncSend instead of send or transfer.
 */
 contract PullPayment {
-  using SafeMath for uint256;
+  Escrow public escrow;


Please make this state variable private.

frangio · 2018-06-16T16:26:13Z

contracts/payment/PullPayment.sol

-  function asyncSend(address dest, uint256 amount) internal {
-    payments[dest] = payments[dest].add(amount);
-    totalPayments = totalPayments.add(amount);
+  function asyncSend(address _dest, uint256 _amount) internal {


The word transfer is quite overloaded but I feel like this is an asyncTransfer more than an asyncSend, in analogy with send vs transfer because of the lack of return value.

Thoughts?

I missed this when updating PullPayment, I agree that transfer is indeed the better term.

frangio · 2018-06-16T16:26:29Z

contracts/payment/RefundEscrow.sol

+/**
+ * @title RefundEscrow
+ * @dev Escrow that holds investor funds for a unique benefitiary, and allows for
+ * either withdrawal by the benefiatiary, or refunds to the investors.


Typo twice here: "benefitiary" → "beneficiary".

Also, "unique" doesn't sound right... "single" is the right word, but it feels redundant to me anyway.

This description doesn't explain the condition according to which either withdrawal or refunding is allowed. We should also explain that this contract was written as the escrow for a RefundableCrowdsale.

Yeah, unique is just plain wrong. I think it could just say 'for a beneficiary' and the meaning would remain clear.

I'm not sure about the RefundableCrowdsale part though: supporting refunds in a payment system doesn't scream 'crowdsale' to me. I actually see the inverse of that being true: the RefundableCrowdsale is nothing more than a crowdsale, the escrow, and glue code.

It's true that RefundEscrow is not necessarily crowdsale-specific. I didn't mean to say that. I'm just interested in cross-linking related contracts. Perhaps this information should be automatically generated by our documentation generator though.

frangio · 2018-06-16T16:29:43Z

contracts/payment/PullPayment.sol

-    payee.transfer(payment);
+  /**
+  * @dev Returns the credit owed to an address.
+  & @param _dest The creditor's address.


& → *

I blame Stroustrup

frangio · 2018-06-16T16:51:52Z

contracts/payment/Escrow.sol

+  */
+  function deposit(address _payee) payable public {
+    uint256 amount = msg.value;
+    require(amount > 0);


This line doesn't add any value (ha!). It will unnecessarily propagate the special case to the users of Escrow, when we can easily treat deposit(0) as a no-op. Let's just remove the line.

deposit(0) would indeed do nothing: I thought it better to protect the caller from using the API wrong, though the argument could be made that this would make the contract more expensive for all other users. It's more of a library-wide decision, IMO. WDYT?

The thing is deposit(0) is not a wrong use of the API, it's just a no-op. It's not a matter of gas efficiency, it's about not forcing users of Escrow to add code to treat 0 specially.

We've discussed this before, though I can't remember where, and the decision was to not revert.

frangio · 2018-06-16T16:54:03Z

contracts/payment/Escrow.sol

+    uint256 payment = deposits[_payee];
+
+    require(payment != 0);
+    require(address(this).balance >= payment);


This is more of an assert than a require.

frangio · 2018-06-16T16:54:34Z

contracts/payment/Escrow.sol

+  function withdraw(address _payee) public {
+    uint256 payment = deposits[_payee];
+
+    require(payment != 0);


This feels unnecessary as well. I would remove it.

frangio · 2018-06-16T16:57:23Z

contracts/payment/RefundEscrow.sol

+  /**
+   * @dev Withdraws the beneficiary's funds.
+   */
+  function withdraw() public {


Escrow#withdraw(address) has the same name but it does something different. Let's name this one something different. beneficiaryWithdraw? I don't know.

Agreed. On a related note, withdraw and refund do the same thing, but withdraw emits no events. Should we prevent the user from calling the base function (i.e. override and revert)?

Oh, I hadn't noticed that. Hm... I think we should keep withdraw instead of adding a new refund function. In the case of RefundableCrowdsale, the user will use RefundableCrowdsale#claimRefund anyway.

frangio · 2018-06-16T16:58:37Z

contracts/payment/RefundEscrow.sol

+   */
+  function refund(address _investor) public {
+    uint256 amount = depositsOf(_investor);
+    super.withdraw(_investor);


No need to use super here.

frangio · 2018-06-18T17:54:03Z

contracts/payment/Escrow.sol

+ * @dev Base escrow contract, holds funds destinated to a payee until they
+ * withdraw them.
+ */
+contract Escrow {


We should add Deposited and Withdrawn events. (I'm open to other names.)

This would make the Refunded event a bit redundant (since it will also emit an identical Withdrawn): do we want to remove it?

frangio

A few more comments. I'd also like to do a last review of correctness and security before merging.

frangio · 2018-06-18T20:09:47Z

contracts/payment/RefundEscrow.sol

@@ -67,23 +61,23 @@ contract RefundEscrow is ConditionalEscrow, Ownable {
  /**
   * @dev Withdraws the beneficiary's funds.
   */
-  function withdraw() public {
+  function beneficiaryWithdraw() public {


I'd like to get a second/third opinion on this name. Alternative: withdrawToBeneficiary.

frangio · 2018-06-18T20:13:14Z

test/payment/Escrow.behaviour.js

@@ -6,42 +6,42 @@ require('chai')
  .use(require('chai-bignumber')(BigNumber))
  .should();

-export default function ([payer1, payer2, payee1, payee2]) {
+export function shouldBehaveLikeEscrow([payer1, payer2, payee1, payee2]) {


It can be the default export and have a name at the same time. We need a library-wide convention for this. Currently all of the behavio(u)rs use a default export so we should stick to that.

None of the other behaviors give a name to this function though. I'd leave it anonymous and default, and implement the library-wide change in a different PR.

Opened #1027

frangio · 2018-06-18T20:15:54Z

contracts/payment/RefundEscrow.sol

+  function withdraw(address _refundee) public {
+    uint256 amount = depositsOf(_refundee);
+    super.withdraw(_refundee);
+    emit Refunded(_refundee, amount);


Remove when we add events to Escrow.

frangio · 2018-06-18T20:20:28Z

test/payment/RefundEscrow.test.js

-        receipt.logs.length.should.equal(1);
-        receipt.logs[0].event.should.equal('Refunded');
-        receipt.logs[0].args.refundee.should.equal(refundee);
+        inLogs(receipt.logs, 'Refunded', { refundee });
        receipt.logs[0].args.weiAmount.should.be.bignumber.equal(amount);


Notice that the helper returns the event, so you can do

const event = expectEvent.inLogs(receipt.logs, 'Refunded', { refundee }); event.args.weiAmount.should.be.bignumber.equal(amount);

In this way you also stop relying on the event being at index 0.

Neat, I missed that bit, ty!

frangio · 2018-06-18T20:22:01Z

test/payment/RefundEscrow.test.js

@@ -1,4 +1,5 @@
 import EVMRevert from '../helpers/EVMRevert';
+import { inLogs } from '../helpers/expectEvent';


The preferred usage for better readability is:

import expectEvent from '../helpers/expectEvent'; expectEvent.inLogs(receipt.logs);

I thought that made more sense, but preferred to copy the existing code style :/

There's both styles, unfortunately.

https://github.com/OpenZeppelin/openzeppelin-solidity/blob/f18c3bc438b366f9cb3a8613f5be160c2cbced5e/test/ownership/Whitelist.test.js#L49-L53

shrugs

This is more of a "light" approval; I don't have the time to do a deep review, but the architecture is 👍and after a scan all of the code looks chill. Would prefer that @frangio follows up with his previous review before we merge :)

nventuro · 2018-06-30T02:07:23Z

After discussing offline with @frangio, we decided to restrict the API somewhat, making the base Escrow inherit from Ownable, forcing all interactions to go through an owner contract (like in the PullPayment and RefundCrowdsale case), to reduce the surface vector for potential attacks.

I'll be updating the contracts and tests soon.

…hods to access it.

nventuro · 2018-07-02T22:22:06Z

Escrows are now an implementation detail of the contracts that use them as a payment method. Users of these contracts will never interact with an Escrow directly, making it easier to reason about security concerns. The two contracts that implement this pattern are PullPayment and RefundableCrowdsale.

frangio · 2018-07-03T21:42:49Z

Thanks @nventuro!

nventuro added 5 commits June 15, 2018 13:46

Added basic Escrow

2d2075a

PullPayment now uses an Escrow, removing all trust from the contract

b879f4c

Abstracted the Escrow tests to a behaviour

48b64ce

Added ConditionalEscrow

3600240

Added RefundableEscrow.

fe63f2e

nventuro requested review from frangio, come-maiz and shrugs June 15, 2018 21:44

RefundableCrowdsale now uses a RefundEscrow, removed RefundVault.

3a33ffb

nventuro force-pushed the feat/escrow branch from 9be72d5 to 3a33ffb Compare June 16, 2018 05:11

frangio requested changes Jun 16, 2018

View reviewed changes

frangio reviewed Jun 18, 2018

View reviewed changes

nventuro force-pushed the feat/escrow branch 2 times, most recently from 786ceac to 3a33ffb Compare June 18, 2018 20:02

nventuro added 5 commits June 18, 2018 17:04

Merge branch 'master' into feat/escrow

d389fd3

Renaming after code review.

e01edb3

Added log test helper.

5dfb9ce

Now allowing empty deposits and withdrawals.

6166248

Style fixes.

cd84c37

frangio reviewed Jun 18, 2018

View reviewed changes

nventuro mentioned this pull request Jun 18, 2018

Behaviour test functions are anonymous #1027

Closed

nventuro added 2 commits June 18, 2018 18:00

Minor review comments.

dabb3d7

Add Deposited and Withdrawn events, removed Refunded

4422858

shrugs approved these changes Jun 21, 2018

View reviewed changes

The base Escrow is now Ownable, users of it (owners) must provide met…

be8d65f

…hods to access it.

frangio approved these changes Jul 3, 2018

View reviewed changes

Merge branch 'master' into feat/escrow

597aba7

frangio merged commit 8fd072c into OpenZeppelin:master Jul 3, 2018

nventuro deleted the feat/escrow branch July 3, 2018 21:58

nventuro mentioned this pull request Aug 22, 2018

PullPayment version that checks balances #197

Closed

		@@ -1,4 +1,5 @@
		import EVMRevert from '../helpers/EVMRevert';
		import { inLogs } from '../helpers/expectEvent';

Escrows #1014

Escrows #1014

Conversation

nventuro commented Jun 15, 2018 • edited Loading

🚀 Description

frangio left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nventuro Jun 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frangio Jun 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frangio left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nventuro Jun 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frangio Jun 18, 2018 • edited Loading

Choose a reason for hiding this comment

shrugs left a comment

Choose a reason for hiding this comment

nventuro commented Jun 30, 2018

nventuro commented Jul 2, 2018

frangio commented Jul 3, 2018

nventuro commented Jun 15, 2018 •

edited

Loading

nventuro Jun 18, 2018 •

edited

Loading

frangio Jun 16, 2018 •

edited

Loading

frangio left a comment •

edited

Loading

nventuro Jun 18, 2018 •

edited

Loading

frangio Jun 18, 2018 •

edited

Loading