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

Track @next contract changes (Aragon 0.7-part.2) #723

Merged
merged 27 commits into from Apr 10, 2019
Merged
Changes from 1 commit
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
01102cb
Survey: optimize voteOption() (#705)
sohkai Mar 20, 2019
26192e1
Finance: add uint256 casts for parameters in authP (#708)
sohkai Mar 20, 2019
4e65165
Token Manager: remove unused storage mapping (#712)
sohkai Mar 20, 2019
1fe5298
Finance, Token Manager, Voting: split utility getters into public and…
sohkai Mar 20, 2019
5f86d8c
Finance: fix naming of status parameter in ChangePaymentState event (…
sohkai Mar 20, 2019
4c9b720
Token Manager: enforce all token controller methods to only be access…
sohkai Mar 20, 2019
d5feb33
Finance: optimize SafeMath (#706)
sohkai Mar 20, 2019
dd59c23
Finance, Token Manager, Vault: use address(this) for address argument…
sohkai Mar 21, 2019
c4f2e7a
Finance: _tryTransitionAccountPeriod should take uint64 argument (#725)
sohkai Mar 21, 2019
4e89224
Voting: mock time in tests rather than use timetravel (#737)
sohkai Mar 28, 2019
4eefbb2
Finance, Survey, Token Manager, Voting: Clarify internal protection s…
sohkai Mar 28, 2019
8477444
Token Manager: disallow minting and vesting tokens to itself (#734)
sohkai Mar 28, 2019
8323e4c
Token Manager: use TimeHelpers and mock time in tests (#738)
sohkai Mar 28, 2019
3d8b2dd
Finance: consistent payment creation (#711)
sohkai Mar 28, 2019
02e79bd
Finance: cosmetic clarifications for newImmediatePayment() (#749)
sohkai Mar 28, 2019
0289975
Finance: rename RecurringPayments to ScheduledPayments (#735)
sohkai Mar 28, 2019
30d30dd
Finance: rename maxRepeats to maxExecutions (#739)
sohkai Mar 28, 2019
65f3a6e
Finance: fix usage of timestamp mock in tests (#750)
sohkai Mar 28, 2019
5136285
Finance: reduce length of error string to under 32 chars (#751)
sohkai Mar 28, 2019
a66f876
Finance: fix usage of ERROR_NEW_PAYMENT_EXECS_ZERO (#752)
sohkai Mar 28, 2019
ca52be5
cosmetic(Finance): update @dev wording to be consistent
facuspagnuolo Apr 9, 2019
7b9cbd0
test: fix test parameterization variables (#779)
sohkai Apr 9, 2019
1af7a59
Token Manager: small improvements for ITokenController interface (#770)
sohkai Apr 10, 2019
d8b52a0
Finance: use constant for minimum period (#781)
sohkai Apr 10, 2019
0d7fecb
Token Manager: remove unused imports (#771)
sohkai Apr 10, 2019
b5df2ef
Finance: rename MAX_UINT to MAX_UINT256 (#772)
sohkai Apr 10, 2019
a62f7ed
Merge branch 'master' into next
sohkai Apr 10, 2019
File filter...
Filter file types
Jump to…
Jump to file or symbol
Failed to load files and symbols.
+293 −140
Diff settings

Always

Just for now

Finance: consistent payment creation (#711)

Splits up `newPayment()` into:

- `newScheduledPayment()`: _always_ saves a `RecurringPayment`, for repeating or scheduled future payments
- `newImmediatePayment()`: _only_ for single, immediately-payable transactions

It also modifies the execution behaviour of payments. Previously, it was possible to call `executePayment()` or `receiverExecutePayment()` even if no payments were made (only a single `PaymentFailure` event would be emitted). Now calling these two functions will cause a revert, with more obvious error messages (either you hadn't waited long enough, or there wasn't enough balance in the Vault).

We still allow `newScheduledPayment()` to create a "failing" payment, as I think it'd still be useful to schedule payments even if you can't immediately pay them out.
  • Loading branch information...
sohkai committed Mar 28, 2019
commit 3d8b2dd1179f47f688557aa76cfd0bccc94bfce0
@@ -49,13 +49,10 @@ class App extends React.Component {
}
handleWithdraw = (tokenAddress, recipient, amount, reference) => {
// Immediate, one-time payment
this.props.app.newPayment(
this.props.app.newImmediatePayment(
tokenAddress,
recipient,
amount,
0, // initial payment time
0, // interval
1, // max repeats
reference
)
this.handleNewTransferClose()
@@ -49,7 +49,8 @@
"Receiver address",
"Token amount",
"Payment interval",
"Max repeats"
"Max repeats",
"Initial payment time"
]
},
{
@@ -41,16 +41,19 @@ contract Finance is EtherTokenConstant, IsContract, AragonApp {
string private constant ERROR_INIT_PERIOD_TOO_SHORT = "FINANCE_INIT_PERIOD_TOO_SHORT";
string private constant ERROR_SET_PERIOD_TOO_SHORT = "FINANCE_SET_PERIOD_TOO_SHORT";
string private constant ERROR_NEW_PAYMENT_AMOUNT_ZERO = "FINANCE_NEW_PAYMENT_AMOUNT_ZERO";
string private constant ERROR_NEW_PAYMENT_INTERVAL_ZERO = "FINANCE_NEW_PAYMENT_INTRVL_ZERO";
string private constant ERROR_NEW_PAYMENT_REPEATS_ZERO = "FINANCE_NEW_PAYMENT_REPEATS_ZERO";
string private constant ERROR_NEW_PAYMENT_IMMEDIATE = "FINANCE_NEW_PAYMENT_IMMEDIATE";
string private constant ERROR_RECOVER_AMOUNT_ZERO = "FINANCE_RECOVER_AMOUNT_ZERO";
string private constant ERROR_DEPOSIT_AMOUNT_ZERO = "FINANCE_DEPOSIT_AMOUNT_ZERO";
string private constant ERROR_ETH_VALUE_MISMATCH = "ERROR_ETH_VALUE_MISMATCH";
string private constant ERROR_ETH_VALUE_MISMATCH = "FINANCE_ETH_VALUE_MISMATCH";
string private constant ERROR_BUDGET = "FINANCE_BUDGET";
string private constant ERROR_EXECUTE_PAYMENT_NUM = "FINANCE_EXECUTE_PAYMENT_NUM";
string private constant ERROR_EXECUTE_PAYMENT_TIME = "FINANCE_EXECUTE_PAYMENT_TIME";
string private constant ERROR_RECEIVER_EXECUTE_PAYMENT_TIME = "FINANCE_RCVR_EXEC_PAYMENT_TIME";
string private constant ERROR_PAYMENT_RECEIVER = "FINANCE_PAYMENT_RECEIVER";
string private constant ERROR_TOKEN_TRANSFER_FROM_REVERTED = "FINANCE_TKN_TRANSFER_FROM_REVERT";
string private constant ERROR_TOKEN_APPROVE_FAILED = "FINANCE_TKN_APPROVE_FAILED";
string private constant ERROR_RECURRING_PAYMENT_INACTIVE = "FINANCE_RECURRING_PAYMENT_INACTIVE";
string private constant ERROR_PAYMENT_INACTIVE = "FINANCE_PAYMENT_INACTIVE";
string private constant ERROR_REMAINING_BUDGET = "FINANCE_REMAINING_BUDGET";

// Order optimized for storage
@@ -145,8 +148,8 @@ contract Finance is EtherTokenConstant, IsContract, AragonApp {
}

/**
* @dev Sends ETH to Vault. Sends all the available balance.
* @notice Deposit ETH to the Vault, to avoid locking them in this Finance app forever
* @dev Sends ETH to Vault. Sends all the available balance.
This conversation was marked as resolved by sohkai

This comment has been minimized.

Copy link
@facuspagnuolo

facuspagnuolo Apr 8, 2019

Contributor

Shouldn't we use "Send" here?

Suggested change
* @dev Sends ETH to Vault. Sends all the available balance.
* @dev Send ETH to Vault. Send all the available balance.
*/
function () external payable isInitialized transitionsPeriod {
require(msg.value > 0, ERROR_DEPOSIT_AMOUNT_ZERO);
@@ -186,8 +189,8 @@ contract Finance is EtherTokenConstant, IsContract, AragonApp {
}

/**
* @dev Deposit for approved ERC20 tokens or ETH
* @notice Deposit `@tokenAmount(_token, _amount)`
* @dev Deposit for approved ERC20 tokens or ETH
* @param _token Address of deposited token
* @param _amount Amount of tokens sent
* @param _reference Reason for payment
@@ -209,7 +212,8 @@ contract Finance is EtherTokenConstant, IsContract, AragonApp {
}

/**
* @notice Create a new payment of `@tokenAmount(_token, _amount)` to `_receiver``_maxRepeats > 0 ? ', executing ' + _maxRepeats + ' times at intervals of ' + @transformTime(_interval) : ''`, for '`_reference`'
* @notice Create a new payment of `@tokenAmount(_token, _amount)` to `_receiver` for `_reference`, executing `_maxRepeats` times at intervals of `@transformTime(_interval)`
* @dev See `newImmediatePayment()` for limitations on how the interval auth parameter can be used
* @param _token Address of token for payment
* @param _receiver Address that will receive payment
* @param _amount Tokens that are paid every time the payment is due
@@ -218,7 +222,7 @@ contract Finance is EtherTokenConstant, IsContract, AragonApp {
* @param _maxRepeats Maximum instances a payment can be executed
* @param _reference String detailing payment reason
*/
function newPayment(
function newScheduledPayment(
address _token,
address _receiver,
uint256 _amount,
@@ -228,28 +232,23 @@ contract Finance is EtherTokenConstant, IsContract, AragonApp {
string _reference
)
external
authP(CREATE_PAYMENTS_ROLE, arr(_token, _receiver, _amount, _interval, _maxRepeats))
// Payment time parameter is left as the last param as it was added later
authP(CREATE_PAYMENTS_ROLE, _arr(_token, _receiver, _amount, uint256(_interval), uint256(_maxRepeats), uint256(_initialPaymentTime)))
transitionsPeriod
returns (uint256 paymentId)
{
require(_amount > 0, ERROR_NEW_PAYMENT_AMOUNT_ZERO);

// Avoid saving payment data for one-time immediate payments
if (_initialPaymentTime <= getTimestamp64() && _maxRepeats == 1) {
_makePaymentTransaction(
_token,
_receiver,
_amount,
NO_RECURRING_PAYMENT, // unrelated to any payment id; it isn't created
0, // also unrelated to any payment repeats
_reference
);
return;
}
require(_interval > 0, ERROR_NEW_PAYMENT_INTERVAL_ZERO);
require(_maxRepeats > 0, ERROR_NEW_PAYMENT_REPEATS_ZERO);

// Token budget must not be set at all or allow at least one instance of this payment each period
require(!settings.hasBudget[_token] || settings.budgets[_token] >= _amount, ERROR_BUDGET);

// Don't allow creating single payments that are immediately executable, use `newImmediatePayment()` instead
if (_maxRepeats == 1) {
require(_initialPaymentTime > getTimestamp64(), ERROR_NEW_PAYMENT_IMMEDIATE);
}

paymentId = paymentsNextIndex++;
emit NewPayment(paymentId, _receiver, _maxRepeats, _reference);

@@ -262,11 +261,41 @@ contract Finance is EtherTokenConstant, IsContract, AragonApp {
payment.maxRepeats = _maxRepeats;
payment.createdBy = msg.sender;

if (_nextPaymentTime(paymentId) <= getTimestamp64()) {
_executePayment(paymentId);
}
// We skip checking how many times the new payment was executed to allow creating new
// recurring payments before having enough vault balance
_executePayment(paymentId);
}

/**
* @notice Create a new payment of `@tokenAmount(_token, _amount)` to `_receiver` for '`_reference`'
* @dev Note that this function is protected by the `CREATE_PAYMENTS_ROLE` but uses `MAX_UINT64`
* as its interval auth parameter. While this protects against most cases (you typically want
* to set a baseline requirement for the interval time), it does mean users can't grant a
* permission that has a upperbound requirement for the interval time.
* @param _token Address of token for payment
* @param _receiver Address that will receive payment
* @param _amount Tokens that are paid every time the payment is due
* @param _reference String detailing payment reason
*/
function newImmediatePayment(address _token, address _receiver, uint256 _amount, string _reference)
external
// Use MAX_UINT256 as the interval parameter, as this payment will never repeat
// Payment time parameter is left as the last param as it was added later
authP(CREATE_PAYMENTS_ROLE, _arr(_token, _receiver, _amount, MAX_UINT, uint256(1), getTimestamp()))
transitionsPeriod
{
require(_amount > 0, ERROR_NEW_PAYMENT_AMOUNT_ZERO);

_makePaymentTransaction(
_token,
_receiver,
_amount,
NO_RECURRING_PAYMENT, // unrelated to any payment id; it isn't created
0, // also unrelated to any payment repeats
_reference
);
}

/**
* @notice Change period duration to `@transformTime(_periodDuration)`, effective for next accounting period
* @param _periodDuration Duration in seconds for accounting periods
@@ -316,8 +345,8 @@ contract Finance is EtherTokenConstant, IsContract, AragonApp {
}

/**
* @dev Executes any payment (requires role)
* @notice Execute pending payment #`_paymentId`
* @dev Executes any payment (requires role)
* @param _paymentId Identifier for payment
*/
function executePayment(uint256 _paymentId)
@@ -326,21 +355,19 @@ contract Finance is EtherTokenConstant, IsContract, AragonApp {
recurringPaymentExists(_paymentId)
transitionsPeriod
{
require(_nextPaymentTime(_paymentId) <= getTimestamp64(), ERROR_EXECUTE_PAYMENT_TIME);

_executePayment(_paymentId);
_executePaymentAtLeastOnce(_paymentId);
}

/**
* @dev Always allow receiver of a payment to trigger execution
* @notice Execute pending payment #`_paymentId`
* @dev Always allow receiver of a payment to trigger execution
* Initialization check is implicitly provided by `recurringPaymentExists()` as new
* recurring payments can only be created via `newScheduledPayment(),` which requires initialization
* @param _paymentId Identifier for payment
*/
function receiverExecutePayment(uint256 _paymentId) external isInitialized recurringPaymentExists(_paymentId) transitionsPeriod {
require(_nextPaymentTime(_paymentId) <= getTimestamp64(), ERROR_RECEIVER_EXECUTE_PAYMENT_TIME);
function receiverExecutePayment(uint256 _paymentId) external recurringPaymentExists(_paymentId) transitionsPeriod {
require(recurringPayments[_paymentId].receiver == msg.sender, ERROR_PAYMENT_RECEIVER);

_executePayment(_paymentId);
_executePaymentAtLeastOnce(_paymentId);
}

/**
@@ -362,10 +389,10 @@ contract Finance is EtherTokenConstant, IsContract, AragonApp {
}

/**
* @notice Send tokens held in this contract to the Vault
* @dev Allows making a simple payment from this contract to the Vault, to avoid locked tokens.
* This contract should never receive tokens with a simple transfer call, but in case it
* happens, this function allows for their recovery.
* @notice Send tokens held in this contract to the Vault
* @param _token Token whose balance is going to be transferred.
*/
function recoverToVault(address _token) external isInitialized transitionsPeriod {
@@ -382,10 +409,10 @@ contract Finance is EtherTokenConstant, IsContract, AragonApp {
}

/**
* @notice Transition accounting period if needed
* @dev Transitions accounting periods if needed. For preventing OOG attacks, a maxTransitions
* param is provided. If more than the specified number of periods need to be transitioned,
* it will return false.
* @notice Transition accounting period if needed
* @param _maxTransitions Maximum periods that can be transitioned
* @return success Boolean indicating whether the accounting period is the correct one (if false,
* maxTransitions was surpased and another call is needed)
@@ -532,7 +559,7 @@ contract Finance is EtherTokenConstant, IsContract, AragonApp {

/**
* @dev Initialization check is implicitly provided by `recurringPaymentExists()` as new
* recurring payments can only be created via `newPayment(),` which requires initialization
* recurring payments can only be created via `newScheduledPayment(),` which requires initialization
*/
function nextPaymentTime(uint256 _paymentId) public view recurringPaymentExists(_paymentId) returns (uint64) {
return _nextPaymentTime(_paymentId);
@@ -568,15 +595,15 @@ contract Finance is EtherTokenConstant, IsContract, AragonApp {
}
}

function _executePayment(uint256 _paymentId) internal {
function _executePayment(uint256 _paymentId) internal returns (uint256) {
RecurringPayment storage payment = recurringPayments[_paymentId];
require(!payment.inactive, ERROR_RECURRING_PAYMENT_INACTIVE);
require(!payment.inactive, ERROR_PAYMENT_INACTIVE);

uint64 paid = 0;
while (_nextPaymentTime(_paymentId) <= getTimestamp64() && paid < MAX_RECURRING_PAYMENTS_PER_TX) {
if (!_canMakePayment(payment.token, payment.amount)) {
emit PaymentFailure(_paymentId);
return;
break;
}

// The while() predicate prevents these two from ever overflowing
@@ -593,6 +620,19 @@ contract Finance is EtherTokenConstant, IsContract, AragonApp {
""
);
}

return paid;
}

function _executePaymentAtLeastOnce(uint256 _paymentId) internal {
uint256 paid = _executePayment(_paymentId);
if (paid == 0) {
if (_nextPaymentTime(_paymentId) <= getTimestamp64()) {
revert(ERROR_EXECUTE_PAYMENT_NUM);
} else {
revert(ERROR_EXECUTE_PAYMENT_TIME);
}
}
}

function _makePaymentTransaction(
@@ -781,6 +821,18 @@ contract Finance is EtherTokenConstant, IsContract, AragonApp {
return nextPayment;
}

// Syntax sugar

function _arr(address _a, address _b, uint256 _c, uint256 _d, uint256 _e, uint256 _f) internal pure returns (uint256[] r) {
r = new uint256[](6);
r[0] = uint256(_a);
r[1] = uint256(_b);
r[2] = _c;
r[3] = _d;
r[4] = _e;
r[5] = _f;

This comment has been minimized.

Copy link
@facuspagnuolo

facuspagnuolo Apr 8, 2019

Contributor

Should we move this one to ACLSyntaxSugar?

This comment has been minimized.

Copy link
@sohkai

sohkai Apr 9, 2019

Author Member

I had debated this, but this is such a super specific instance that I don't think it'd make sense to add back in ACLSyntaxSugar. Apps that have giant params lists like this will need their own syntax sugar :).

}

// Mocked fns (overrided during testing)
// Must be view for mocking purposes

Oops, something went wrong.
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.