search

nileio / node-red-contrib-ccxt-v2

A Node-RED node for communicating with all crypto exchanges using ccxt library
MIT License
10 stars 4 forks source link

api endpoint conflict with node-red-contrib-ccxt #1

Closed LTangaF closed 3 years ago

LTangaF commented 3 years ago

A couple of the various registered data endpoints, specifically /exchanges and /apis conflicted with the same endpoints defined in the other module.

I'm already working on a fix for this, as I think it's possible, at least while the two projects are split, that people will try both and get strange behavior (like I did).

Right now, I've just tacked on v2 on the end of their registered routes, but one of the other changes I made to get things working was to change the ccxt/ path in the module to ccxt-v2, so perhaps instead of tacking on v2, I'll change the routes to all start with /ccxt-v2/ to make a fully clean differentiation.

I would like to have a live discussion with you at some point, @nileio. I don't know what chat systems you participate in. I run a Matrix server. That's a federated system, so we could talk on that. Discord or Slack are also possibilities. What UTC times might you be around to have a conversation?

BTW, if it wasn't already clear, I'm already fixing the issues I've encountered and will create a PR here.

LTangaF commented 3 years ago

Other things that I'm fixing are the service endpoint reload issue that you'd mentioned in the other conversation. I discovered when fixing the older module that registering a service endpoint on the internals of a node instance registration is considered to be a "no no" by the Node-Red creators.

That registration code is run every time another instance of a node is added to the flow and the services should only be registered once. I moved the service functions and registrations outside of the node registrations in both the old project and in the branch of yours with good success. The only negative is that you can't use node.warn() in those scenarios. Any errors thrown by those APIs will only be thrown in the Node.js console unless you send back an error response to the client and then have the client handle that separately.

The other thing I'm actively fixing, though this is something I might want to discuss with you, is that certain exchange.api sections are deeper than apitype/verb. Bybit, in the version of the ccxt library you have selected, has extra levels, and it ends up by creating a non-useable drop-down selector where all the available api's end up in these weird, comma-delimited list items.

opt-group only allows one level deep, so I was going to create an iterating function that could follow the nest deeper and add indentation to the subsequent levels. However, I think the ccxt library maintainers have since flattened out Bybit's API and it's possibly moot. One of the things I was doing to the old node-red-contrib-ccxt module was to add Bybit support because it was missing, and in that process I selected a newer version of the underlying ccxt library.

I like better your solution of completely constructing the list from that underlying library rather than hand-coding it in, but possibly we need to use a newer version of that underlying library. I actually wasted a little time discovering that when debugging, it matters which directory level in the project I actually run node in to manually execute code. I was scratching my head at what the endpoints were creating and what the ccxt library was yielding to me until I realized that I had a different version of that library higher up in the directory structure.

And that leads me to my last hurdle, one of the things I fixed was the serving up of the .map file as the browser was complaining that it couldn't load it. I'm not exactly sure what purpose that the .map file provides to the browser, but it was complaining and I fixed it. However, I can tell that the .map files are generated by a tool called genmaps and I can't figure out where that comes from. Does that come from one of the devDependencies ? I'm thinking I do need to regenerate those .maps, though, because I've been touching the files that they are derived from.

nileio commented 3 years ago

A couple of the various registered data endpoints, specifically /exchanges and /apis conflicted with the same endpoints defined in the other module.

I'm already working on a fix for this, as I think it's possible, at least while the two projects are split, that people will try both and get strange behavior (like I did).

The nodes are actually meant to be an upgrade/next version so I didn't originally think of using both at the same time but it all depends on effort vs benefits.

Right now, I've just tacked on v2 on the end of their registered routes, but one of the other changes I made to get things working was to change the ccxt/ path in the module to ccxt-v2, so perhaps instead of tacking on v2, I'll change the routes to all start with /ccxt-v2/ to make a fully clean differentiation.

Yes makes sense. This should work as long as you register the new nodes with the new path.

I would like to have a live discussion with you at some point, @nileio. I don't know what chat systems you participate in. I run a Matrix server. That's a federated system, so we could talk on that. Discord or Slack are also possibilities. What UTC times might you be around to have a conversation?

BTW, if it wasn't already clear, I'm already fixing the issues I've encountered and will create a PR here.

Great Thanks for working on this @LTangaF .. happy to merge any PR.

