Updated getTransactions and added getPendingTransactions#17
Updated getTransactions and added getPendingTransactions#17JustinNothling wants to merge 6 commits intobausmeier:masterfrom
Conversation
|
@JustinNothling Thanks for the PR. It looks like the tests are failing because of your change - it would be awesome if you can update those to match your change. I'll take a closer look when I can find some time. |
|
@bausmeier i updated your tests, let me know if that fixes things? |
|
Ok, all tests are passing (except for "SyntaxError: Use of const in strict mode.") |
|
@JustinNothling Thanks, that test was failing before your change anyway. I haven't had a chance to look at your change but will try and find time this evening. |
bausmeier
left a comment
bausmeier
left a comment
There was a problem hiding this comment.
@JustinNothling The code mostly looks good. I just left a few minor comments. You also haven't added any tests for the new getPendingTransactions function that you added - please do that if you can.
lib/BitX.js
Outdated
| } | ||
|
|
||
| BitX.prototype.getTransactions = function (asset, options, callback) { | ||
| BitX.prototype.getTransactions = function (account_id, options, callback) { |
There was a problem hiding this comment.
Please could you name the account id parameter using camel case so that it's consistent with the rest of the code like:
Line 19 in 54fd76a
And please update the docs to match.
lib/BitX.js
Outdated
| this._request('GET', 'accounts/' + account_id + '/transactions', extend(defaults, options), callback) | ||
| } | ||
|
|
||
| BitX.prototype.getPendingTransactions = function (account_id, callback) { |
There was a problem hiding this comment.
Same comment as above applies here.
test/IntegrationTests.js
Outdated
| offset: 5, | ||
| limit: 5 | ||
| min_row: 5, | ||
| max_row: 5 |
There was a problem hiding this comment.
Please use different values for the min and max so that this test will fail if the logic gets accidentaly inverted at some point.
README.md
Outdated
| ### getTransactions(asset, [options, ]callback) | ||
| GET https://api.mybitx.com/api/1/transactions | ||
| ### getTransactions(account_id, [options, ]callback) | ||
| GET https://api.mybitx.com/api/1/accounts/:id/transactions |
There was a problem hiding this comment.
Please use the same style of placeholder for variables as the rest of the docs i.e. {accountId}.
Codecov Report
@@ Coverage Diff @@
## master #17 +/- ##
=========================================
Coverage ? 99.22%
=========================================
Files ? 1
Lines ? 129
Branches ? 0
=========================================
Hits ? 128
Misses ? 1
Partials ? 0
Continue to review full report at Codecov.
|
3 participants
The new Luno API requires an account_id to retrieve transactions. If you are currently using getTransactions you will need to update your code to use min_row and max_row to specify transaction range. You will also need to pass in your account_id instead of using the asset code.