Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
sc-14491 Part 2: Updates GDS endpoints #1008
sc-14491 Part 2: Updates GDS endpoints #1008
Changes from 15 commits
a0718b2
e3f4493
4bb2818
2b8f4d0
f9b1869
db6ab51
2b25080
7b21928
9666d28
cf11794
dee63da
8d8b7b8
84093d7
cc78dc4
a2e888c
c3f8b00
7ea0880
d987afb
98e1991
bee5b60
8fdf165
040ff25
729c2e9
05b8a5d
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that ideally we want to do the logging as close to the handlers as possible; I realize that the existing method also did logging here, so maybe we should just create a follow on story to remove the logging in this file and push it into the handlers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: we probably want to distinguish between "not found" errors and all other database errors. In the "not found" case we want to do what you're doing here, create the contact since the store told us it doesn't exist. In the second case we actually aren't sure if the contact exists, it's possible that kubernetes bounced the trtl pod or something. In that case we just want to return a status error aborted to avoid overwriting an existing contact.
We should be able to do
if errors.Is(err, storeerrors.ErrEntityNotFound)
(may have to import the package above) to check for the not found error, and more generally we should be doing this for all thes.db.Retrieve
error handling. However, it's possible that other places in the code don't have this handling because this code is old.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we revert this message back now, since we are potentially sending multiple emails again?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok I think the only place we should be setting
found = true
is on line 539. We should be returning not found if the user supplies a token but it doesn't match any contact's verification token on the VASP. In other words, it's not possible to verify a contact twice. Note that this also means we need to send an empty string for the token on line 530 below, since verified contacts should no longer have a verification token.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we remove the
found = true
or is that causing issues?