Skip to content
This repository was archived by the owner on Jul 6, 2022. It is now read-only.

fix #49 (Multiple KYC Providers) -> TBD in V2 #91

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

satyamakgec
Copy link
Contributor

I didn't re-write the test cases yet because of the high chance of modification in the logic. Please review it once with the logic. after approval, I will rewrite the test cases.

@adamdossa
Copy link
Contributor

Bit confused by the function function addToWhitelist(address _KYC) - this now adds msg.sender to the whitelist, rather than an address supplied as a parameter. Is this intentional? If so, please could you explain this bit of the logic a bit more.

@satyamakgec
Copy link
Contributor Author

Yes, I try to change the concept now the customer can whitelist themselves. It will make the process easy and more investor-oriented. if an owner can whitelist, the owner/issuer needs the KYC address of the investor and the investor address to a call the addToWhiteList(address _whitelistedAddress, address _KYC). Provide your suggestion which way we have to go the current one or the owner based.

@pabloruiz55
Copy link
Contributor

Yes, I try to change the concept now the customer can whitelist themselves. It will make the process easy and more investor-oriented.

Unfortunately that doesn't make much sense. The only person that should be able to add someone to the whitelist is the Issuer.
Even if the KYC provider verified someone, the issuer might have their own set of rules or reasons for not getting someone into the whitelist.

@satyamakgec
Copy link
Contributor Author

ok fair enough, So we are final that only issuer can call addToWhiteList(address _whiteListAddress, address _kyc) function.
any more Suggestions for change @pabloruiz55 @adamdossa

@sangheraio
Copy link
Contributor

Yeah agree with @pabloruiz55 , we need to remove KYC provider from adding to whitelist/blacklist on a security token. They can build their mapping of user verifications all they want, but should be up to the issuer to add them in.

function addToWhitelist(address _whitelistAddress) public returns (bool success) {
require(KYC == msg.sender || owner == msg.sender);
var (jurisdiction, accredited, role, verified, expires) = PolyCustomers.getCustomer(KYC, _whitelistAddress);
function addToWhitelist(address _KYC) public returns (bool success) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is missing what we discussed yesterday about having the issuer be the only one that can add addresses to the whitelist.

* @return bool
*/

function addAllowedKYC(address[10] _whiteListedKYC) internal returns(bool) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How would a new KYC provider be added later on?

* @param _whiteListedKYC Array of permitted providers.
* @return bool
*/

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we are missing a function to allow the issuer to disable/reenable a KYC provider if they want to stop using one of them. If a KYC provider is disabled, then they can't be used to verify new investors.
We need to figure out what we would do with addresses already verified by a KYC provider if it gets disabled.

Copy link
Contributor Author

@satyamakgec satyamakgec Jan 25, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the particular template is used by one securitytoken only then we can add the functionality of adding more KYC providers and disable them in the Template contract. otherwise, we can maintain their allowedKYC mapping in the security token itself. But I think it doesn't have the sense to use the whitelistedKYC array in the first place(Template Constructor).

I am not sure whether the template is used by more than one securityToken or used by one only.
if we can confirm the above line then I can re-design accordingly

@pabloruiz55
Copy link
Contributor

Leaving this PR for V2

@pabloruiz55 pabloruiz55 changed the title fix #49 fix #49 (Multiple KYC Providers) -> TBD in V2 Jan 26, 2018
@pabloruiz55 pabloruiz55 added this to the V2 milestone Jan 29, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants