search

masalinas / node-red-contrib-ccxt

node-RED ccxt wrapper flow
Apache License 2.0
6 stars 7 forks source link

error on custom apis #2

Closed nileio closed 2 years ago

nileio commented 5 years ago

hi there, I think this line has a problem but I have not debug it .. exchangeconfig.apitype + '_' + config.apicustom.toLowerCase();

If the custom API name pattern is apiname/{param} , then this line fails with an error message message: "exchange[(config.apitype + "" + config.apicustom.toLowerCase(...))] is not a function"

do you know a fix for this issue ? you can reproduce it by choosing a custom API which accepts an url parameter

LTangaF commented 3 years ago

This project seems fairly dead, unfortunately. I had wanted to use it, too. I'm having a similar issue and determined that the registration of the routes at https://github.com/masalinas/node-red-contrib-ccxt/blob/master/ccxt/ccxt-api.js#L68-L70 don't seem to actually work.

nileio commented 3 years ago

I forked the project and created a new node altogether long time ago .. it works really well and I plan to contribute to node-red community just never had the time to do proper documentation. I will get to it soon https://github.com/nileio/node-red-contrib-ccxt-v2 do not use that version in the repo but just give you idea of all the features there.. I implemented way too much as it was actually my first node-red node .. the code is way behind my dev branch so should not be used as I have not committed to github yet my local code

LTangaF commented 3 years ago

Hmm. Let me push what I've done so far in my fork and let you look. There's probably some duplication of effort, but that's alright. I'll drop another comment here when I've done that.

masalinas commented 3 years ago

Hello and sorry for the big delay, I'm trying not dead the project, I'll try to help and correct any bug let me know.

Sorry and Regards

LTangaF commented 3 years ago

Miguel,

Nice to hear from you. I think I'd prefer to contribute to yours, too, as it's the module already registered with the Node Red contributor site. It sounds like @nileio has likely already fixed this in his fork and I've fixed a couple of things as well in a working branch on mine. If you're going to be active in reviewing PRs, I think that will help, but it might be a good idea to have 1 or 2 more reviewers with merge authority so that it doesn't all fall on you.

LTangaF commented 3 years ago

Here is the working branch where I've added bybit and fixed some functional issues: https://github.com/LTangaF/node-red-contrib-ccxt/tree/add_bybit

Let me know if you cannot see it.

masalinas commented 3 years ago

Perfect, thanks PR completed ...

LTangaF commented 3 years ago

Oh. That was someone else's PR. I haven't opened one yet.

nileio commented 3 years ago

@masalinas hi - are you interested to work on my fork instead. It is way more feature-rich and I originally used your base but ended up doing much more than I imagined originally. It works pretty well but if interested please PM me and we can discuss. the github source is out-dated though..

nileio commented 3 years ago

@LTangaF I am glad we have contributors.. I will push my fork to github this week, and it will be great if we get to a single branch and take it from there. I implemented dynamic population of supported APIs for selected exchange, multi-exchange support, private and public APIs, dynamic secrets based on supported methods of the exchange, selection of test/live net, many other features while playing with this node

nileio commented 3 years ago

@masalinas @LTangaF I merged my changes on my fork https://github.com/nileio/node-red-contrib-ccxt-v2 If you manage to install my version, feel free to open an issue and I will support you. There are couple of gotchas there that I am aware of but don't have the time to revisit. This was my first node-red node so I was learning many things but anyway. If any is keen to merge/have a single branch including all the features that will be awesome. Now I implemented few things that I wouldn't have done if I were to start fresh including multi-exchange support, etc. In all cases, feel free to open an issue and we can discuss what goes in/out of a merged version likely. cheers

LTangaF commented 3 years ago

@nileio

I'm curious what you mean by a couple of your enhancements, because I think they were already in the code, for instance:

The thing I added that it sounds like you did, too, was to add support for 'Sandbox', which is what it's named in the underlying ccxt library. Basically, it's a checkbox that switches to an exchanges test URL.

I also added allowing the apipayload for CustomAPI to come in on the flow.

I'm not sure what you mean by dynamic secrets, but if it the ability to not have to use them for public apis that don't require them (for instance, downloading tradeBucket data from the Bitmex API doesn't require a set of keys and it is in fact preferable not to as there's a strong likelihood of wanting to get the symbol index data from Bitmex without having to create an account with them), then that is a feature that I'm needing and was about to add.

LTangaF commented 3 years ago

Oh, and thank you, @nileio, for adding -v2 on the end of your node definitions. That allows me to test your changes without ripping out the old one.

LTangaF commented 3 years ago

@nileio

Actually, I can't quite get it to work and you don't have issues turned on in your fork to move the discussion there (though getting the two projects aligned would be good)

nileio commented 3 years ago

@LTangaF sorry I didnt realise. I added the Issues and Discussions section. Feel free to either open an issue or start a discussion and I will support you. I am aware of couple of gotchas when you first install I just didn't have the time to fix those. You need to deploy the flow first without any config and then after first deploy open the node again and things should work for you.

nileio commented 3 years ago

@LTangaF

I'm curious what you mean by a couple of your enhancements, because I think they were already in the code, for instance:

  • dynamic population of supported APIs for selected exchange (I believe this occurred already under the Custom API section)
  • multiple-exchange support (that is built-in to take advantage of underlying ccxt module)

multiple exchange support actually means that I can in a single node have multiple exchanges selected, then only the markets which exist on those exchanges will be populated in the menu to work with, then you can for example query multiple exchanges for balances, or choose any of the Unified APIs which are supported by those exchanges only.

I used select2 library https://select2.org/ to provide a multi-select dropdown list.. which is a dependency for the node.

dynamic population means : automatically show the fields required for the selected API only.

also I added support for showing the required secrets (API Keys) fields for the selected exchange. Exchanges have different implementations and some of them use 2 fields, others use 3, etc. So I use ccxt to query that and show only the required fields in the config which is pretty neat.

Also I added support for selection of specific exchange url , that is either testnet or livenet

give it a go anyway and happy to support. As I said it would be good if we get contributors and perhaps merge the best parts of both those nodes. I didn't follow the best practice trying to implement too much in a single node but I was just learning at the time.