Closed tspoff closed 5 years ago
From the smart contract, I see ETH is not accepted as a payment method. I think that helps keep the authenticity of the loan agreement, and makes the loans not attractive for arbitrage seekers. But, I think we could have more than one token accepted as payment method, but I can see how that adds complexity, especially since we would like to create a MVP first.
I'll look into adding tests, on the contract, and any optimization that comes to mind
I see we do not really have contract logic yet, What do you have in mind?
I can see adding more than one token being pretty straitforward, we're just trading ownership tokens for ERC20 transfers at a certain rate per token, but I agree that getting the MVP out first is more important.
Agreed with removing functions, I just started hacking on top of the openzeppelin crowdsale. I actually had to make some changes to the base contract to handle token payments rather than either, so the whole clean inheritance from existing libraries is kind of moot.
No particular reason for that created event, it's a holdover from before I wrote the factory. Late night hacking :)
One thing I'm interested in discussing is the repayments array - where the required payment for each pay period is preloaded into the contract. I wanted to minimize calcuation on-chain, but on-chain may be the way to go as opposed to using all this storage, especially as we've settled on a simple type of loan.
I dealt with something similar to this in my Crypto-will product: https://github.com/adibas03/crypto-will https://github.com/adibas03/crypto-will/blob/master/contracts/Will.sol#L178 https://github.com/adibas03/crypto-will/blob/master/contracts/Will.sol#L67
In the end, I resolved to doing a minimalist add
and divide
calculation on chain to ensure errors or intentional skips were accounted for. But, to make this work, we need to store the nextRepayment
which will be a uint
of the index of the array to be paid next.
I will code this up, and we can optimize, for omissions and unnecessary additions.
Also, looking through the updates: https://github.com/onggunhao/enable/blob/loan-tenor-units/contracts/LoanRequest.sol#L22
Do we really want TenorsUnits, when we can just make our calculations in seconds, and that means we do not need to do any more calculation, when trying to check for if repayment is due. Using tenors, mean we have to account for the selected tenor. We can always do the Tenor calculation on the fronted using libraries like moment.js
without TenorUnits
struct Loan {
...
uint private nextRepayment;
uint private repaymentTenor; //Repayment tenor in seconds
}
function internal _getRepaymentTimestamp (uint repayment) returns (uint) {
return (repayment+1) * loan.repaymentTenor;
}
isRepaymentDue () {
...
return now >= _getRepaymentTimestamp(loan.nextRepayment);
}
with TenorUnits
struct Loan {
...
uint private nextRepayment;
uint private repaymentTenor; //Repayment tenor in TenorUnits
TenorUnits tenorUnit
}
enum TenorUnits {
DAYS, WEEKS, MONTHS, YEARS
}
function internal _getRepaymentTimestamp (uint repayment) returns (uint) {
uint multiplier;
if (loan.tenorUnit == TenorUnits.DAYS) {
multiplier = 86400;// seconds in a day: '1 days' is also supported by solidity
} else if (loan.tenorUnit == TenorUnits.WEEKS) {
multiplier = 604800;// seconds in a week: '1 weeks' is also supported by solidity
} else if (loan.tenorUnit == TenorUnits.MONTHS) {
multiplier = 2592000;// seconds in a '30 day' month: Not supported by solidity
} else if (loan.tenorUnit == TenorUnits.YEARS) {
multiplier = 31536000;// seconds in a year: Not supported by solidity
}
return (repayment+1) * (loan.repaymentTenor * multiplier);
}
isRepaymentDue () {
...
return now >= _getRepaymentTimestamp(nextRepayment);
}
@tspoff mini repayment logic implemented: #22
Removing the tenor units sounds good to me.
Archiving discussion
The loan contract is essentially an extension of openzeppelin crowdsale contracts that takes in a loan token rather than either.
Next work: