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

Add some process_inbox calls for the matching_engine end to end test #2129

Closed

Conversation

MathieuDutSik
Copy link
Contributor

Motivation

Replacement of the error failed to post query with a more precise error message revealed the origin of some errors.

Proposal

Two entries of process_queries are added to the matching_engine.
It is possible that the iterative loop for make_application needs to be eliminated.

Test Plan

An improved CI is the objective of this work.

Release Plan

Not relevant.

Links

Comment on lines +1439 to +1441
node_service_admin.process_inbox(&chain_admin).await?;
node_service_a.process_inbox(&chain_a).await?;
node_service_b.process_inbox(&chain_b).await?;
Copy link
Contributor

Choose a reason for hiding this comment

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

Yes these ones look necessary.

@@ -1500,6 +1503,9 @@ async fn test_wasm_end_to_end_matching_engine(config: impl LineraNetConfig) -> R
&[token0.forget_abi(), token1.forget_abi()],
)
.await?;
node_service_admin.process_inbox(&chain_admin).await?;
node_service_a.process_inbox(&chain_a).await?;
node_service_b.process_inbox(&chain_b).await?;
Copy link
Contributor

Choose a reason for hiding this comment

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

These ones look too early. We want to make sure that the RequestApplication messages are processed, then their answers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I changed the position of the process_inbox statements.

@MathieuDutSik MathieuDutSik marked this pull request as ready for review June 13, 2024 12:13
Comment on lines 1527 to 1529
node_service_admin.process_inbox(&chain_admin).await?;
node_service_a.process_inbox(&chain_a).await?;
node_service_b.process_inbox(&chain_b).await?;
Copy link
Contributor

Choose a reason for hiding this comment

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

What messages are these waiting for?
I don't think admin and b are needed, and a should maybe move to after line 1513?

Copy link
Contributor

@ma2bd ma2bd Jun 13, 2024

Choose a reason for hiding this comment

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

@MathieuDutSik I believe the process_inbox are useful when they happen between request_application and make_application

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I changed the position of the process_inbox for a and b and eliminated it for admin.

@MathieuDutSik
Copy link
Contributor Author

This attempt was in the right direction but was superseded by PR #2144

@MathieuDutSik MathieuDutSik deleted the attempt_matching_engine branch June 18, 2024 15:10
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