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

perp positions changes #63

Merged
merged 19 commits into from
Jan 13, 2023
Merged

perp positions changes #63

merged 19 commits into from
Jan 13, 2023

Conversation

tinystarinagalaxy
Copy link
Contributor

No description provided.

proto/api.proto Outdated
option (grpc.gateway.protoc_gen_openapiv2.options.openapiv2_operation) = {
description: "Returns the list current perp positions";
summary: "PERP positions";
tags: ["Trade", "Universal"];
Copy link

Choose a reason for hiding this comment

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

Think this should be Orderbook, not Universal? Also, probably worth adding a "Perp" tag too

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added "Perp", "Orderbook" tags

proto/api.proto Outdated
option (grpc.gateway.protoc_gen_openapiv2.options.openapiv2_operation) = {
description: "closes current perp positions";
summary: "PERP positions";
tags: ["Trade", "Universal"];
Copy link

Choose a reason for hiding this comment

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

same comment ehre

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added "Perp", "Orderbook" tags

proto/api.proto Outdated
repeated string transactions = 1;
}

message GetCurrentPerpPosition {
Copy link

Choose a reason for hiding this comment

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

naming: PerpPosition?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

renamed

Copy link

Choose a reason for hiding this comment

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

I think also get rid of the Get in front of the name. This doesn't have to just be part of a Get requests and is a more general datatype

proto/api.proto Outdated
option (grpc.gateway.protoc_gen_openapiv2.options.openapiv2_operation) = {
description: "post a perp order";
summary: "perp order";
tags: ["Trade", "Orderbook"];
Copy link

Choose a reason for hiding this comment

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

Perp tag here too

proto/api.proto Outdated

rpc GetPerpPositions(GetPerpPositionsRequest) returns (GetPerpPositionsResponse) {
option (google.api.http) = {
get: "/api/v1/trade/perp-positions"
Copy link

Choose a reason for hiding this comment

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

trade/perp/positions I think

proto/api.proto Outdated

rpc PostClosePerpPositions(PostClosePerpPositionsRequest) returns (PostClosePerpPositionsResponse) {
option (google.api.http) = {
get: "/api/v1/trade/close-perp-positions"
Copy link

Choose a reason for hiding this comment

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

/trade/perp/close-positions I think

proto/api.proto Outdated
repeated string transactions = 1;
}

message GetCurrentPerpPosition {
Copy link

Choose a reason for hiding this comment

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

I think also get rid of the Get in front of the name. This doesn't have to just be part of a Get requests and is a more general datatype

Copy link

@aspin aspin left a comment

Choose a reason for hiding this comment

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

couple more suggestions. good when these are done though

Copy link

@aspin aspin left a comment

Choose a reason for hiding this comment

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

take a look at the last comment then LGTM

proto/api.proto Outdated
Project project = 1;
string ownerAddress = 2;
string accountAddress = 3;
repeated common.PerpContract contracts = 4;
}

message GetPerpPositionsResponse {
message PerpPositionsResponse {
Copy link

Choose a reason for hiding this comment

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

think you misunderstood my comment here. These messages should have the Get in front of it. GetPerpPosition (field #3 here) is the one that shouldn't

Copy link

Choose a reason for hiding this comment

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

the messages that match the RPC method (e.g. GetPerpPositions(PerpPositionsRequest) returns (PerpPositionsResponse)) should always have the Get prefix. It's jsut the data types within the Request/Response objects that should not

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed this

@tinystarinagalaxy tinystarinagalaxy merged commit 565ce39 into develop Jan 13, 2023
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