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

Websocket support #18

Merged
merged 6 commits into from
Feb 19, 2024
Merged

Websocket support #18

merged 6 commits into from
Feb 19, 2024

Conversation

m30m
Copy link
Collaborator

@m30m m30m commented Feb 15, 2024

No description provided.

Copy link
Contributor

@anihamde anihamde left a comment

Choose a reason for hiding this comment

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

just a few comments from me. will leave it to someone more rust experienced to comment on the websocket code

store
.ws
.update_tx
.send(NewOpportunity(opportunity.clone().into()))
Copy link
Contributor

Choose a reason for hiding this comment

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

instead of cloning multiple times, maybe you could push opportunity into opportunities_existing, and then call

store.ws.update_tx.send(NewOpportunity(opportunities_existing.last().into()))

Copy link
Contributor

Choose a reason for hiding this comment

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

You also don't need to call opportunity.into() below, you could instead just create a new OpportunityParamsWithMetadata object and fill in.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

For the first comment, we technically need opportunity in both places and both structs need to own them in the end so we need to clone.

I didn't understand the second comment, we implement the Into trait so we don't have to do this here.

let mut exit_check_interval = tokio::time::interval(EXIT_CHECK_INTERVAL);

// this should be replaced by a subscription to the chain and trigger on new blocks
let mut submission_interval = tokio::time::interval(Duration::from_secs(5));
Copy link
Contributor

Choose a reason for hiding this comment

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

the frequency here should depend on the chain_id, probably set in the config

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

agreed but out of scope for this PR, will do it separately

cloned_bids.sort_by(|a, b| b.bid.cmp(&a.bid));
tokio::select! {
_ = submission_interval.tick() => {
for (chain_id, chain_store) in &store.chains {
Copy link
Contributor

Choose a reason for hiding this comment

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

and relatedly have run_submission_loop take chain_id as an argument and run distinct loops for different chain_ids


let submission_loop = tokio::spawn(run_submission_loop(store.clone()));
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: since these are in different places I think it might be more natural to move this function entirely to main.rs

@@ -127,22 +139,30 @@ pub async fn post_opportunity(
}
Copy link
Contributor

Choose a reason for hiding this comment

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

The clone in iteration above seems a bit off. I don't know whether it's unnecessary but if you use a map then try_insert can give you this shorter. I see some searchs with the order id.s too.

@@ -25,3 +25,4 @@ utoipa-swagger-ui = { version = "3.1.4", features = ["axum"] }
serde_yaml = "0.9.25"
ethers = "2.0.10"
axum-macros = "0.4.0"
dashmap = { version = "5.4.0" }
Copy link
Contributor

Choose a reason for hiding this comment

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

Bump the auction-server version as well?

Copy link
Contributor

@ali-bahjati ali-bahjati left a comment

Choose a reason for hiding this comment

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

Nice!

@m30m m30m merged commit 72531f7 into main Feb 19, 2024
1 check passed
@m30m m30m deleted the ws branch February 19, 2024 16:56
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