Skip to content
This repository has been archived by the owner on Jan 14, 2020. It is now read-only.

Attestation service responses and exceptions #48

Merged
merged 19 commits into from
May 10, 2018

Conversation

ambertch
Copy link
Contributor

@ambertch ambertch commented May 5, 2018

Checklist:

  • Code contains relevant tests for the problem you are solving
  • Submit to the develop branch instead of master
  • Ensure all new and existing tests pass
  • Update any relevant READMEs and docs

Description:

Please explain the changes you made here:

Implements #29, #33, #42, #40:

  • standard return type for VerificationService methods
  • separate exceptions for each claim type
  • exception handling for Twilio and Sendgrid API calls
  • phone numbers are validated using the Twilio lookup API (supports both US and international numbers)

In addition,

  • handling of web3 account exceptions (for when an invalid account is passed in)
  • use web3 auto-initialization off of environment variable (web3 provider URI added to dotenv file)
  • request handler outputs a standard format for errors (array of strings, 422 status code)

@ambertch
Copy link
Contributor Author

ambertch commented May 5, 2018

One inconsistency with develop (which probably should be resolved before this PR is merged) is that #29 was implemented with exceptions having an string rather than dictionary value, so handle_request now returns two error responses:

  1. In the event of an exception being raised in VerificationService (an array of strings): 980d618#diff-c6a86d88d4862dc3dc261772dc68e5daR41)
  2. In the event of a validation error from Marshmallow (an array of dictionaries): https://github.com/OriginProtocol/bridge-server/blob/develop/api/helpers.py#L26

A dapp developer calling the attestation service endpoints will get a different error response format returned depending on whether they made the error-causing request with bad params, or the request hit an exception.

I wasn't sure if the [code/path/message] format that was originally implemented was meant for filtering on, and if it's ok to use string error messages (the errors could also be filtered based on class type).

If so, returning the errors as strings would change a request that fails Marshmallow validation from

{
    "errors": [
        {
            "code": "INVALID_REQUEST",
            "path": "code",
            "message": "Missing data for required field."
        },
        {
            "code": "INVALID_REQUEST",
            "path": "email",
            "message": "Not a valid email address."
        },
        {
            "code": "INVALID_REQUEST",
            "path": "identity",
            "message": "Not a valid string."
        }
    ]
}

to something like

{
    "errors": ["code - Missing data for required field.",        
               "email - Not a valid email address.",
               "identity - Not a valid string."
    ]
}

Feedback appreciated!

@ambertch ambertch requested review from tyleryasaka and Gzing May 5, 2018 04:19
Copy link
Contributor

@tyleryasaka tyleryasaka left a comment

Choose a reason for hiding this comment

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

@ambertch Looks great!

I think this simpler error format is better. We can merge this as-is - the other repos are not dependent on the current structure of error responses.

One request: can you update the README to reflect the new error response format?

@ambertch
Copy link
Contributor Author

ambertch commented May 8, 2018

api/README has been updated

@tyleryasaka
Copy link
Contributor

@ambertch Looking good! I can approve once the conflicts have been resolved.

@@ -40,6 +40,13 @@

CODE_EXPIRATION_TIME_MINUTES = 30

CLAIM_TYPES = {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice ⚡️

@tyleryasaka
Copy link
Contributor

Thanks for all the work here - it's looking good. Tomorrow I'm going to resolve the conflicts in this PR and then make sure everything works with the rest of the stack - origin-js and demo-dapp. That's a to-do for me. Sorry for the delay on this.

Copy link
Contributor

@tyleryasaka tyleryasaka left a comment

Choose a reason for hiding this comment

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

@ambertch Just merged in the develop branch. Looks good to me!

@joshfraser joshfraser merged commit 1c3f7f0 into develop May 10, 2018
@joshfraser joshfraser deleted the attestation-service-responses-and-exceptions branch May 10, 2018 06:04
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants