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

[Proposal] Merge Xchange project with bitrich-info project #3220

Closed
makarid opened this issue Sep 13, 2019 · 39 comments
Closed

[Proposal] Merge Xchange project with bitrich-info project #3220

makarid opened this issue Sep 13, 2019 · 39 comments

Comments

@makarid
Copy link
Collaborator

makarid commented Sep 13, 2019

Hello,

i would like to propose the merge of those 2 projects. Bitrich-info is a very good project which i believe it is a must for every developer that wants real-time prices and response time. That merge will create a huge amount of value for everyone. Right now most of us, that we use this library on our projects, have a major issue with rate limits. This will help us eliminate most of them just by using websockets instead of the classic REST calls, as every exchange suggest to do if the offer such. What do you think?

@timmolter
Copy link
Member

Did you know that Bitrich-info originally started as XChange code? XChange used to support streaming as well, but we peeled out the streaming part to form Bitrich-info because it was just way too much to maintain in one project. Plus the streaming dependencies were not at all robust back then, and it was a real headache.

@makarid
Copy link
Collaborator Author

makarid commented Sep 14, 2019

Thanks for the response @timmolter . I didn't know that. Do you believe that now it is much simpler with the dependencies? Is the project more mature now? Thanks

@timmolter
Copy link
Member

I honestly do not know because I haven't been following that project. From a quick look however it looks waaaay more advanced now than it was before and many more exchanges are supported. I started doing monthly XChange releases specifically for the bitrich-info project meaning the two projects are very closely coupled. It looks like they are looking for a maintainer right now, which might be something you'd be interested in.

@makarid
Copy link
Collaborator Author

makarid commented Sep 15, 2019

@timmolter i have already talked with today maintainers about becoming one, but because i don't have the experience or the knowledge they have, i am not ready for it yet. It is a shame that bitrich project don't have the maintainers it deserves. That is why i propose a merge. More people will work for the same project and PRs will be merge much faster.

@walec51
Copy link
Collaborator

walec51 commented Sep 17, 2019

Merging the two projects won't change much in the matter how much work is being devoted to them. Most ppl contribute only the modules / features they use. That said refactoring done to xchange-core would be easier to keep in sync with streams.

If an eventual merge was supposed to happen I think streaming features should still be kept in a separate module. For example you would have xchange-binance and xchange-binance-stream. This is because they usually have additional dependencies like websockets that may be undesired by people that don't need streams - and lots of use cases don't need them.

@makarid
Copy link
Collaborator Author

makarid commented Sep 17, 2019

The thing that will change is how fast the PR will be merge.This is the pain in the ass, on the bitrich-info project. If we merge the projects this will change. I totally agree with your second point, i also was thinking something like this, definitely separate modules. So what do you think? Can we do something like that?

@makarid
Copy link
Collaborator Author

makarid commented Sep 18, 2019

I can do all the work of merging.

@timmolter
Copy link
Member

Have you had PRs there that aren't getting any attention?

I personally cannot dedicate any time to reviewing streaming PRs or anything related to the streaming API.

@makarid
Copy link
Collaborator Author

makarid commented Sep 22, 2019

@walec51 or anyone here, are you willing to review streaming PRs? Right now on bitrich project there are only 2 persons (i am not sure) which review and merge. I can review them, but i cannot merge them because i don't have the privilege.

@makarid
Copy link
Collaborator Author

makarid commented Sep 22, 2019

Have you had PRs there that aren't getting any attention?

I personally cannot dedicate any time to reviewing streaming PRs or anything related to the streaming API.

There are many PRs which hang there for days. I totally understand that you don't have that extra time to do it. At least there will not be any version issues with the merge. I am more than happy to review streaming PRs, although i am not expert or very good. I am dedicating a lot of my time on these 2 projects, so i will definitely be a reviewer.

@walec51
Copy link
Collaborator

walec51 commented Sep 23, 2019

I don't use streaming currently in my projects so no, I will not review streaming PRs.

I'm against merging projects until someone that actively maintains bitrich comes here personally and says that he thinks that merging is a good idea and that he will continue maintaining that code after the merge.

@makarid if you want to be that person then please gain some experience first by contributing to the bitrich project in its current form. After some time of working together they should give you collaborator status.

@makarid
Copy link
Collaborator Author

makarid commented Sep 24, 2019

I don't use streaming currently in my projects so no, I will not review streaming PRs.

