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

Fetch crypto deposit addresses #265

Merged
merged 5 commits into from
Apr 5, 2018
Merged

Conversation

passabilities
Copy link
Contributor

Thanks to a hack found by @pcfreak30 and @sagivo in this thread #91

@fb55
Copy link
Contributor

fb55 commented Feb 13, 2018

Can you add a test for this?

depositCrypto(params, callback) {
this._requireParams(params, ['currency'])
return new Promise((resolve, reject) => {
this.get(['coinbase-accounts']).then((coinbaseAccounts) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Use getCoinbaseAccounts?

@@ -245,6 +245,17 @@ class AuthenticatedClient extends PublicClient {
return this.post(['deposits/coinbase-account'], { body: params }, callback);
}

depositCrypto(params, callback) {
this._requireParams(params, ['currency'])
return new Promise((resolve, reject) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why return a new promise here?

@passabilities
Copy link
Contributor Author

@fb55 fixed

.then((coinbaseAccounts) => {
let account = coinbaseAccounts.find(a => a.currency === params.currency);
return this.post(['coinbase-accounts', account.id, 'addresses'], callback);
});
Copy link
Contributor

Choose a reason for hiding this comment

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

Catch errors if there is a callback provided?

depositCrypto(params, callback) {
this._requireParams(params, ['currency']);
return this.getCoinbaseAccounts()
.then((coinbaseAccounts) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Prettier should get rid of these parenthesis

@fb55
Copy link
Contributor

fb55 commented Feb 20, 2018

Prettier seems to be skipped. Otherwise lgtm!

@fb55
Copy link
Contributor

fb55 commented Feb 21, 2018

The formatting is still off. Can you please run prettier on these files?

@fb55 fb55 merged commit 6967f07 into coinbase:master Apr 5, 2018
@fb55
Copy link
Contributor

fb55 commented Apr 5, 2018

Sorry for the pause and thanks a lot!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants