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

feat:Adding ability to send back picture of Id document #90

Closed

Conversation

0xtuytuy
Copy link

@0xtuytuy 0xtuytuy commented Jan 6, 2023

In order to prevent fraud, most identity cards worldwide include anti-forgery specifications on the back.

As a result, many Know Your Customer (KYC) providers require or allow the submission of the back of the identity card. To accommodate this requirement, we should support the submission of the back of identity cards as an optional field.

It should be noted that not all identity documents, such as passports, require a back image to be submitted.

@0xtuytuy 0xtuytuy force-pushed the add-option-to-send-back-of-image branch from 4717c75 to a41a34b Compare January 10, 2023 13:18
Copy link
Contributor

@cajubelt cajubelt left a comment

Choose a reason for hiding this comment

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

once the email field is required I'll approve, looks good

fiatconnect-api.md Outdated Show resolved Hide resolved
fiatconnect-api.md Outdated Show resolved Hide resolved
fiatconnect-api.md Outdated Show resolved Hide resolved
fiatconnect-api.md Outdated Show resolved Hide resolved
cajubelt
cajubelt previously approved these changes Jan 23, 2023
Copy link
Contributor

@cajubelt cajubelt left a comment

Choose a reason for hiding this comment

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

🥳

Copy link
Contributor

@jophish jophish left a comment

Choose a reason for hiding this comment

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

No issues with the actual content of the PR, just a few nits and a comment about creating an independent IdentificationDocumentType enum in the specification itself.

fiatconnect-api.md Outdated Show resolved Hide resolved
fiatconnect-api.md Outdated Show resolved Hide resolved
fiatconnect-api.md Outdated Show resolved Hide resolved
fiatconnect-api.md Outdated Show resolved Hide resolved
fiatconnect-api.md Outdated Show resolved Hide resolved
Copy link
Contributor

@jophish jophish left a comment

Choose a reason for hiding this comment

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

Please also add sections 9.3.1.2. and 9.3.1.2.1. to the Table of Contents at the beginning of the specification.

fiatconnect-api.md Show resolved Hide resolved
fiatconnect-api.md Outdated Show resolved Hide resolved
IdentificationTypeEnum and
edited the table of content +
some formating
@0xtuytuy
Copy link
Author

@jophish just updated the PR with all your comments, let me know your thoughts.

fiatconnect-api.md Outdated Show resolved Hide resolved
@@ -2227,12 +2230,67 @@ A KYC schema containing personal data about a user, as well as documents such as
},
phoneNumber: `string`,
selfieDocument: `string`,
identificationDocument: `string`
identificationDocument: `string`
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: indentation (I don't think I can add tab characters to suggestions)

Copy link
Author

Choose a reason for hiding this comment

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

Should be fixed, the file is not consistent as a whole though, sometime there is use of tab, sometime there is double space:
image

and

image

fiatconnect-api.md Outdated Show resolved Hide resolved
fiatconnect-api.md Outdated Show resolved Hide resolved
fiatconnect-api.md Outdated Show resolved Hide resolved
fiatconnect-api.md Outdated Show resolved Hide resolved
fiatconnect-api.md Outdated Show resolved Hide resolved
@cajubelt cajubelt dismissed their stale review January 25, 2023 18:40

joe requested necessary changes

@0xtuytuy
Copy link
Author

@jophish @cajubelt should be ready if we agree on how to handle white spaces / tabs

@cajubelt
Copy link
Contributor

in an effort to shorten the cycle time Joe opened his own version of this to address his own comments #92

@cajubelt cajubelt closed this Jan 30, 2023
jophish added a commit that referenced this pull request Jan 30, 2023
…nges) (#92)

See #90; this is just a new PR with minor styling/syntax changes applied
to expedite the process of getting the content merged. (I tried using
`git format-patch` and `git am` but got frustrated and gave up
:sweat_smile:)
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

4 participants