Support For DI

[granthoff1107]

I know we briefly discussed this before, but I think we should reconsider support for DI. I have 3 projects I'm using the API and OAuth in. And I've found myself duplicating the class and interface definitions in each project. I'm tempted to move it into a shared assembly between the projects.

public interface ICoinbaseApiClient : ICoinbaseClient
{
}

public class CoinbaseApiClient: CoinbaseClient, ICoinbaseApiClient
{
    public CoinbaseApiClient(ApiKeyConfig config): base(config)
    {
    }
}

public interface ICoinbaseOAuthClient : ICoinbaseClient
{
}

public class CoinbaseOAuthClient : CoinbaseClient, ICoinbaseOAuthClient
{
    public CoinbaseOAuthClient(OAuthConfig config) : base(config)
    {
    }
}

I should have small pull request upcoming for notifications,
If you reconsider I'll add the support for DI, let me know what you think.

[granthoff1107]

Additionally, Notification should only be support by the API client, because there is no way to validate the signature from OAuth

[bchavez]

Hi Grant,

Respectfully, I disagree about adding these interfaces and classes to explicitly support DI. However, that being said, you can send a PR to add them as long as there is no implementation details in any of these classes or any member declarations in these interfaces.

FWIW, webhook notification signature validation only applies to the WebhookHelper static class and is not related to API key or OAuth.

Thanks, Brian

[granthoff1107]

I understand, splitting up the logic into two classes is too much for the right now.

Thanks for allowing the pull request

I’m testing notifications and I’m talking with the Coinbase support team.

The behavior I’ve seen when attempting to grab notification using the OAuth client. Coinbase throws a 403 forbidden.

I’ll do a bit more investigation and let you know what I find out

On Thu, Nov 29, 2018 at 9:11 PM Brian Chavez notifications@github.com wrote:

Hi Grant,

Respectfully, I disagree about adding these interfaces and classes to explicitly support DI. However, that being said, you can send a PR to add them as long as there is no implementation details in any of these classes or any member declarations in these interfaces.

FWIW, webhook notification signature validation only applies to the WebhookHelper static class and is not related to API key or OAuth.

Thanks, Brian

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/bchavez/Coinbase/issues/46#issuecomment-443065438, or mute the thread https://github.com/notifications/unsubscribe-auth/AMsMndbatZ_vYzV9k0tfjb7McPPcsrTiks5u0JPsgaJpZM4Y6zoH .

[bchavez]

Ah I see, pulling down notifications using OAuth should work.

Might want to double check and make sure you have a wallet:notifications:read scope permission when the OAuth app gets authorized. IIRC, you can check your scopes by pinging: https://developers.coinbase.com/api/v2#show-authorization-information

[granthoff1107]

Gah, I think your right I added the scope to the api but I didn’t logout and log back in to refresh my token with the new scopes

On Thu, Nov 29, 2018 at 9:38 PM Brian Chavez notifications@github.com wrote:

Ah I see, that pulling down notifications using OAuth should work.

Might want to double check and make sure you have a wallet:notifications:read scope permission when the OAuth app gets authorized. IIRC, you can check your scopes by pinging: https://developers.coinbase.com/api/v2#show-authorization-information

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/bchavez/Coinbase/issues/46#issuecomment-443070488, or mute the thread https://github.com/notifications/unsubscribe-auth/AMsMnVhq9e3pNMpsRNCSKnwH7rEn1CXnks5u0Jo4gaJpZM4Y6zoH .

[granthoff1107]

Hmm even with the Notifications scope on I'm still getting a 403 have you tested notifications with OAuth against coinbase?

[granthoff1107]

I got have the error code now, It's not allowed: "This endpoint is only available for API key authentication"

[bchavez]

DI interfaces been added in #47. Closing this.