Feature/fix perpetual futures net position of market #480

taiki1frsh · 2023-04-10T06:10:50Z

close #479

Besides, implemented ClosePosition unit test

fix closing fee

…stance remove SizeInMicro from instance in proto and treat by using SizeInDe…

fix closing fee

…nOfMarket

…sts of it

kimurayu45z · 2023-04-10T09:00:55Z

proto/derivatives/perpetual_futures.proto

+  PositionType position_type = 2 [
+    (gogoproto.moretags) = "yaml:\"position_type\""
+  ];
+  string position_size_in_denom_unit = 3 [


Is it difficult to make this PositionSize as sdk.Dec?

No,
should it be sdk.Dec?

But, it's treated as sdk.Int inside

Interface that is exposed to API users should be treated as well as other places. PositionSize is treated in Dec in Msg for example so should follow it.

no, this type is not exposed as API at this moment

If so, it is ok to use Int.

just denom_exponent is better I think instead of denom_unit

yeah, it makes sense.
I will modify and merge this.

cr-gpt · 2023-04-10T09:20:41Z

docs/core/proto-docs.md

@@ -1695,7 +1695,8 @@ Query defines the gRPC querier service.
 | Field | Type | Label | Description |
 | ----- | ---- | ----- | ----------- |
 | `market` | [Market](#ununifi.derivatives.Market) |  |  |
-| `position_size_in_micro` | [string](#string) |  |  |
+| `position_type` | [PositionType](#ununifi.derivatives.PositionType) |  |  |
+| `position_size_in_denom_exponent` | [string](#string) |  |  |





with a brief code review:

This patch looks like it is adding a new field called position_type and replacing the existing position_size_in_micro field with position_size_in_denom_exponent. Both of these fields are of type PositionType and string respectively. No bugs appear to be present in the patch, however it is recommended to ensure that the new field is properly initialized and the data types are correct. Additionally, documentation should be added if necessary to explain what the fields represent and how they will be used.

cr-gpt · 2023-04-10T09:20:46Z

proto/derivatives/perpetual_futures.proto

+    (gogoproto.moretags) = "yaml:\"position_type\""
+  ];
+  string position_size_in_denom_exponent = 3 [
+    (gogoproto.moretags) = "yaml:\"position_size_in_denom_exponent\"",
    (gogoproto.customtype) = "github.com/cosmos/cosmos-sdk/types.Int",
    (gogoproto.nullable) = false
  ];


The code patch looks good, however there are a few issues that could be improved upon. Firstly, the variable names should be more descriptive. For example, the variable "position_size_in_micro" should be "position_size_in_denom_exponent". Secondly, the variable names should be consistent with the rest of the codebase. Thirdly, there should be comments to explain what each variable is used for and how it is calculated. Finally, the code should be tested thoroughly to ensure that all changes work as expected.

cr-gpt · 2023-04-10T09:20:51Z

x/derivatives/types/perpetual_futures.go

-		PositionSizeInMicro: position_size_in_micro,
+		Market:                      market,
+		PositionType:                positionType,
+		PositionSizeInDenomExponent: position_size_in_denom_exponent,
 	}
 }



with the code review:

The code looks good and there are no obvious bugs. The main improvement that I suggest is to add more comments to explain why the code is written in a certain way. This can help others understand the code better and also make maintenance easier. In addition, it would be beneficial to add unit tests to ensure that the code works as expected.

github-actions · 2023-04-10T09:20:52Z

docs/core/proto-docs.md

@@ -1695,7 +1695,8 @@ Query defines the gRPC querier service.
 | Field | Type | Label | Description |
 | ----- | ---- | ----- | ----------- |
 | `market` | [Market](#ununifi.derivatives.Market) |  |  |
-| `position_size_in_micro` | [string](#string) |  |  |
+| `position_type` | [PositionType](#ununifi.derivatives.PositionType) |  |  |
+| `position_size_in_denom_exponent` | [string](#string) |  |  |





The code patch changes the Query gRPC service by adding two fields: position_type of type PositionType and position_size_in_denom_exponent of type string.

Based on the information provided, it is not possible to identify any bug risks in this code patch. However, some suggestions for improvement are:

Providing more information about the purpose and usage of the new fields can help with understanding the changes made to the Query service.

Adding validation logic for the input values of position_size_in_denom_exponent can improve the reliability and security of the code.

github-actions · 2023-04-10T09:21:07Z

proto/derivatives/perpetual_futures.proto

+    (gogoproto.moretags) = "yaml:\"position_type\""
+  ];
+  string position_size_in_denom_exponent = 3 [
+    (gogoproto.moretags) = "yaml:\"position_size_in_denom_exponent\"",
    (gogoproto.customtype) = "github.com/cosmos/cosmos-sdk/types.Int",
    (gogoproto.nullable) = false
  ];


The code patch seems to be adding two new fields to the PerpetualFuturesNetPositionOfMarket message.

The first field is a position_type of type PositionType. It has a YAML tag "yaml:\"position_type\"". The PositionType type should be defined elsewhere in the codebase, and it would be helpful to have more information about what values it can take.

The second field is position_size_in_denom_exponent of type string. It has a YAML tag "yaml:\"position_size_in_denom_exponent\"". This field also has additional Go protocol buffer tags, including "github.com/cosmos/cosmos-sdk/types.Int" for the custom type definition and "(gogoproto.nullable) = false" to ensure that the value is not nullable. However, the name of the field is somewhat unclear, as it suggests that the value is an exponent but it is stored as a string. Some additional clarification or documentation on this field would be helpful.

Without further information about the rest of the codebase and its functionality, it is difficult to identify specific risk areas or improvement suggestions.

github-actions · 2023-04-10T09:21:16Z

x/derivatives/types/perpetual_futures.go

-		PositionSizeInMicro: position_size_in_micro,
+		Market:                      market,
+		PositionType:                positionType,
+		PositionSizeInDenomExponent: position_size_in_denom_exponent,
 	}
 }



The code changes seem fine overall.

One suggestion would be to add input validation for the NewPerpetualFuturesNetPositionOfMarket function, especially for the position_size_in_denom_exponent argument since it is now taking user-input in denomination unit. Additionally, you could add comments or documentation about what PositionType entails and how it should be used in this function.

Another point to consider is whether the PositionType field can capture all the necessary information needed for a perpetual futures position. If you find that more information is needed, you might want to consider adding additional fields to this struct.

Overall, there don't appear to be any major bug risks in this code patch.

taiki1frsh added 11 commits April 6, 2023 14:09

psize to micro scale

67f4841

fix closing fee retreving

c7c0eab

modify field

7d8500e

support modifed perpmarket type

121d9a6

fix genesis test

b041755

Merge pull request #475 from UnUniFi/feature/fix-closing-fee

78bd900

fix closing fee

Merge pull request #477 from UnUniFi/patch/remove-SizeInMicro-from-in…

459683e

…stance remove SizeInMicro from instance in proto and treat by using SizeInDe…

Merge pull request #475 from UnUniFi/feature/fix-closing-fee

cd48d2c

fix closing fee

Merge branch 'newDevelop' into feature/fix-PerpetualFuturesNetPositio…

1f3f448

…nOfMarket

fix error

83f3e15

insert perpmarket net position handling in close position and impl te…

6d5696b

…sts of it

taiki1frsh marked this pull request as ready for review April 10, 2023 08:57

taiki1frsh requested a review from kimurayu45z April 10, 2023 08:57

kimurayu45z reviewed Apr 10, 2023

View reviewed changes

kimurayu45z approved these changes Apr 10, 2023

View reviewed changes

fix unit to exponent for explicity

73cb690

cr-gpt bot reviewed Apr 10, 2023

View reviewed changes

github-actions bot reviewed Apr 10, 2023

View reviewed changes

taiki1frsh merged commit 363fbc3 into newDevelop Apr 10, 2023

taiki1frsh deleted the feature/fix-PerpetualFuturesNetPositionOfMarket branch April 10, 2023 09:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/fix perpetual futures net position of market #480

Feature/fix perpetual futures net position of market #480

taiki1frsh commented Apr 10, 2023 •

edited

Loading

kimurayu45z Apr 10, 2023

taiki1frsh Apr 10, 2023

taiki1frsh Apr 10, 2023

kimurayu45z Apr 10, 2023

taiki1frsh Apr 10, 2023

kimurayu45z Apr 10, 2023

taiki1frsh Apr 10, 2023

cr-gpt bot Apr 10, 2023

cr-gpt bot Apr 10, 2023

cr-gpt bot Apr 10, 2023

github-actions bot Apr 10, 2023

github-actions bot Apr 10, 2023

github-actions bot Apr 10, 2023

Feature/fix perpetual futures net position of market #480

Feature/fix perpetual futures net position of market #480

Conversation

taiki1frsh commented Apr 10, 2023 • edited Loading

kimurayu45z Apr 10, 2023

Choose a reason for hiding this comment

taiki1frsh Apr 10, 2023

Choose a reason for hiding this comment

taiki1frsh Apr 10, 2023

Choose a reason for hiding this comment

kimurayu45z Apr 10, 2023

Choose a reason for hiding this comment

taiki1frsh Apr 10, 2023

Choose a reason for hiding this comment

kimurayu45z Apr 10, 2023

Choose a reason for hiding this comment

taiki1frsh Apr 10, 2023

Choose a reason for hiding this comment

cr-gpt bot Apr 10, 2023

Choose a reason for hiding this comment

cr-gpt bot Apr 10, 2023

Choose a reason for hiding this comment

cr-gpt bot Apr 10, 2023

Choose a reason for hiding this comment

github-actions bot Apr 10, 2023

Choose a reason for hiding this comment

github-actions bot Apr 10, 2023

Choose a reason for hiding this comment

github-actions bot Apr 10, 2023

Choose a reason for hiding this comment

taiki1frsh commented Apr 10, 2023 •

edited

Loading