krystianity / node-bitstamp

bitstamp REST and WS API Node.js client :dollar:
MIT License
59 stars 41 forks source link

improper handling of errors #6

Closed dutu closed 6 years ago

dutu commented 6 years ago

For some reason Bitstamps returns the error below:

{
  "status": 403,
  "headers": { },
  "body": {
    "status": "error",
    "reason": "Invalid signature",
    "code": "API0005"
  }
}

It is expected to have the result returned as an error and be passed to catch, however it is resolved as successful and passed to then

krystianity commented 6 years ago

Hi @dutu that is not the goal of this API, the promise will only reject when there is a network or configuration error in the http request itself, I am always returning {status, headers, body} - meaning you will have to check the status code yourself.

dutu commented 6 years ago

Hi @krystianity thank you for your reply.

I would expect the module to handle all errors and return resolve with valid API data when no error has occurred and reject when an error occurred. This is exactly what I would expect an API wrapper to do, not to return raw data from an http request.

But ok, up to you - it just makes the library difficult to use over other available implementations such as https://github.com/askmike/bitstamp

Feel free to close this issue as you consider

krystianity commented 6 years ago

True, depends on the high and low level accessibility of the API. I might add an option to run it in a higher level mode, lets see.

tuloski commented 6 years ago

I agree with @dutu. And it shouldn't be that complex, just check the body for the "error" field and reject the promise. But it's usable also as it is now.

tuloski commented 6 years ago

For @dutu, you can have what you want, by wrapping each function to a new promise such as:

async function get_bitstamp_balance(){
    let res = await privateBit_node.balance();
    return new Promise((resolve, reject) => {
        var error = bitstamp_check_errors(res);
        if (!error){
            return resolve(res.body);
        } else {
            return reject(error);
        }
    }); 
}

function bitstamp_check_errors(res){
    if (!res.body.hasOwnProperty('status')){
        //Success
        return null;
    } else {
        if (res.body.status == 'error'){
            return res.body.reason;
        } else {
            //Good
            return null;
        }
    }
}

I'm very noob in JS, so maybe that is not recommended or the best way but it works, because in that way you have a promise which rejects in case of error inside the "body".

nicolasgarnier commented 6 years ago

I agree, normally a Promise should resolve only if the call was successful. e.g. when asking for a Withdrawal the promise should resolve only of the withdrawal has succeeded and reject in all other cases. Developers can catch the error states in the catch method of the promise.