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

Refactor auth object to { key, secret, passphrase } #151

Merged
merged 1 commit into from
Dec 24, 2017

Conversation

rmm5t
Copy link
Contributor

@rmm5t rmm5t commented Nov 30, 2017

This more consistent usage allows an AuthenticatedClient instance to be used
in place of an auth object thereby simplifying codebase and general usage patterns.

AuthenticatedClient used to have a b64secret property, but all auth object credentials expected a secret property. By making the two consistent, we gain better consistency, and the codebase can be simplified a bit.

Other considerations
This will likely need to be rebased after #150 is merged.

This more consistent usage allows an AuthenticatedClient instance to be used
in place of an auth object
@rmm5t rmm5t changed the title Simplify auth object to <key, secret, passphrase> Refactor auth object to <key, secret, passphrase> Nov 30, 2017
@rmm5t
Copy link
Contributor Author

rmm5t commented Nov 30, 2017

I should also mention that if this refactor is accepted, I'd also like to submit a small change such that the OrderbookSync doesn't necessarily ask for an AuthenticatedClient, but instead just an optional auth object (i.e. { key, secret, passphrase }). Afterall, the authenticatedClient config might conflict with the other OrderbookSync arguments, like apiURI.

(This would still remain backwards compatible, because the property signatures would still behave the same way, but it would free up the library as a whole to move towards better consistency)

@rmm5t rmm5t changed the title Refactor auth object to <key, secret, passphrase> Refactor auth object to { key, secret, passphrase } Nov 30, 2017
@fb55 fb55 merged commit 17619ca into coinbase:master Dec 24, 2017
@fb55
Copy link
Contributor

fb55 commented Dec 24, 2017

Great change. Also in favor of the proposed extension, no reason to need an authenticated client here.

@rmm5t rmm5t deleted the consistent-auth-object branch December 24, 2017 22:55
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.

None yet

2 participants