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

Consider adding repeatable future PoC #1558

Closed
sergeyboyko0791 opened this issue Nov 27, 2022 · 5 comments · Fixed by #1564
Closed

Consider adding repeatable future PoC #1558

sergeyboyko0791 opened this issue Nov 27, 2022 · 5 comments · Fixed by #1564
Assignees

Comments

@sergeyboyko0791
Copy link

sergeyboyko0791 commented Nov 27, 2022

I've been wanting for a repeatable futures for a long time, and I guess I got a solution using a macro.
Let's see an example:
https://github.com/KomodoPlatform/atomicDEX-API/blob/dff315f90c1ede37e5ccfcc5dec9018f6dd40b82/mm2src/coins/utxo/utxo_builder/utxo_coin_builder.rs#L820-L841

The main idea is to capture the code of the async block and return it from an Fn closure:
https://github.com/KomodoPlatform/atomicDEX-API/blob/dff315f90c1ede37e5ccfcc5dec9018f6dd40b82/mm2src/common/patterns/repeatable.rs#L14-L16

Currently, I added support for the future attempts, but I'm looking forward to continue the implementation, for example, repeat-until feature:

repeatable!(async {
  match rpc_client.version().await {
    Ok(version) => ready!(version),
    Err(_) => retry!(),
  }
})
.repeat_until(now_ms() + 1000 * 10) // Repeat until now + 10 seconds.
. await;

Current implementation is available here

@sergeyboyko0791
Copy link
Author

@shamardy @artemii235 @ozkanonur do you think it's worth to proceed with the implementation?

@shamardy
Copy link
Collaborator

shamardy commented Nov 28, 2022

do you think it's worth to proceed with the implementation?

I like the idea :) We use loops in a lot of places for the same functionalities and this will make the code much more readable.
@artemii235 proposed a similar idea in a previous review of a PR of mine here #1240 (comment) but I couldn't implement it using a trait that's implemented for Future.

@sergeyboyko0791
Copy link
Author

Yeah, I lost this comment, but I was inspired by exactly it 😁
I also couldn't find the way to repeat a complete future adding a RepeatableFuture trait, but found a workaround using the macro

@onur-ozkan
Copy link
Member

This looks good. We have to many duplicated loops in mm2 source code. We can replace them using this.

@sergeyboyko0791 sergeyboyko0791 self-assigned this Dec 4, 2022
@sergeyboyko0791 sergeyboyko0791 linked a pull request Dec 4, 2022 that will close this issue
artemii235 pushed a commit that referenced this issue Dec 27, 2022
* Add `repeatable` future PoC

* Use the `repeatable` future as an example at `utxo_coin_builder.rs`

* Move `repeatable.rs` to `common/custom_futures` mod

* Refactor `custom_futures`
* Remove `SendAll` future

* Further `repeatable` improvements

* Add `Repeatable::inspect_err`
* Add the possibility to convert `Result<T, E>` into `Action<T, E>`

* Add `RepeatUntil` future

* Integrate `repeatable` futures into mm2 tests

* Add `retry_on_err` macro that expects from the future to return a result

* Apply `retry_on_err` for spv, `UTXO::validate_payment`

* Fill doc comments

* Fix `test_hd_utxo_tx_history` test

* Combine `RepeatUntil` and `RepeatAttempts` into `Repeatable` future

* Fix warn messages

* Add `TODO` messages

* Remove `ready`, `retry` macros

* Fix PR issues

* Allow the user to specify only `until_ms`
* Avoid declaring `exec_fut: F` and `repeat_every: Duration` optional

* Fix PR issues

* Add `RepeatUntil` enum
* Set the default `repeat_every` to 1s
* Panic on `total_attempts == 0`

* Set the `RepeatUntil` to 1 attempt by default
@sergeyboyko0791
Copy link
Author

Close as solved

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 a pull request may close this issue.

3 participants