.. I am at UTC+10 (Melbourne/Australia) ! Feel free to send me an invite to your Matrix server. I use Slack and Telegram. I am actually working in a bigger project atm and would depend on this node so keen to develop on that.

nileio commented 3 years ago

Other things that I'm fixing are the service endpoint reload issue that you'd mentioned in the other conversation. I discovered when fixing the older module that registering a service endpoint on the internals of a node instance registration is considered to be a "no no" by the Node-Red creators.

That registration code is run every time another instance of a node is added to the flow and the services should only be registered once. I moved the service functions and registrations outside of the node registrations in both the old project and in the branch of yours with good success. The only negative is that you can't use node.warn() in those scenarios. Any errors thrown by those APIs will only be thrown in the Node.js console unless you send back an error response to the client and then have the client handle that separately.

Yes, this is right and I am aware of this service endpoint registration issue! It is in the wrong place, and I couldn't figure out at the time a way of using node.. I will check on that point and see how to handle errors correctly.

The other thing I'm actively fixing, though this is something I might want to discuss with you, is that certain exchange.api sections are deeper than apitype/verb. Bybit, in the version of the ccxt library you have selected, has extra levels, and it ends up by creating a non-useable drop-down selector where all the available api's end up in these weird, comma-delimited list items.

Every upgrade of ccxt seem to break those things ! They keep changing their schema file. Now I updated to the latest ccxt version and I didnt test every exchange after that. Though those ccxt version changes make sense , they just break my node. So after complete testing we should hard lock the version of ccxt used by the node. Should not upgrade ccxt unless tested in the future. We can change that in the dependencies of the node.

opt-group only allows one level deep, so I was going to create an iterating function that could follow the nest deeper and add indentation to the subsequent levels. However, I think the ccxt library maintainers have since flattened out Bybit's API and it's possibly moot. One of the things I was doing to the old node-red-contrib-ccxt module was to add Bybit support because it was missing, and in that process I selected a newer version of the underlying ccxt library.

Yes that is right. Do we need more than one level deep with other exchanges ? the ccxt schema should be consistent across all exchanges. I am confused of that bit, not sure if we can get an official schema file. The node depends on a particular structure.

I like better your solution of completely constructing the list from that underlying library rather than hand-coding it in, but possibly we need to use a newer version of that underlying library. I actually wasted a little time discovering that when debugging, it matters which directory level in the project I actually run node in to manually execute code. I was scratching my head at what the endpoints were creating and what the ccxt library was yielding to me until I realized that I had a different version of that library higher up in the directory structure.

Yes. The bundled version should be the one used. I upgraded the ccxt recently but never tested all menus with all exchanges. Yes, I tried to construct the menus dynamically which is should be good if it works well. The hard dependancy is ccxt schema file, as we cannot in the current version make overrides for some exchanges. Though , an enhancement could be that we add a concept of overrides to point assign the correct location for those fields required. We only need a handfull of those pointers but I don't know this might be an overkill.

And that leads me to my last hurdle, one of the things I fixed was the serving up of the .map file as the browser was complaining that it couldn't load it. I'm not exactly sure what purpose that the .map file provides to the browser, but it was complaining and I fixed it. However, I can tell that the .map files are generated by a tool called genmaps and I can't figure out where that comes from. Does that come from one of the devDependencies ? I'm thinking I do need to regenerate those .maps, though, because I've been touching the files that they are derived from.

I built a utility to generate source maps for node-red front end code ! It enables me to debug the front-end code and step through the code using VSCode nicely. This way I can have breakpoints for the editor code, etc. You can really ignore them, they only good for debugging.

However, I will add it in the repository. You can place the utility anywhere and wire it up to your node scripts. The example vscode launch.json contains the debug configuration I use. You need to install the utility somewhere like /usr/local/bin or anywhere you want, then update package.json with the location of gensourcemaps. You can then run the debugger using ccxt using NPM configuration. This configuration starts node-red then after it starts, launches Chrome in debug mode and uses the source maps. This way you should be able to place breakpoints,etc. in node-red html files. I assume you use VSCode of course. I also meant to share this utility with the community ! I am sure lots will be needing a similar solution

LTangaF commented 3 years ago

Sorry about the slow response. I have most of the changes queued up, but had to get a production deployment done before I could finish it up and create the PR. I would like to chat. The other thing I did was establish a Matrix-Telegram bridge (and a Telegram account). You can reach me at @Tangaloor