search

TokenMarketNet / smart-contracts

Ethereum smart contracts for security and utility tokens
https://tokenmarket.net/
Other
1.34k stars 561 forks source link

Feat/security token cleanup #166

Closed villesundell closed 5 years ago

voith commented 5 years ago

@ville There were some test failures. I have re-triggered the build to see if it can be fixed.

Code-wise, the cleanup looks good to me, but I cannot approve the PR, till the tests pass.

Since you're cleaning up code, there are two files that I would like see moved to a new location. PayoutContract.sol and VotingContract.sol are currently located under the tests folder. Do you think it would be a good idea to move these to the contracts directory?

villesundell commented 5 years ago

Yeah, the "push" test was successful, but the "pr" test is failing. It seems to fail at "buyWithKYCData", which is outside of the scope of this PR, maybe this is a Travis problem?

About the files in contracts/security-token/tests: I have used it for proof-of-concept contracts which are not production ready yet (and not currently needed in production). I think there should be a clear differentiation between proof-of-concepts (and implementation suggestions) and production ready code. There could be a directory for proof of concepts, but we don't really know yet how to deal with auxiliary contracts in the future (should there be a different repo for those, for instance?)

voith commented 5 years ago

About the files in contracts/security-token/tests: I have used it for proof-of-concept contracts which are not production ready yet (and not currently needed in production). I think there should be a clear differentiation between proof-of-concepts (and implementation suggestions) and production ready code. There could be a directory for proof of concepts, but we don't really know yet how to deal with auxiliary contracts in the future (should there be a different repo for those, for instance?)

The only reason why I asked was because we earlier tried to implement Payout's feature in the sto repo on-chain. However, it was changed later to off-chain. Since you had advised me to use the Payout contract to implement the feature, I assumed that it was production ready. Anyway, I don't have any objections against merging this PR. My only concern was that production ready code shouldn't reside under tests. But now that you're specifically mentioned that it's a POC, I have no issues.

villesundell commented 5 years ago

Thanks :slightly_smiling_face: Yep, the original plan was to finalize the Payout contract at the same time with STO Tool's on-chain payout, but priorities changed, and the Payout contract was left to tests/ unfinalized.

Payout contract should be fine, but since no extra effort have been paid to finalize and polish it, I think it should be in tests/ for now, and treat it more as an implementation proposal :slightly_smiling_face:

@miohtama Btw, should we place the auxiliary contracts somewhere else (different directory, or even a repo, since these will grow), and dedicate the security-token/ for the core token only?