I'm against merging projects until someone that actively maintains bitrich comes here personally and says that he thinks that merging is a good idea and that he will continue maintaining that code after the merge.

@makarid if you want to be that person then please gain some experience first by contributing to the bitrich project in its current form. After some time of working together they should give you contributor status.

@badgerwithagun what do you thing about the merging? Will you continue to contribute to the bitrich project if we make the merge?

@badgerwithagun
Copy link
Collaborator

Merging the two projects would make a huge difference to me and others, I'm sure. I can't be the only one that has to maintain synchronised forks of both libraries to keep a production app stable. There are some differences in project structure which would need to be fixed to get the streaming project ready for a merge, but that's all achievable if someone has the time.

I would continue to review & merge PRs after a merge, but like most contributors to the xchange-stream project, I don't use all exchanges. I won't be able to review BitMex PRs, for example (which covers most of the long, outstanding unreviewed PRs).

I certainly can't guarantee long-term support (who can? Including @timmolter, I'm sure!), but have no plans to stop my involvement in the near future. However, to split hairs: I'm not a maintainer; I'm just a collaborator. @dozd is the only (semi) active maintainer.

Broadly: I'm in favour of it, but the challenge xchange-stream has is a lack of committed maintainers, and that won't be immediately solved by merging projects (at least immediately - I suspect more people would get involved pretty quickly if streaming were more obviously available).

@makarid
Copy link
Collaborator Author

makarid commented Sep 24, 2019

Thank you @badgerwithagun for your input.I would love to be a maintainer when i am fit. So @timmolter and @walec51 what do think?

@dozd
Copy link
Contributor

dozd commented Sep 24, 2019

I remember when Xchange library supported streaming and also when and why they dropped it. Their implementation was not good mainly due to bad design choices as well as bad technologies for the streaming implementation. When I started the implementation of the streaming interfaces, usage of rxjava was a crucial point to make it as simple as possible and solved a lot of bad choices from the past.

But as I am in favor of a loosely coupled architecture I think these two libraries benefits from their separation. Modular approach is much more easier to maintain for a long term. Maybe I don't see that but what would be the benefits of merging them together? If you need the basics, use Xchange. If you need streaming, add Xchange-stream on top of it. Big all-in-one libraries are obsolete.

The bad thing is that I'm not using any of these two libraries anymore and I don't have a time (and energy) to maintain xhcnage-stream just without any of my need. But I do not think that merging with Xchange will solve this problem. But maybe I am wrong.

Anyway, if there will be an opinion that these two libraries should be merged, I'm happy to assist you if needed.

@makarid
Copy link
Collaborator Author

makarid commented Sep 24, 2019

@dozd the main issue with bitrich project is the luck of maintainers as you correctly told us. I have a lot of code in my local branch which i cannot merge because of that. But the biggest pain in the ass is the versioning issue. Bitrich project depend on Xchange but cannot keep up with the versioning of Xchange. Without Xchange bitrich cannot exists and that is way it is crucial to have always the latest version of Xchange on bitrich project.This is not what is happening now. For example, lets say that i want to fix an issue on Bitrich library. After searching on the code i found a solution, but this solution needs to add a feature on Xchange library first. So i make the PR to Xchange and they merge. But because bitrich doesn't use the latest version on Xchange,i cannot use the new feature. I need to wait for the new release in order to see the new Xchange feature on Bitrich library. This is at minimum frustration. I believe that the merge will at least fix that issue and maybe will attract more devs on streaming version. There are developers from exchanges that support Xchange with new implementation from their APIs but don't do it for bitrich because of that.

@badgerwithagun
Copy link
Collaborator

xchange-stream suffers from a lack of attention. People find XChange, it does what they need, done. If streaming were a more first-class citizen, more people might use it. More users means more interest, more eyes on the problem, more PRs, more collaborators, more predictable release cycle. I don't see any element of this that stands as a benefit for the XChange project though; it's purely to the benefit of xchange-stream. I wouldn't try and pretend otherwise!

I fully understand if others want to maintain the separation, but for the sake of argument if nothing else, I will try and make an architectural case.

In principle, @dozd is absolutely right; smaller, focused libraries are the "right" way to work, but it's important to apply appropriate domain-driven-design if you're going to enforce modularity, and having worked in parallel on both XChange and xchange-stream, it feels to me that "not streaming" and "streaming" are not the bounded contexts; the exchanges are.

