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

kuna exchange added #330

Merged
merged 4 commits into from
Oct 18, 2017
Merged

kuna exchange added #330

merged 4 commits into from
Oct 18, 2017

Conversation

mkutny
Copy link
Contributor

@mkutny mkutny commented Oct 17, 2017

No description provided.

@kroitor kroitor self-assigned this Oct 17, 2017
@kroitor
Copy link
Member

kroitor commented Oct 17, 2017

Hi, @mkutny ! Thx for the PR! We will merge it! But, unfortunately, Travis and GitHub are having troubles at the moment, so, I guess, we'll have to wait until they resolve (usually should not take more than a few hours). Just letting you know! Thx again!

} else {
account = this.account ();
}
let account = this.extend (this.account (), result[currency]);
Copy link
Member

@kroitor kroitor Oct 17, 2017

Choose a reason for hiding this comment

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

Ah, unfortunately it fails on Python, because result[currency] is illegal in Python if result does not have such key (throws a KeyError exception)... This is why i had to rewrite that extend into an if () {} else {}... But don't bother, I'll fix for that, it's just a Python-only issue and you didn't have to know that. No worries.

@kroitor kroitor merged commit e30f492 into ccxt:master Oct 18, 2017
kroitor added a commit that referenced this pull request Oct 18, 2017
kroitor added a commit that referenced this pull request Oct 18, 2017
kroitor added a commit that referenced this pull request Oct 18, 2017
kroitor added a commit that referenced this pull request Oct 18, 2017
kroitor added a commit that referenced this pull request Oct 18, 2017
kroitor added a commit that referenced this pull request Oct 18, 2017
@kroitor
Copy link
Member

kroitor commented Oct 18, 2017

Ok, I merged it and it should be available as of v1.9.169+! Respect and thanks a lot for your efforts!

@mkutny
Copy link
Contributor Author

mkutny commented Oct 18, 2017

@kroitor,

Just curious, why did you decide to re-implement sign() in kuna class? Order management worked with sign() in acx class as well.

And as a side note: actually, I wanted to start small with just one market, test it thoroughly and then extend to other markets but I see you already added them. I noticed few bugs there so will send a new PR later on today.

@kroitor
Copy link
Member

kroitor commented Oct 18, 2017

@mkutny

Just curious, why did you decide to re-implement sign() in kuna class? Order management worked with sign() in acx class as well.

There's a very slight difference in their endpoint URLs (at least that is what I was observing on my side). ACX and Yunbi do add .json extension to their endpoints, whereas Kuna does not. Because of that I had some failures on public methods. But apart from that everything else is the same. Of course, I should've refactored it into a configurable extension property and keep a single method using that property instead. But that copy-paste does it as a quickfix (temporarily). I'm sure we will eventually bring it back to a unified sign(), hopefully, very soon.

To reproduce those mentioned pubic API failures, remove that sign() method from Kuna and run:
node test/test kuna or node test/test --verbose kuna.

Looking forward to see more commits from you! Thx again!

kroitor added a commit that referenced this pull request Oct 18, 2017
@kroitor
Copy link
Member

kroitor commented Oct 18, 2017

@mkutny, ok, somewhat better now )

academe-01 pushed a commit to academe-01/ccxt that referenced this pull request Oct 2, 2022
Phemex :: fix authentication resolving
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants