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

Resolve [ErrorException: Undefined property] - Return changelly errors #3

Merged

Conversation

mattsmithies
Copy link
Contributor

Rationale

The Changelly API returns an error if a request is invalid in particular when trying to request getExchangeAmount. For example when there isn't enough liquidity or there is a minimum amount required for an exchange to take place.

Solution

I've updated src/Changelly.php to check that a result exists from the Changelly API service, if not it reverts to using the error which is returned. The sample code has been updated to illustrate the update.

If you are happy with this PR feel free to merge and update your composer package.

Copy link
Owner

@vittominacori vittominacori left a comment

Choose a reason for hiding this comment

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

What do you think about throwing error instead?

if ($response->code == 200) {
    $body = $response->body;

    if (isset($body->result)) {
        return $body->result;
    }

    throw new \Exception($body->error->message, 400);
} else {
    throw new \Exception($response->body, $response->code);
}

And intercept it in a try catch?

// Throw error when Changelly cannot process the request
try {
    echo json_encode($changelly->getExchangeAmount('btc', 'eth', 22000000)) . PHP_EOL;
} catch (Exception $e) {
    echo $e->getMessage();
}

Otherwise you should check that returned value must not to be an error message.

@vittominacori vittominacori self-assigned this Apr 28, 2020
@mattsmithies
Copy link
Contributor Author

It would make more sense to use 422 (Unprocessable Entity) and include the full JSON error, as Changelly as provides helpful insight to resolve the situation.

If this was a normal situation I'd stay away from throwing an exception as the design of the Changelly API considers these "warnings" to be a normal part of the exchange flow.

Given that I don't know how many people are using this specific client it is a good compromise to throw an actual exception without breaking anyone's code!

@vittominacori vittominacori merged commit fda49c8 into vittominacori:master Apr 28, 2020
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

2 participants