search

wisespace-io / binance-rs

Rust Library for the Binance API
Other
636 stars 287 forks source link

feat: change from sync to async work #154

Closed asiman161 closed 1 year ago

asiman161 commented 2 years ago

Hello! I started to look Binance rust clients and only this was actual. I saw some issues that users don't want to use this lib because it's sync. (as me. I forked it to fix this problem. I don't want to use workaround. Write my own lib or fork this and fix) Today I don't know why it should work with sync only (historical? then I want to have async version) and I thought that it will be useful to move it to async API.

I checked examples and tests and it works.

danieleades commented 1 year ago

@asiman161 i'm afraid i made a couple of PRs that may mean you need to rebase.

I hope this PR gets merged soon! I think 'async-first' is the right approach for API clients

asiman161 commented 1 year ago

@Sydney-o9 Hello! I just read your message :) I can update my MR in 1-2 days

asiman161 commented 1 year ago

@danieleades I updated PR to actual repo version so you can check it

danieleades commented 1 year ago

you'll need @wisespace-io to trigger the workflows for you

asiman161 commented 1 year ago

I suspect this will need a rustfmt pass before the workflows are happy.

months ago this PR has passed workflow but I updated this PR with cargo fmt to avoid any extra problems

testforvln99 commented 1 year ago

I'm only a common user of binance-rs, but I don't suggest this PR should be merged.

  1. Async rust is not always more efficient than sync rust. My trading system is constructed with full of sync rust modules and its performance beats async frameworks.
  2. The async functions are contagious. If this PR is merged, many people's systems relied on this crate would have to be rewritten. I don't think such changes with serious consequences should be introduced easily.
  3. If you really want to use async runtime, you can easily find async binance crates on crates.io.
asiman161 commented 1 year ago

@testforvln99 I can't find async crates that's not outdated. Async is future and more simpler to write fast apps. The only problem with async that it requires runtime... But it's almost free.

The only 1 reason that I agree with you is this PR is dangerous because it breaks back compatibility and forces you to use async.

I want to get approve for this PR from @danieleades or get rejected, but we have no answer about it

danieleades commented 1 year ago

I want to get approve for this PR from @danieleades or get rejected, but we have no answer about it

I don't have any authority to approve or reject a PR in this repo

testforvln99 commented 1 year ago

@testforvln99 I can't find async crates that's not outdated. Async is future and more simpler to write fast apps. The only problem with async that it requires runtime... But it's almost free.

The only 1 reason that I agree with you is this PR is dangerous because it breaks back compatibility and forces you to use async.

I want to get approve for this PR from @danieleades or get rejected, but we have no answer about it

When it comes to IO-intensive applications, async systems are simpler. But to CPU-intensive applications, async system design may not be a good idea. For async crates, have you looked through https://github.com/Igosuki/binance-rs-async ?

wisespace-io commented 1 year ago

@asiman161 @danieleades @testforvln99 Sorry, I am quite busy. Some years ago we had agreed to keep this project as a sync version as we had other implementations taking the async approach. I am not following these projects to know if someone is maintaining or not, I recall that many attempts happened before. It seems that https://github.com/Igosuki/binance-rs-async got some love recently. You can also check, https://github.com/nash-io/openlimits/tree/main/crates/openlimits-binance which used this project as inspiration in the beginning. If you will maintain, I would suggest to fork binance-rs, you put a lot of effort in your PR, so I think it would be the best thing to do.

testforvln99 commented 1 year ago

@asiman161 @danieleades @testforvln99 Sorry, I am quite busy. Some years ago we had agreed to keep this project as a sync version as we had other implementations taking the async approach. I am not following these projects to know if someone is maintaining or not, I recall that many attempts happened before. It seems that https://github.com/Igosuki/binance-rs-async got some love recently. You can also check, https://github.com/nash-io/openlimits/tree/main/crates/openlimits-binance which used this project as inspiration in the beginning. If you will maintain, I would suggest to fork binance-rs, you put a lot of effort in your PR, so I think it would be the best thing to do.

Thanks for your effort and reply!

asiman161 commented 1 year ago

OK. If you speak that this project will be always sync then I should close this PR because it will never be merged.