Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Escrows #1014

Merged
merged 15 commits into from
Jul 3, 2018
Merged

Escrows #1014

Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions contracts/Bounty.sol
Original file line number Diff line number Diff line change
Expand Up @@ -35,15 +35,15 @@ contract Bounty is PullPayment, Destructible {
}

/**
* @dev Sends the contract funds to the researcher that proved the contract is broken.
* @dev Transfers the contract funds to the researcher that proved the contract is broken.
* @param target contract
*/
function claim(Target target) public {
address researcher = researchers[target];
require(researcher != address(0));
// Check Target contract invariants
require(!target.checkInvariant());
asyncSend(researcher, address(this).balance);
asyncTransfer(researcher, address(this).balance);
claimed = true;
}

Expand Down
9 changes: 4 additions & 5 deletions contracts/crowdsale/distribution/RefundableCrowdsale.sol
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ import "../../payment/RefundEscrow.sol";
* @title RefundableCrowdsale
* @dev Extension of Crowdsale contract that adds a funding goal, and
* the possibility of users getting a refund if goal is not met.
* Uses a RefundEscrow as the crowdsale's escrow.
*/
contract RefundableCrowdsale is FinalizableCrowdsale {
using SafeMath for uint256;
Expand All @@ -19,7 +18,7 @@ contract RefundableCrowdsale is FinalizableCrowdsale {
uint256 public goal;

// refund escrow used to hold funds while crowdsale is running
RefundEscrow public escrow;
RefundEscrow private escrow;

/**
* @dev Constructor, creates RefundEscrow.
Expand All @@ -38,7 +37,7 @@ contract RefundableCrowdsale is FinalizableCrowdsale {
require(isFinalized);
require(!goalReached());

escrow.refund(msg.sender);
escrow.withdraw(msg.sender);
}

/**
Expand All @@ -55,7 +54,7 @@ contract RefundableCrowdsale is FinalizableCrowdsale {
function finalization() internal {
if (goalReached()) {
escrow.close();
escrow.withdraw();
escrow.beneficiaryWithdraw();
} else {
escrow.enableRefunds();
}
Expand All @@ -67,7 +66,7 @@ contract RefundableCrowdsale is FinalizableCrowdsale {
* @dev Overrides Crowdsale fund forwarding, sending funds to escrow.
*/
function _forwardFunds() internal {
escrow.invest.value(msg.value)(msg.sender);
escrow.deposit.value(msg.value)(msg.sender);
}

}
6 changes: 3 additions & 3 deletions contracts/mocks/PullPaymentMock.sol
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,9 @@ contract PullPaymentMock is PullPayment {

constructor() public payable { }

// test helper function to call asyncSend
function callSend(address dest, uint256 amount) public {
asyncSend(dest, amount);
// test helper function to call asyncTransfer
function callTransfer(address dest, uint256 amount) public {
asyncTransfer(dest, amount);
}

}
2 changes: 1 addition & 1 deletion contracts/payment/Escrow.sol
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ contract Escrow {
uint256 payment = deposits[_payee];

require(payment != 0);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This feels unnecessary as well. I would remove it.

require(address(this).balance >= payment);
assert(address(this).balance >= payment);

deposits[_payee] = 0;

Expand Down
8 changes: 4 additions & 4 deletions contracts/payment/PullPayment.sol
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,10 @@ import "./Escrow.sol";
/**
* @title PullPayment
* @dev Base contract supporting async send for pull payments. Inherit from this
* contract and use asyncSend instead of send or transfer.
* contract and use asyncTransfer instead of send or transfer.
*/
contract PullPayment {
Escrow public escrow;
Escrow private escrow;

constructor() public {
escrow = new Escrow();
Expand All @@ -25,7 +25,7 @@ contract PullPayment {

/**
* @dev Returns the credit owed to an address.
& @param _dest The creditor's address.
* @param _dest The creditor's address.
*/
function payments(address _dest) public view returns (uint256) {
return escrow.depositsOf(_dest);
Expand All @@ -36,7 +36,7 @@ contract PullPayment {
* @param _dest The destination address of the funds.
* @param _amount The amount to transfer.
*/
function asyncSend(address _dest, uint256 _amount) internal {
function asyncTransfer(address _dest, uint256 _amount) internal {
escrow.deposit.value(_amount)(_dest);
}
}
42 changes: 18 additions & 24 deletions contracts/payment/RefundEscrow.sol
Original file line number Diff line number Diff line change
Expand Up @@ -6,22 +6,23 @@ import "../ownership/Ownable.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.
* @dev Escrow that holds funds for a beneficiary, deposited from multiple parties.
* The contract owner may close the deposit period, and allow for either withdrawal
* by the beneficiary, or refunds to the depositors.
*/
contract RefundEscrow is ConditionalEscrow, Ownable {
enum State { Active, Refunding, Closed }

event Closed();
event RefundsEnabled();
event Refunded(address indexed investor, uint256 weiAmount);
event Refunded(address indexed refundee, uint256 weiAmount);

State public state;
address public beneficiary;

/**
* @dev Constructor.
* @param _beneficiary The beneficiary of the investments.
* @param _beneficiary The beneficiary of the deposits.
*/
constructor(address _beneficiary) public {
require(_beneficiary != address(0));
Expand All @@ -31,23 +32,16 @@ contract RefundEscrow is ConditionalEscrow, Ownable {

/**
* @dev Stores funds that may later be refunded.
* @param _investor The address funds will be sent to if a refund occurs.
* @param _refundee The address funds will be sent to if a refund occurs.
*/
function invest(address _investor) payable public {
function deposit(address _refundee) payable public {
require(state == State.Active);
super.deposit(_investor);
}

/**
* @dev Disable the base deposit function, use invest instead.
*/
function deposit(address _payee) payable public {
revert();
super.deposit(_refundee);
}

/**
* @dev Allows for the beneficiary to withdraw their funds, rejecting
* further investments.
* further deposits.
*/
function close() onlyOwner public {
require(state == State.Active);
Expand All @@ -56,7 +50,7 @@ contract RefundEscrow is ConditionalEscrow, Ownable {
}

/**
* @dev Allows for refunds to take place, rejecting further investments.
* @dev Allows for refunds to take place, rejecting further deposits.
*/
function enableRefunds() onlyOwner public {
require(state == State.Active);
Expand All @@ -67,23 +61,23 @@ contract RefundEscrow is ConditionalEscrow, Ownable {
/**
* @dev Withdraws the beneficiary's funds.
*/
function withdraw() public {
function beneficiaryWithdraw() public {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

require(state == State.Closed);
beneficiary.transfer(address(this).balance);
}

/**
* @dev Refunds an investor.
* @param _investor The address to refund.
* @dev Refunds a refundee.
* @param _refundee The address to refund.
*/
function refund(address _investor) public {
uint256 amount = depositsOf(_investor);
super.withdraw(_investor);
emit Refunded(_investor, amount);
function withdraw(address _refundee) public {
uint256 amount = depositsOf(_refundee);
super.withdraw(_refundee);
emit Refunded(_refundee, amount);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove when we add events to Escrow.

}

/**
* @dev Returns whether investors can withdraw their investments (be refunded).
* @dev Returns whether refundees can withdraw their deposits (be refunded).
*/
function withdrawalAllowed(address _payee) public view returns (bool) {
return state == State.Refunding;
Expand Down
12 changes: 6 additions & 6 deletions test/payment/ConditionalEscrow.test.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import shouldBehaveLikeEscrow from './Escrow.behaviour';
import { shouldBehaveLikeEscrow } from './Escrow.behaviour';
import EVMRevert from '../helpers/EVMRevert';

const BigNumber = web3.BigNumber;
Expand All @@ -11,12 +11,12 @@ const ConditionalEscrowMock = artifacts.require('ConditionalEscrowMock');

contract('ConditionalEscrow', function (accounts) {
beforeEach(async function () {
this.contract = await ConditionalEscrowMock.new();
this.escrow = await ConditionalEscrowMock.new();
});

context('when withdrawal is allowed', function () {
beforeEach(async function () {
await Promise.all(accounts.map(payee => this.contract.setAllowed(payee, true)));
await Promise.all(accounts.map(payee => this.escrow.setAllowed(payee, true)));
});

shouldBehaveLikeEscrow(accounts);
Expand All @@ -30,13 +30,13 @@ contract('ConditionalEscrow', function (accounts) {
const withdrawer = accounts[2];

beforeEach(async function () {
await this.contract.setAllowed(payee, false);
await this.escrow.setAllowed(payee, false);
});

it('reverts on withdrawals', async function () {
await this.contract.deposit(payee, { from: payer, value: amount });
await this.escrow.deposit(payee, { from: payer, value: amount });

await this.contract.withdraw(payee, { from: withdrawer }).should.be.rejectedWith(EVMRevert);
await this.escrow.withdraw(payee, { from: withdrawer }).should.be.rejectedWith(EVMRevert);
});
});
});
38 changes: 19 additions & 19 deletions test/payment/Escrow.behaviour.js
Original file line number Diff line number Diff line change
Expand Up @@ -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]) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@nventuro nventuro Jun 18, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

const amount = web3.toWei(42.0, 'ether');

describe('as an escrow', function () {
it('can accept a single deposit', async function () {
await this.contract.deposit(payee1, { from: payer1, value: amount });
await this.escrow.deposit(payee1, { from: payer1, value: amount });

const balance = await web3.eth.getBalance(this.contract.address);
const deposit = await this.contract.depositsOf(payee1);
const balance = await web3.eth.getBalance(this.escrow.address);
const deposit = await this.escrow.depositsOf(payee1);

balance.should.be.bignumber.equal(amount);
deposit.should.be.bignumber.equal(amount);
});

it('reverts on empty deposits', async function () {
await this.contract.deposit(payee1, { from: payer1, value: 0 }).should.be.rejectedWith(EVMRevert);
await this.escrow.deposit(payee1, { from: payer1, value: 0 }).should.be.rejectedWith(EVMRevert);
});

it('can add multiple deposits on a single account', async function () {
await this.contract.deposit(payee1, { from: payer1, value: amount });
await this.contract.deposit(payee1, { from: payer2, value: amount * 2 });
await this.escrow.deposit(payee1, { from: payer1, value: amount });
await this.escrow.deposit(payee1, { from: payer2, value: amount * 2 });

const balance = await web3.eth.getBalance(this.contract.address);
const deposit = await this.contract.depositsOf(payee1);
const balance = await web3.eth.getBalance(this.escrow.address);
const deposit = await this.escrow.depositsOf(payee1);

balance.should.be.bignumber.equal(amount * 3);
deposit.should.be.bignumber.equal(amount * 3);
});

it('can track deposits to multiple accounts', async function () {
await this.contract.deposit(payee1, { from: payer1, value: amount });
await this.contract.deposit(payee2, { from: payer1, value: amount * 2 });
await this.escrow.deposit(payee1, { from: payer1, value: amount });
await this.escrow.deposit(payee2, { from: payer1, value: amount * 2 });

const balance = await web3.eth.getBalance(this.contract.address);
const depositPayee1 = await this.contract.depositsOf(payee1);
const depositPayee2 = await this.contract.depositsOf(payee2);
const balance = await web3.eth.getBalance(this.escrow.address);
const depositPayee1 = await this.escrow.depositsOf(payee1);
const depositPayee2 = await this.escrow.depositsOf(payee2);

balance.should.be.bignumber.equal(amount * 3);
depositPayee1.should.be.bignumber.equal(amount);
Expand All @@ -51,11 +51,11 @@ export default function ([payer1, payer2, payee1, payee2]) {
it('can withdraw payments from any account', async function () {
const payeeInitialBalance = await web3.eth.getBalance(payee1);

await this.contract.deposit(payee1, { from: payer1, value: amount });
await this.contract.withdraw(payee1, { from: payer2 });
await this.escrow.deposit(payee1, { from: payer1, value: amount });
await this.escrow.withdraw(payee1, { from: payer2 });

const escrowBalance = await web3.eth.getBalance(this.contract.address);
const finalDeposit = await this.contract.depositsOf(payee1);
const escrowBalance = await web3.eth.getBalance(this.escrow.address);
const finalDeposit = await this.escrow.depositsOf(payee1);
const payeeFinalBalance = await web3.eth.getBalance(payee1);

escrowBalance.should.be.bignumber.equal(0);
Expand All @@ -64,7 +64,7 @@ export default function ([payer1, payer2, payee1, payee2]) {
});

it('reverts on empty withdrawals', async function () {
await this.contract.withdraw(payee1, { from: payer1 }).should.be.rejectedWith(EVMRevert);
await this.escrow.withdraw(payee1, { from: payer1 }).should.be.rejectedWith(EVMRevert);
});
});
};
4 changes: 2 additions & 2 deletions test/payment/Escrow.test.js
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
import shouldBehaveLikeEscrow from './Escrow.behaviour';
import { shouldBehaveLikeEscrow } from './Escrow.behaviour';

const Escrow = artifacts.require('Escrow');

contract('Escrow', function (accounts) {
beforeEach(async function () {
this.contract = await Escrow.new();
this.escrow = await Escrow.new();
});

shouldBehaveLikeEscrow(accounts);
Expand Down
12 changes: 6 additions & 6 deletions test/payment/PullPayment.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,22 +19,22 @@ contract('PullPayment', function (accounts) {

it('can record an async payment correctly', async function () {
const AMOUNT = 100;
await this.contract.callSend(accounts[0], AMOUNT);
await this.contract.callTransfer(accounts[0], AMOUNT);

const paymentsToAccount0 = await this.contract.payments(accounts[0]);
paymentsToAccount0.should.be.bignumber.equal(AMOUNT);
});

it('can add multiple balances on one account', async function () {
await this.contract.callSend(accounts[0], 200);
await this.contract.callSend(accounts[0], 300);
await this.contract.callTransfer(accounts[0], 200);
await this.contract.callTransfer(accounts[0], 300);
const paymentsToAccount0 = await this.contract.payments(accounts[0]);
paymentsToAccount0.should.be.bignumber.equal(500);
});

it('can add balances on multiple accounts', async function () {
await this.contract.callSend(accounts[0], 200);
await this.contract.callSend(accounts[1], 300);
await this.contract.callTransfer(accounts[0], 200);
await this.contract.callTransfer(accounts[1], 300);

const paymentsToAccount0 = await this.contract.payments(accounts[0]);
paymentsToAccount0.should.be.bignumber.equal(200);
Expand All @@ -47,7 +47,7 @@ contract('PullPayment', function (accounts) {
const payee = accounts[1];
const initialBalance = web3.eth.getBalance(payee);

await this.contract.callSend(payee, amount);
await this.contract.callTransfer(payee, amount);

const payment1 = await this.contract.payments(payee);
payment1.should.be.bignumber.equal(amount);
Expand Down
Loading