25 mBTC Bounty - Refactor xchange-bitstamp module #279

Closed
timmolter opened this Issue Jan 28, 2014 · 14 comments

Comments

Projects
None yet
3 participants
@timmolter
Member

timmolter commented Jan 28, 2014

Rules for bounties and the explanation of the refactor can be found here: #270

Classes that need to be split:

BitstampAccountService
BitstampMarketDataService
BitstampTradeService

Relevant example classes should also be updated to show both raw and generic polling functions

@gnandiga

This comment has been minimized.

Show comment
Hide comment
@gnandiga

gnandiga Feb 5, 2014

I will be working on this.

gnandiga commented Feb 5, 2014

I will be working on this.

@gnandiga

This comment has been minimized.

Show comment
Hide comment
@gnandiga

gnandiga Feb 6, 2014

I have started to work on BitstampAccountService

  1. What do you guys think about naming these classes as BitStampAccountService(camelcase at "S" in BitStamp). If this is something you would like I will change all classes. This seems more inline with site.
  2. As per the documentation, https://www.bitstamp.net/api/ : //bitstamp.net/api/withdrawal_requests, returns a boolean, and hence I think the raw service should return a boolean. However the PollingAccountService expects to return a transactionId. what should be done in that scenario. As per the present implementation, it was returning "true".

Let me know what should be done here?

gnandiga commented Feb 6, 2014

I have started to work on BitstampAccountService

  1. What do you guys think about naming these classes as BitStampAccountService(camelcase at "S" in BitStamp). If this is something you would like I will change all classes. This seems more inline with site.
  2. As per the documentation, https://www.bitstamp.net/api/ : //bitstamp.net/api/withdrawal_requests, returns a boolean, and hence I think the raw service should return a boolean. However the PollingAccountService expects to return a transactionId. what should be done in that scenario. As per the present implementation, it was returning "true".

Let me know what should be done here?

@gnandiga

This comment has been minimized.

Show comment
Hide comment
@gnandiga

gnandiga Feb 6, 2014

With present credentials in bitStamp demo's I am getting "User not verified". Any idea how I should continue?

gnandiga commented Feb 6, 2014

With present credentials in bitStamp demo's I am getting "User not verified". Any idea how I should continue?

gnandiga pushed a commit to gnandiga/XChange that referenced this issue Feb 6, 2014

gnandiga pushed a commit to gnandiga/XChange that referenced this issue Feb 6, 2014

@jamespedwards42

This comment has been minimized.

Show comment
Hide comment
@jamespedwards42

jamespedwards42 Feb 6, 2014

Collaborator

My Votes:

