Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

NDAX: added REST api Support #465

Merged
merged 7 commits into from
Oct 22, 2019
Merged

NDAX: added REST api Support #465

merged 7 commits into from
Oct 22, 2019

Conversation

Kukks
Copy link
Contributor

@Kukks Kukks commented Oct 16, 2019

No description provided.

@vslee
Copy link
Collaborator

vslee commented Oct 17, 2019

Since this PR uses different JSON parsing logic than the rest of the library, I'll let jjxrta review it.

@vslee
Copy link
Collaborator

vslee commented Oct 17, 2019

Can simplify the namespaces a bit though. For example, the converters and models can be in ExchangeSharp.NDAX. Also, looks like the official capitalization is NDAX.

using System;
using Newtonsoft.Json;

namespace ExchangeSharp.API.Exchanges.Ndax.Converters
Copy link
Collaborator

Choose a reason for hiding this comment

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

simplify to ExchangeSharp.NDAX

@@ -0,0 +1,16 @@
using Newtonsoft.Json;

namespace ExchangeSharp.API.Exchanges.Ndax.Models
Copy link
Collaborator

Choose a reason for hiding this comment

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

simplify to ExchangeSharp.NDAX


public partial class ExchangeName
{
public const string Ndax = "Ndax";
Copy link
Collaborator

Choose a reason for hiding this comment

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

From their site, the capitalization is NDAX. Conflicts slightly w/ C# capitalization guidelines, but I've been changing the exchange capitalization to match their official titles.

@jjxtra
Copy link
Collaborator

jjxtra commented Oct 20, 2019

Let's make all those model objects use internal instead of public, I don't think anyone outside of the internals of this exchange will care to see them. Even better, make the exchange a partial class and embed the models inside of the class.

@jjxtra
Copy link
Collaborator

jjxtra commented Oct 20, 2019

Binance uses similar json parsing and model object strategy so it is fine. Depending on complexity of Exchange model objects can be helpful.

@@ -1,7 +1,7 @@
using System;
using Newtonsoft.Json;

namespace ExchangeSharp.API.Exchanges.Ndax.Converters
namespace ExchangeSharp.NDAX
{
public class BoolConverter : JsonConverter
{
Copy link
Collaborator

Choose a reason for hiding this comment

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

This can be an internal class, like the others

Copy link
Collaborator

@vslee vslee left a comment

Choose a reason for hiding this comment

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

Since we're being nitpicky and giving you conflicting instructions/reviews, I'm going to go ahead and approve. I'll make the changes in a separate PR. Appreciate your contribution.

@vslee vslee merged commit 80b0610 into DigitalRuby:master Oct 22, 2019
@Kukks
Copy link
Contributor Author

Kukks commented Oct 22, 2019

Thanks guys, I'll do a followup PR with partial websocket support and any cleanup you want( just leave a comment here) tonight. :)

@vslee vslee changed the title Add NDAX REST Api Support NDAX: added REST api Support Nov 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants