Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

(Feature) Validate imported whitelist addresses #659

Merged
merged 2 commits into from
Mar 12, 2018

Conversation

fvictorio
Copy link
Contributor

Depends on #656. Closes #641.

@vbaranov
Copy link
Collaborator

@fvictorio could you clarify which types of validations we cover with this PR? do we need more validations, or it covers all possible validations?

@fvictorio
Copy link
Contributor Author

fvictorio commented Mar 12, 2018

It performs the same validations that are done when you type an address in the input (it just uses Web3.util.isAddress).

@vbaranov
Copy link
Collaborator

Ok, I see. The issue is that when we import an address with individual min cap > individual max cap there is no validation for now. will it be resolved in the frame of #578 ?

@fvictorio
Copy link
Contributor Author

Oh, no, the min an max cap are not being validated (it just checks that they are numbers). We should think of a way to have those validations in a single place and use that both when importing a CSV and when typing an address.

Do you think we should do that as part of this PR?

@vbaranov
Copy link
Collaborator

Do you think we should do that as part of this PR?

No. I think we could merge it. I mentioned here just to not forget to do this as a part of #578 or as a separate issue. @fvictorio please create a new issue if you prefer to do it on a separate one.

@vbaranov vbaranov merged commit 3e31b35 into master Mar 12, 2018
@ghost ghost removed the awaiting for review label Mar 12, 2018
@vbaranov vbaranov deleted the validate-imported-whitelist-addresses-#641 branch March 12, 2018 13:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants