[WIP] Credit #198
[WIP] Credit #198
Conversation
…ls to TokenRules.
|
Started reviewing this. It's in "Review/QA" column. |
| @@ -33,7 +35,7 @@ import "../organization/Organized.sol"; | |||
| * During a execution, rule can call TokenRules.executeTransfers() | |||
| * function only once. | |||
| */ | |||
| contract TokenRules is Organized { | |||
| contract TokenRules is Organized, TransfersAgent { | |||
There was a problem hiding this comment.
Since TokenRules contract is updated with TransfersAgent inheritance, is it needed to redeploy new TokenRules contract for an economy?
| // limitations under the License. | ||
|
|
||
|
|
||
| interface TransfersAgent { |
There was a problem hiding this comment.
"title" for the interface is missing.
| const budgetHolderSessionKeyExpirationDelta = 300; | ||
|
|
||
| await TokenHolderUtils.authorizeSessionKey( | ||
| budgetHolder, budgetHolderOwnerAddress, |
There was a problem hiding this comment.
budgetHolderOwnerAddress should go to new line. Please check once if there are other instances like this.
| const tokenHolderSessionKeyExpirationDelta = 200; | ||
|
|
||
| await TokenHolderUtils.authorizeSessionKey( | ||
| tokenHolder, tokenHolderOwnerAddress, |
There was a problem hiding this comment.
tokenHolderOwnerAddress should go to new line.
|
|
||
| struct CreditInfo { | ||
| uint256 amount; | ||
| bool inProgress; |
There was a problem hiding this comment.
Should we call inProgress to isAllocated/allocated ? I believe amount is the allocated amount to user.
Any specific reason to keep inProgress naming?
| @@ -0,0 +1,69 @@ | |||
| // Copyright 2018 OpenST Ltd. | |||
There was a problem hiding this comment.
Update the copyright with
Copyright 2019 OpenST Ltd.
Generic Comment applicable to other test files also.
| ); | ||
| }); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
CreditRule.executeRule unit test cases are missing. Adding this comment here as I wasn't sure where to comment.
| assert.isOk( | ||
| actualTransfersAmountLength.eqn(2), | ||
| ); | ||
|
|
||
| assert.isOk( | ||
| transferredAmount1.eq(expectedTransferAmount1), | ||
| ); |
There was a problem hiding this comment.
I think we should also introduce integration tests in openst-contracts like we have in mosaic-contracts.
CreditRule with CustomRule is a good starting point 🚗 . It could be a separate ticket 💭
|
|
||
| /* Storage */ | ||
|
|
||
| CreditRule public creditRule; |
There was a problem hiding this comment.
Should we introduce CreditRuleInterface contract for rules contract to integrate ❓
|
|
||
| TransfersAgent public transfersAgent; | ||
|
|
||
| mapping(address => CreditInfo) public credits; |
There was a problem hiding this comment.
Do we need to handle use case where credits for a user are only valid for a particular duration or blockheight? 💭
I might not be aware of scope of the project ⏲
No description provided.