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.

Sign up for GitHub

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

Jump to bottom

Replace Websockets with GRPC #20

Merged
merged 11 commits into from
Sep 5, 2024
Merged

Replace Websockets with GRPC #20

merged 11 commits into from
Sep 5, 2024

Conversation

webwarrior-ws
Copy link
Contributor

knocte
knocte reviewed Feb 8, 2024
View reviewed changes
src/FX.Tests/E2ETests.cs Outdated Show resolved Hide resolved
knocte
knocte reviewed Feb 8, 2024
View reviewed changes
src/FX.Tests/E2ETests.cs Outdated Show resolved Hide resolved
knocte
knocte reviewed Feb 8, 2024
View reviewed changes
src/FX.Tests/E2ETests.cs Outdated Show resolved Hide resolved
knocte
knocte reviewed Feb 8, 2024
View reviewed changes
.github/workflows/CI.yml Outdated Show resolved Hide resolved
@knocte
Copy link
Member

knocte commented Feb 8, 2024

@webwarrior-ws please split the e2e commit in two: first commit will work with normal HTTP, 2nd would change it to HTTPS

@webwarrior-ws
Copy link
Contributor Author

@webwarrior-ws please split the e2e commit in two: first commit will work with normal HTTP, 2nd would change it to HTTPS

It doesn't work with HTTP:

Grpc.Core.RpcException : Status(StatusCode="Unavailable", Detail="Error starting gRPC call. HttpRequestException: An HTTP/2 connection could not be established because the server did not complete the HTTP/2 handshake. (InvalidResponse) HttpIOException: An HTTP/2 connection could not be established because the server did not complete the HTTP/2 handshake. (InvalidResponse) HttpIOException: The response ended prematurely while waiting for the next frame from the server. (ResponseEnded)", DebugException="System.Net.Http.HttpRequestException: An HTTP/2 connection could not be established because the server did not complete the HTTP/2 handshake. (InvalidResponse)

@knocte
Copy link
Member

knocte commented Feb 8, 2024

It doesn't work with HTTP:

It works in RunIntoMe. In fact it's what we have in production.

@webwarrior-ws webwarrior-ws force-pushed the websocket-to-grpc-squashed branch 2 times, most recently from 33ca1ba to 2ee88ec Compare February 8, 2024 10:19
@webwarrior-ws
Copy link
Contributor Author

Got it to work with HTTP. Split commit into 2.

@knocte
Copy link
Member

knocte commented Feb 8, 2024

CI of 1st commit is red, please fix; after that I'll merge

@webwarrior-ws webwarrior-ws force-pushed the websocket-to-grpc-squashed branch 2 times, most recently from 1c811c5 to a8e60f8 Compare February 8, 2024 11:22
@webwarrior-ws
Copy link
Contributor Author

CI for all commits is green now.

@knocte
Copy link
Member

knocte commented Feb 8, 2024

I just realized that the WebSocket proj already had the Core project hooked up (see src/WebSocketApp/Models.fs), so:

  • In the last commit, let's move the warningsAsError settings to Directory.Build.Props file so that it applies to all projs.
  • Let's add one more extra commit that hooks to same operations as we had in src/WebSocketApp/Models.fs. For this you might need a new project (not 100% sure) shared between GrpcClient and GrpcService, so that they can serialize/deserialize to/from JSON to extract/encode request details.

@webwarrior-ws
Copy link
Contributor Author

@knocte I added models and client/server interop.

@knocte
Copy link
Member

knocte commented Feb 8, 2024

@webwarrior-ws great, but you didn't move WarningsAsErrors yet

@webwarrior-ws
Copy link
Contributor Author

Moved TreatWarningsAsErrors to Directory.Build.props.

knocte
knocte reviewed Feb 9, 2024
View reviewed changes
Directory.Build.props Outdated Show resolved Hide resolved
@webwarrior-ws webwarrior-ws force-pushed the websocket-to-grpc-squashed branch 2 times, most recently from bc5181a to fb814ad Compare September 4, 2024 13:27
@knocte
Copy link
Member

knocte commented Sep 5, 2024

I have concerns with 2 of the commits you pushed yesterday:

  • Tests: clean DB after run in tests: why is its CI red?
  • Core,Grpc.Service: make SendMarketOrder return: this is wrong, market order can never match partially (unless a liquidity problem is found, in which case an exception will be raised; that's why it returns unit)

@webwarrior-ws
Copy link
Contributor Author

  • Tests: clean DB after run in tests: why is its CI red?

Because old version of test expected first LimitOrder call to be a full match. Which was wrong, but worked due to not cleaning DB.

@knocte
Copy link
Member

knocte commented Sep 5, 2024

Because old version of test expected first LimitOrder call to be a full match. Which was wrong, but worked due to not cleaning DB.

Are you saying that these cleanup exposed a bug? Then explain in commit msg this fact so that it is understood why CI is red -> because CI is catching the bug that will be fixed later?

@webwarrior-ws webwarrior-ws force-pushed the websocket-to-grpc-squashed branch 2 times, most recently from 7cb299d to ee31d03 Compare September 5, 2024 08:56
@knocte
Copy link
Member

knocte commented Sep 5, 2024

Last but not least, these 3 commits:

  • GrpcModels,Core: move Match converter -> can be squashed to the commit that introduces this converter for the first time
  • Directory.Packages.props: update STJ -> can be squashed? if this is already in master, then put this as the 1st commit of the PR please
  • Core: fix typo in MatchTypeConverter -> can be squashed to the commit that introduces this converter for the first time

@webwarrior-ws
Copy link
Contributor Author

  • Directory.Packages.props: update STJ -> can be squashed? if this is already in master, then put this as the 1st commit of the PR please

I don't see any commit to squash this with. It is not connected to any other work in this PR, but is necessary for CI to succeed now as version 8.0.1 got marked as vulnerable.

@knocte
Copy link
Member

knocte commented Sep 5, 2024

I don't see any commit to squash this with. It is not connected to any other work in this PR, but is necessary for CI to succeed now as version 8.0.1 got marked as vulnerable.

Read the message again, I gave a hint of what to do in that case.

@webwarrior-ws
Copy link
Contributor Author

You only said what to do if that commit is in master (it is not).

@knocte
Copy link
Member

knocte commented Sep 5, 2024

I meant if STJ is already in master.

@webwarrior-ws
Directory.Packages.props: update STJ
db64f15 
Update System.Text.Json library to version 8.0.4 since
previously used version (8.0.1) contains vlunerabilities.
@webwarrior-ws
Replace WebSocketApp proj with GRPC*
844b45f 
Removed WebSocket project and added GRPC projects from RIM
(copy&paste).

Then removed unused stuff from GRPC* projects and specified
package versions in Directory.Packages.props.
@webwarrior-ws
Tests,GrpcClient,GrpcServer: made e2e GRPC test
60642f4 
Made end-to-end GRPC test that makes sure GRPC communication is
working.
@webwarrior-ws
CI,GrpcClient,Tests: use HTTPS connection
dfbecad 
Use HTTPS connection for GRPC communtication.

Also added commands to CI that trust ASP.NET certificate on
Linux because otherwise GRPC communication fails.
@webwarrior-ws
Enable TreatWarningsAsErrors
b75f01b 
Enable TreatWarningsAsErrors in Directory.Build.props.
Exception is CS8981 (lowercase type names) in GrpcClient and
GrpcService projects because it is triggered by auto-generated
protobuf types.
@webwarrior-ws
Add GrpcModels project for server/client interop
90fe289 
Added GrpcModels project for server/client interop which
includes message models (that were in WebSocketApp), as well
as Marshalling module (code from RIM).

Use serialization options defined in Core/RedisStorageLayer.fs
in Marshalling module. This is needed for F# types support.
@knocte
Copy link
Member

knocte commented Sep 5, 2024

Last but not least, these 3 commits:

Only one of these has been addressed.

@webwarrior-ws
Core,GrpcModels: Match & Option json converters
7b4e449 
Implemented json converters for Match and Option<'T> types.

Match type converter is placed in GrpcModels.ModelSerialization
since it's not used in Redis, but only in GRPC.
@webwarrior-ws
Tests: use message that responds in e2e test
549c4f4 
Modified e2e test to use message that returns response so that
serilization is tested.
@webwarrior-ws
Tests: clean DB after run in tests
2796a76 
Clean DB after each test run in BasicTests, MakerOnlyOrders,
MarketOrders, and LimitOrders test classes. This is needed for
all tests to start from empty state.

This commit exposes bug in end2end test - response to the first
LimitOrder should be None, not Some(Full). It will be fixed
later.
@webwarrior-ws
Tests: expand GrpcE2ETest to use Partial
c7708ca 
Expand GrpcE2ETest so that Match.Partial case is used and
any serialization errors cancerning it will be caught by this
test.
@webwarrior-ws
GrpcModels: fix in Match type deserialization
75ecbfd 
Fix in Match type deserialization (Partial case).
@webwarrior-ws
Copy link
Contributor Author

Squashed json converters commits

@knocte knocte merged commit c28a23d into nblockchain:master Sep 5, 2024
2 checks passed
@knocte knocte mentioned this pull request Sep 5, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@knocte knocte knocte left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@webwarrior-ws @knocte