search

bchavez / Coinbase

:moneybag: A .NET/C# implementation of the Coinbase API.
https://developers.coinbase.com/api/v2
MIT License
170 stars 92 forks source link

Architecture Discussion For v6 #39

Open granthoff1107 opened 5 years ago

granthoff1107 commented 5 years ago

I was trying to integrate refresh tokens when I came across some issues. The Architecture changes I'm going to propose should probably be for the next version v6 and not for now. I'm a bit confused on why the coinbase is inheriting from the FlurlClient. This is creating a hard dependency on the FlurlClient. The client is subject to be changed (e.g) the last version was using RestSharp Client Perhaps their should be a protected property instead for the client, to abstract the depedency.

The current architecture makes it difficult to modify the request control flow, if someone wanted to catch an error such as expired_token, currently they would have to wrap every request made.

If the endpoints were made properties, and all inherited from the same base interface/ class, a builder could be used to construct the request, and the base class could have an execute function (similar to the prior version) which could be overridden to make it easier to change the control of each request.

For now I'm going to integrate the beta with my api.
lets discuss this more when you get a chance

bchavez commented 5 years ago

Hi Grant,

Can you give some C# code examples of the token expiration flow and how you handle renewal?

I'd like to see these pain points first hand and then I'll be able to comment further on the issue.

Let me know.

Thanks, Brian

granthoff1107 commented 5 years ago

I’ll have a pull request for you soon

On Mon, Nov 12, 2018 at 10:04 PM Brian Chavez notifications@github.com wrote:

Hi Grant,

Can you give some C# code examples of the token expiration flow and how you'd handle renewal?

I'd like to see these pain points are first hand and then I'll be able to comment further on the issue.

Let me know.

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/39#issuecomment-438114276, or mute the thread https://github.com/notifications/unsubscribe-auth/AMsMnewYY-uCVcc9NGE-Hz82kDsMoP-Bks5uujbagaJpZM4YYKTe .

granthoff1107 commented 5 years ago

I've created a branch for Normalization for future version, it's mostly done, but I'm not satisifed with it currently, its not as fluent as it should be https://github.com/granthoff1107/Coinbase/tree/NormalizationV7

I'll open a seperate issue for OAuth and Well discuss it there I think its do able with the current api