Initial/draft implementation of token rules.#161
Initial/draft implementation of token rules.#161pgev wants to merge 2 commits intoOpenSTFoundation:feature/token-holderfrom
Conversation
jasonklein
left a comment
jasonklein
left a comment
There was a problem hiding this comment.
A few comments—mostly about design choices that are probably OK, but of which I want to make sure we understand the implications.
Two global comments:
- I mostly did not comment on style because I wasn't sure that makes sense given the evident inchoate state, but to the extent you know that (a) there are inconsistencies with the style guide that are not only because more work is forthcoming (e.g., spacing, function documentation, etc.) and (b) there are decisions in here that are not spoken to in the style guide (e.g., doxygen comments that are not part of Ethereum's Natspec), please correct/update this code or the style guide as relevant; and
- where decisions reflect assumptions about what will or may be true in the future, please provide comments so it is clear what is intended (e.g., how constraints will be used, why the token rules are in a mapping where key and value are the same, etc.) and how to move forward when the future is now 🤖
contracts/TokenRules.sol
Outdated
| } | ||
|
|
||
| /** | ||
| * @param _rule The address of rule to add. |
contracts/TokenRules.sol
Outdated
| SharedStructs.TokenRuleTransfer[] _transfers | ||
| ) | ||
| external | ||
| view |
There was a problem hiding this comment.
I think that, because this calls non-view functions (transferFrom, clearAllowance), it is not a view function.
contracts/TokenRules.sol
Outdated
|
|
||
| require ( | ||
| checkConstraints(_from, _transfers), | ||
| "Constraints do not fullfilled." |
| "Constraints do not fullfilled." | ||
| ); | ||
|
|
||
| for (uint256 i = 0; i < _transfers.length; ++i) { |
There was a problem hiding this comment.
I believe best practice advises avoiding usage of loops in this manner. Should we be concerned about gas here?
contracts/TokenRules.sol
Outdated
| ); | ||
| } | ||
|
|
||
| brandedToken.clearAllowance(_from); |
There was a problem hiding this comment.
This appears to be duplicative with TokenHolder: https://github.com/OpenSTFoundation/openst-contracts/pull/160/files#diff-65cbb9bc9658dc2fe0c2f217fa670020R339
As the approach in TokenHolder is consistent with EIP20, I'd vote for that one; but, ultimately, my primary concern is eliminating the duplication, so please come to a view with @abhayks1 and @gulshanvasnani.
| "Index is out of range." | ||
| ); | ||
|
|
||
| while ( _index < constraints.length - 1) { |
There was a problem hiding this comment.
And still the same question about loops :-)
| SharedStructs.TokenRuleTransfer[] _transfers | ||
| ) | ||
| public | ||
| view |
There was a problem hiding this comment.
Would it make sense to mark this as pure? Or no because it is clearly a work in progress?
| return true; | ||
| } | ||
|
|
||
| function clearAllowance(address _approver) |
There was a problem hiding this comment.
If resetting the allowance to 0 is kept in TokenHolder, then this change should be struck.
| address ruleAddress; | ||
| } | ||
|
|
||
| struct TokenRuleConstraint { |
There was a problem hiding this comment.
Is the idea behind a constraint to give the flexibility of having global constraints that apply to one or more (or every?) token rule but that is not reflected in any individual token rule?
- if yes:
- I'd argue this is an enhancement that may not be apposite for a first complete iteration.
- but if the decision is to retain the concept all the same, I suggest renaming to "TokenRulesConstraint".
- if no—each constraint will be specific to a token rule—then I suggest reflecting that in the token rule, and not in the token rules
| @@ -0,0 +1,264 @@ | |||
| pragma solidity ^0.4.24; | |||
| pragma experimental ABIEncoderV2; | |||
There was a problem hiding this comment.
As discussed, considering that experimental features may likely still be experimental by the time we intend to have these concepts on Mainnet, please make the necessary changes to not need to rely on those features.
| ) | ||
| public | ||
| { | ||
| organization = _organization; |
There was a problem hiding this comment.
Should these inputs be validated?
| */ | ||
| contract BrandedToken is EIP20Token, UtilityTokenAbstract, Internal { | ||
| contract BrandedToken | ||
| is EIP20Token, UtilityTokenAbstract, TokenRulesTokenInterface, Internal { |
There was a problem hiding this comment.
TokenRulesTokenInterface can be renamed AllowanceClearable ?
3 participants
No description provided.