It would make more sense to me if there were:

  • A clear "core xchange api"
  • A clear "core streaming api" which builds on the core API
  • Exchange A (implements core and streaming API)
  • Exchange B (implements only core API)
  • Exchange C...

In an ideal world, each would be a standalone project (for the very reasons @dozd describes - and because each exchange implementation is likely to have completely different userbases).

However, of course, XChange and xchange-stream are both monorepos covering all the different supported exchanges - the reason being that it allows the API to remain loose because developers can change it at any time by updating all the exchange implementations at the same time. To move away from this to individual projects would mean being much more careful with API design and rollout and probably slow down a lot of people.

One could argue that in a mature project that would be the right thing to do anyway; by splitting out the exchange implementations it would force the core project to apply better, more sustainable API designs - but I don't think this is the kind of project in which such rigour is realistic. It survives on lots of small, disconnected, ad-hoc enhancements, not longer-running design projects.

So - the monorepo has enormous benefit in practice for most people - and I think for those of us that use xchange-stream, that benefit would be double if it covered both libraries. In practice, xchange-stream is regularly broken by XChange API changes and this is every bit as much of a pain as it would be if XChange Exchange A and XChange Exchange B were in separate projects from the XChange core. They would benefit from being in one library for the same reason.

Anyway, I fully understand either way, but that's the best case I can make.

@walec51
Copy link
Collaborator

walec51 commented Sep 26, 2019

Thank you all for taking part in this discussion.

After reading it I came to the conclusion that there are only two small benefits to merging:

  • one could propose changes / fixes to xchange-core and streaming functionality in one PR
  • streaming functionality would get more attention from xchanges users

This will not solve the problem of lack of maintainers because if someone really needs streaming (to the point he would be willing to contribute to make it work) then he will have no problem finding out that Bitrich-info exists and creating a PR there. The only inconvenience is that if some changes in xchange-core are required then he has to have two PRs instead of one.

Secondly I still oppose creating modules suggested by @badgerwithagun that will force me to import streaming dependencies for a given exchange. Modules do not only separate business domains, they are also technical artefacts. If I don't use streaming then I don't want to be forced to import RxJava and other libraries to integrate with Binance for example.

If this merge was supposed to happen then proper dependencies should look like this:

xchange-stream

If this type of module separation was to be kept then I'm personally neither in favour nor against of this merge. This is simply because my use cased do not require it, I just don't want it to get in my way ;)

@badgerwithagun
Copy link
Collaborator

@walec51 I was not talking about modules/JARs; I was talking about projects. Absolutely the modules WITHIN the project should be as you describe. My point was that there is a good reason that Binance is in the same project as the API: they often change together. The same logic applies to the streaming API and the streaming implementations: they often change at the same time as the XChange API.

@makarid
Copy link
Collaborator Author

makarid commented Sep 27, 2019

Thank you all for the inputs. Can we vote in order to see how many we are in favor and against?

I am in favor of a merge.

@makarid
Copy link
Collaborator Author

makarid commented Sep 27, 2019

Just one question. Why you don't want to use websockets in order to get realtime market data? It is blazing fast and you solve the rate limit problem also. We are talking about financial prices here,a 5 second delay,in order to not rate limit you, can destroy you. I really cannot understand that.

@timmolter
Copy link
Member

As @walec51 pointed out it's critical that users don't need to import streaming libs if using the polling API. His proposed module separation makes this a non issue though.

I think if xchange-stream was actually actively developed and maintained and synced with xchange regularly, this merger wouldn't be necessary, but a @dozd said he doesn't have the time and energy to dedicate to it. The issue is less of a technical one and more of a practical one. Therefore a merge would definitely benefit those users who really want to use the xchange-stream project as a critical part of their projects because changes will get merged faster and smoother. So for the benefit of those users (which I personally am not) this merger does make sense. Therefore my vote is to do the merge.

I would hesitate to vote "yes" if @dozd would be upset but it seems he's ok with a merge:

Anyway, if there will be an opinion that these two libraries should be merged, I'm happy to assist you if needed.

@badgerwithagun
Copy link
Collaborator

If we decide yes, I'm quite happy to do a bunch of the work needed. I'd need @dozd's help to get a bunch of laggard PRs pushed through first and a final release done and would aim to conduct the merge in a series of smaller PRs to make it manageable.

I think it'll be important to work on maintaining reversibility throughout and being disciplined enough to resist the urge to do any refactoring whatsoever.

@walec51
Copy link
Collaborator

walec51 commented Sep 30, 2019