When they write about Bitstamp on the site it is considered one word (https://www.bitstamp.net/news/), so I think it should be left as is.

The raw service should return a boolean true and the generic service should continue to return "true".

Do all authenticated api calls require a verified account?...After your done with the demos, just let me know which ones need it and I can test them with my verified account. Also, I don't remember it being too much trouble to get a verified account, just a photo id and proof of residence.

Collaborator

jamespedwards42 commented Feb 6, 2014

My Votes:

When they write about Bitstamp on the site it is considered one word (https://www.bitstamp.net/news/), so I think it should be left as is.

The raw service should return a boolean true and the generic service should continue to return "true".

Do all authenticated api calls require a verified account?...After your done with the demos, just let me know which ones need it and I can test them with my verified account. Also, I don't remember it being too much trouble to get a verified account, just a photo id and proof of residence.

@gnandiga

This comment has been minimized.

Show comment
Hide comment
@gnandiga

gnandiga Feb 6, 2014

Can you give me an example JSON response for https://www.bitstamp.net/api/bitcoin_withdrawal/

I created a BitstampGenericResponse as, we were not parsing the error that was sent in the body and throwing a JSON parse exception.

I want to create associated response class.

gnandiga commented Feb 6, 2014

Can you give me an example JSON response for https://www.bitstamp.net/api/bitcoin_withdrawal/

I created a BitstampGenericResponse as, we were not parsing the error that was sent in the body and throwing a JSON parse exception.

I want to create associated response class.

gnandiga pushed a commit to gnandiga/XChange that referenced this issue Feb 6, 2014

@jamespedwards42

This comment has been minimized.

Show comment
Hide comment
@jamespedwards42

jamespedwards42 Feb 6, 2014

Collaborator

Looks like it just returns true/false:

21:00:27.755 [default] [main] DEBUG si.mazi.rescu.HttpTemplate - Executing POST request at https://www.bitstamp.net/api/bitcoin_withdrawal/
21:00:27.756 [default] [main] TRACE si.mazi.rescu.HttpTemplate - Request body = key=&signature=&nonce=1391662827753&amount=0.001&address=1346rkXduVvQYUM8DiqWfvHgsRcFktbbRr
21:00:27.756 [default] [main] TRACE si.mazi.rescu.HttpTemplate - Request headers = {}
21:00:27.756 [default] [main] TRACE si.mazi.rescu.HttpTemplate - Header request property: key='Accept-Charset', value='UTF-8'
21:00:27.756 [default] [main] TRACE si.mazi.rescu.HttpTemplate - Header request property: key='User-Agent', value='ResCU JDK/6 AppleWebKit/535.7 Chrome/16.0.912.36 Safari/535.7'
21:00:27.756 [default] [main] TRACE si.mazi.rescu.HttpTemplate - Header request property: key='Content-Type', value='application/x-www-form-urlencoded'
21:00:27.756 [default] [main] TRACE si.mazi.rescu.HttpTemplate - Header request property: key='Accept', value='application/json'
21:00:28.613 [default] [main] DEBUG si.mazi.rescu.HttpTemplate - Request http status = 200
21:00:28.614 [default] [main] TRACE si.mazi.rescu.HttpTemplate - Response body: true

Collaborator

jamespedwards42 commented Feb 6, 2014

Looks like it just returns true/false:

21:00:27.755 [default] [main] DEBUG si.mazi.rescu.HttpTemplate - Executing POST request at https://www.bitstamp.net/api/bitcoin_withdrawal/
21:00:27.756 [default] [main] TRACE si.mazi.rescu.HttpTemplate - Request body = key=&signature=&nonce=1391662827753&amount=0.001&address=1346rkXduVvQYUM8DiqWfvHgsRcFktbbRr
21:00:27.756 [default] [main] TRACE si.mazi.rescu.HttpTemplate - Request headers = {}
21:00:27.756 [default] [main] TRACE si.mazi.rescu.HttpTemplate - Header request property: key='Accept-Charset', value='UTF-8'
21:00:27.756 [default] [main] TRACE si.mazi.rescu.HttpTemplate - Header request property: key='User-Agent', value='ResCU JDK/6 AppleWebKit/535.7 Chrome/16.0.912.36 Safari/535.7'
21:00:27.756 [default] [main] TRACE si.mazi.rescu.HttpTemplate - Header request property: key='Content-Type', value='application/x-www-form-urlencoded'
21:00:27.756 [default] [main] TRACE si.mazi.rescu.HttpTemplate - Header request property: key='Accept', value='application/json'
21:00:28.613 [default] [main] DEBUG si.mazi.rescu.HttpTemplate - Request http status = 200
21:00:28.614 [default] [main] TRACE si.mazi.rescu.HttpTemplate - Response body: true

@gnandiga

This comment has been minimized.

Show comment
Hide comment
@gnandiga

gnandiga Feb 6, 2014

On successful crawl this returns true as response body
and on error it returns {"error" : "User not verified"}

I am leaning towards doing
-- Catch JSON parse exception in case of error, and throw a ExchangeException

do you have any other ideas?

gnandiga commented Feb 6, 2014

On successful crawl this returns true as response body
and on error it returns {"error" : "User not verified"}

I am leaning towards doing
-- Catch JSON parse exception in case of error, and throw a ExchangeException

do you have any other ideas?

@jamespedwards42

This comment has been minimized.

Show comment
Hide comment
@jamespedwards42

jamespedwards42 Feb 6, 2014

Collaborator

Does the parse exception return that same body string '"error": "User not verified"? If not I think it would be better to do a custom deserializer that returns something like BitstampCancelResponse or maybe just a Boolean and during deserialization check for an "error" entry and if it exists throw an ExchangeException with the message body.

see https://github.com/timmolter/XChange/blob/develop/xchange-kraken/src/main/java/com/xeiam/xchange/kraken/dto/marketdata/KrakenOrder.java to get an idea of how to implement a JsonDeserializer.

Collaborator

jamespedwards42 commented Feb 6, 2014

Does the parse exception return that same body string '"error": "User not verified"? If not I think it would be better to do a custom deserializer that returns something like BitstampCancelResponse or maybe just a Boolean and during deserialization check for an "error" entry and if it exists throw an ExchangeException with the message body.

see https://github.com/timmolter/XChange/blob/develop/xchange-kraken/src/main/java/com/xeiam/xchange/kraken/dto/marketdata/KrakenOrder.java to get an idea of how to implement a JsonDeserializer.

@gnandiga

This comment has been minimized.

Show comment
Hide comment
@gnandiga

gnandiga Feb 6, 2014

I get the idea now. I will do a similar implementation.

gnandiga commented Feb 6, 2014

I get the idea now. I will do a similar implementation.

@gnandiga

This comment has been minimized.

Show comment
Hide comment
@gnandiga

gnandiga Feb 6, 2014

Mistakenly pushed to wrong issue, and hence you are not seeing my checkin.

Here is the final checkin as per @jamespedwards42 comments.
gnandiga@448f9ee

Please code review and let me know.

gnandiga commented Feb 6, 2014

Mistakenly pushed to wrong issue, and hence you are not seeing my checkin.

Here is the final checkin as per @jamespedwards42 comments.
gnandiga@448f9ee

Please code review and let me know.

timmolter added a commit that referenced this issue Feb 7, 2014

Merge pull request #296 from gnandiga/develop
#279: Completed Refactoring Bitstamp services
@timmolter

This comment has been minimized.

Show comment
Hide comment
@timmolter

timmolter Feb 7, 2014

Member

@gnandiga For future reference, there is a code formatting file for the XChange project here: https://github.com/timmolter/XChange/wiki/New-Implementation-Best-Practices

Your code looks really good. I made some minor changes.

@jamespedwards42 Would you be willing to test the authenticated api calls?

Member

timmolter commented Feb 7, 2014

@gnandiga For future reference, there is a code formatting file for the XChange project here: https://github.com/timmolter/XChange/wiki/New-Implementation-Best-Practices

Your code looks really good. I made some minor changes.

@jamespedwards42 Would you be willing to test the authenticated api calls?

@timmolter

This comment has been minimized.

Show comment
Hide comment
@timmolter

timmolter Feb 8, 2014

Member

Bitcoin Address: 112LnfRzmA6AMU5i6SHPCGpjM3Ya5pBVLb as provided in pull request.

Member

timmolter commented Feb 8, 2014

Bitcoin Address: 112LnfRzmA6AMU5i6SHPCGpjM3Ya5pBVLb as provided in pull request.

@timmolter

This comment has been minimized.

Show comment
Hide comment
@timmolter

timmolter Feb 8, 2014

Member

Bounty paid to 112LnfRzmA6AMU5i6SHPCGpjM3Ya5pBVLb. Thanks for your work!

Member

timmolter commented Feb 8, 2014

Bounty paid to 112LnfRzmA6AMU5i6SHPCGpjM3Ya5pBVLb. Thanks for your work!

@timmolter timmolter closed this Feb 8, 2014

@gnandiga

This comment has been minimized.

Show comment
Hide comment
@gnandiga

gnandiga Feb 8, 2014

Thank you.

gnandiga commented Feb 8, 2014

Thank you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment