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

OKX solver update #109

Merged
merged 9 commits into from
Feb 7, 2025
Merged

OKX solver update #109

merged 9 commits into from
Feb 7, 2025

Conversation

mstrug
Copy link
Collaborator

@mstrug mstrug commented Feb 5, 2025

Summary

Addressing PR #104 comments.

Details

  1. Replaced lru-cache crate with moka to improve inserting data into the cache (now using segmentation blocking).
  2. Sending /swap and /approve-transaction requests in parallel to improve latency.
  3. Moved preparation and sending of the API requests to separate function which is now instrumented using one tracing span for both API calls.
  4. Added test which checks if cache is used (evaluating two swaps of the same tokens should result in only one /approve-transaction request).

How to test

Existing tests and newly added test.

@mstrug mstrug marked this pull request as ready for review February 6, 2025 12:06
swap_request_future,
self.dex_approved_addresses
.try_get_with(order.sell, approve_transaction_request_future)
.map_err(|_: std::sync::Arc<Error>| Error::CacheEvaluationFailed(order.sell))
Copy link
Collaborator Author

@mstrug mstrug Feb 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had to convert the error from Arc<Error> to Error in such a way, because Error cannot be cloned due to Http error. I think this is enough because in an error situation Http error can be checked by enabling trace level logs for instance.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, do you need to specify std::sync::Arc<Error>? I think just |_| should do unless you want to make it apparent the error type.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it is needed because compiler is not resolving automatically error type of the approve_transaction_request_future. Alternatively I could define return type inside the future like: Ok::<_, Error>(...) but I thought it looked worse. However now I'm thinking it may be better to define that type in the future, because the future itself provides a hint what is its return type (instead of providing type hint when this future is used by cache).

Copy link
Contributor

@squadgazzz squadgazzz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mstrug mstrug merged commit 0fcbfe5 into main Feb 7, 2025
3 checks passed
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 this pull request may close these issues.

3 participants