Finance App #99
Finance App #99
Conversation
readevalprint
commented
Sep 13, 2017
•
readevalprint
commented
- update Accounting with recurring payments/invoices
- deposit function should record transactions
- Add natspec
- More tests
Add aesthetically pleasing space in README
readevalprint
force-pushed
the
accountingapp
branch
from
65b2fdb
to
1011d3f
Compare
There was a problem hiding this comment.
With the introduction of Payments, now all the logic regarding transaction states is no longer relevant as _newOutgoingTransaction is only called when withdrawing a payment. All state handling functions (approveTransaction, denyTransaction) don't have any real effect (but spend a lot in storage) as transactions can only go from creation to execution.
When you test the code it is possible that you will run into issues because of using the memory keyword in instances where you need to modify storage.
Also code is hard to follow as functions are weirdly ordered. Following the style guide or a more logical approach would go a long way in terms of readability.
Support for wrapping and unwrapping ether in an EtherToken is missing.
| import "../vault/Vault.sol"; | ||
| import "../../misc/Crontab.sol"; | ||
|
|
||
|
|
|
|
||
|
|
||
|
|
||
| contract FinanceApp is App, Initializable, Crontab { |
There was a problem hiding this comment.
We should rename dir 'apps/accounting/FinanceApp.sol' to 'apps/finance/FinanceApp.sol'
| Vault vault; | ||
|
|
||
| struct AccountingPeriod { | ||
|
|
There was a problem hiding this comment.
no newline needed before first struct member (the Transaction struct is perfect)
| @dev Set the schedual of payments | ||
| */ | ||
| function _setPaymentSchedual(uint pid, uint repeat, bytes2 ct_sec, bytes2 ct_min, bytes2 ct_hour, bytes2 ct_day, bytes2 ct_month, bytes2 ct_weekday, bytes2 ct_year) internal { | ||
| Payment memory p = payments[pid]; |
There was a problem hiding this comment.
Payment should be in storage here or modifications won't be saved to storage
| require(state == TransactionState.PendingApproval); | ||
| _updateTransaction(transactionId, TransactionState.Denied, reason); | ||
| } | ||
| bytes4 constant TRANSFER_SIG = bytes4(sha3('transfer(address,address,uint256)')); |
| // TODO: store endBlock of last accountingPeriod? | ||
| ap.startBlock = block.number; | ||
| accountingPeriods.push(ap); | ||
| //vault.requestAllowances(ap.budgetTokens, ap.budgetAmounts); |
There was a problem hiding this comment.
I hadn't pulled from your branch when I added this initially so the function didn't exist
| /** | ||
| @dev External authenticated function to start the next accounting period | ||
| */ | ||
| function startNextAccountingPeriod() external auth { |
There was a problem hiding this comment.
startNewAccountingPeriod should not only not be protected behind the ACL but it should be tried to be executed in every write function, as if the accounting period has ended, to ensure transactions are in the correct accounting period.
There was a problem hiding this comment.
Ah good point
| Transaction memory t = transactions[transactionId]; | ||
| var (state, r) = getTransactionState(transactionId); | ||
| require(state == TransactionState.Approved); | ||
| bool succeeded = ERC20(t.token).transfer(t.externalAddress, t.amount); |
There was a problem hiding this comment.
This needs to be a transferFrom. The FinanceApp doesn't have a balance in the tokens, just whatever it was allowed.
| function newPayment(ERC20 token, uint amount, address to, uint repeat, uint startTimestamp, bytes2 ct_sec, bytes2 ct_min, bytes2 ct_hour, bytes2 ct_day, bytes2 ct_month, bytes2 ct_weekday, bytes2 ct_year) auth external { | ||
| require(repeat > 0); | ||
| uint pid = _newPayment(token, amount, to, startTimestamp); | ||
| _setPaymentSchedual(pid, repeat, ct_sec, ct_min, ct_hour, ct_day, ct_month, ct_weekday, ct_year); |
There was a problem hiding this comment.
If a payment can be withdrawn on creation, we should make it happen so only one transaction is needed for individual payments.
There was a problem hiding this comment.
So you mean if repeat=1 then do the token.transfer() immediately?
There was a problem hiding this comment.
Actually if a transaction can happen at the moment of payment creation, we should execute the withdrawal.
| bytes2 ct_year; | ||
| uint startBlock; | ||
| uint startTimestamp; | ||
| uint endTimestamp; |
There was a problem hiding this comment.
By not specifying the uint size the compiler will default to using uint256 for timestamps and block numbers that could be stored in uint64.
| } | ||
|
|
||
| /** | ||
| @param vaultAddress The vault app to use with this FinanceApp |
There was a problem hiding this comment.
Add a * before the comment line for conformance with the spec.
/**
* @param ...
*/
|
Closed in favor of #118 |