@makarid many of us don't you web sockets because getting data via a request - response method is much easier and less error prone then caching data from a stream. For example if I getOrderbook after a placeOrder I know this order book reflects it state after my orders ware matched. If I have a cache asynchronously build from a stream I have no way of telling did the events representing my order get applied already or nor.

@dozd
Copy link
Contributor

dozd commented Sep 30, 2019

@walec51 method is much easier and less error prone - that's because you have probably not seen my stream implementation with rxJava :-).

@timmolter You know, if the merge makes a better open source project, that's the whole reason of the contribution to open source. No reason to be upset. Our apache 2 is compatible with your MIT so everything's OK.

@badgerwithagun i have zero to minus one time because of a work on this game... so I hope it will OK. But I will surely find some spare (from minus one to minus two there is some nice negative space here)

@badgerwithagun
Copy link
Collaborator

Hi @timmolter & @walec51. We're slowly working towards cleaning up xchange-stream for the merge. The current plan is:

  • Clear outstanding pull requests
  • Clear compiler warnings
  • Add coveo-fmt and format the code to match XChange
  • Rename the Maven modules to fit the naming patterns discussed above
  • Create merge PR

The merge PR will be too big to usefully review in one go. Are we happy to get it merged and go incrementally from there?

@cymp
Copy link
Contributor

cymp commented Dec 10, 2019

I'm strongly in favor of merging the two projects since I use both streaming and REST calls, and syncing XChange and XChange-stream versions is painful.

For me streaming is really helping, for the both fresh data but and for the API calls restrictions we have on a lot of exchanges. Some exchanges give much higher api private calls limits for like placing orders with websocket: for example bitfinex which give close to infinite place/cancel limit, and only allows something like 1/sec with rest, not the exact numbers, but the difference is huge. All of the exchanges strongly encourage users to use the streaming api too.

@badgerwithagun
Copy link
Collaborator

@cymp have no fear; that seems to be the general conclusion too. It will happen. If you'd like to help, clearing compiler warnings on xchange-stream would be very welcome!

@dozd
Copy link
Contributor

dozd commented Dec 11, 2019

BTW: try to research how to do the "merge" of these projects correctly in terms of git. It would be very beneficial not to loose the commit history for the future development.

Doing it through the regular pull request by copying the files would delete this valuable info. You surely know the situation: "Why this line of code is here?" I'll check the blame lines. Ah, it was just committed with the another thousands of lines :).

@badgerwithagun
Copy link
Collaborator

@dozd indeed. I was thinking on this yesterday!

@timmolter
Copy link
Member

sounds good!

@mdvx
Copy link
Contributor

mdvx commented Dec 15, 2019

I just saw this thread, and tend to agree with a merge, as I too often have to make changes in both XChange and XStream, and managing the dependencies is painful and thawrt with risks. My only concern is that XChange feels slower paced, concentrating on breadth of exchanges over depth of functionality, but working with one snapshot must be better than having two,

@mdvx
Copy link
Contributor

mdvx commented Dec 15, 2019

One other thing to note: I feel that due to the nature of REST calls, and their rate limits, XChange is much less concerned about GC, and for me that is a problem since I subscribe to multiple exchanges and feeds simultaneously.

@TSavo
Copy link
Contributor

TSavo commented Mar 4, 2020

I've made a provisionary/experimental PR #3436 which is a draft attempt at merging the two projects. It's been marked at DO NOT MERGE because the green light has not been fully given by all involved parties, so this is a draft of what one possible future might look like available to brave souls who wish to test the two projects building and testing together.

There remain a few open PR's in XChange-stream which should still be merged, and I would love to see some discussion regarding the merge choices made, so as to better inform what an actual merge would look like in the future when both projects are ready.

@earce
Copy link
Collaborator

earce commented May 22, 2020

Not sure if this is relevant, but is the streaming part of this project still looking for maintainers? If so what is the general process to bring someone on as such?

@timmolter
Copy link
Member

I'd be happy to take on a maintainer for the streaming part.

@timmolter
Copy link
Member

FYI, I just added @badgerwithagun

@earce
Copy link
Collaborator

earce commented May 24, 2020

@timmolter sounds good, I want to go through a few more reviews maybe some that are more invasive and if you guys are comfortable with that I would like to become one.

@earce
Copy link
Collaborator

earce commented May 24, 2020

also I figure this thread can be closed right?

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

No branches or pull requests

9 participants