Feature/fix metrics type handling #507
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| MarginMaintenanceRate: perpetualFuturesPosition.MarginMaintenanceRate(baseMetricsRate, quoteMetricsRate), | ||
| EffectiveMargin: sdk.NewCoin("uusd", perpetualFuturesPosition.EffectiveMarginInMetrics(baseMetricsRate, quoteMetricsRate)), | ||
| EffectiveMargin: sdk.NewCoin("uusd", types.NormalToMicroInt(perpetualFuturesPosition.EffectiveMarginInMetrics(baseMetricsRate, quoteMetricsRate))), | ||
| }, nil | ||
| } | ||
|
|
There was a problem hiding this comment.
The code patch adds comments explaining the meaning and units of the variables "InMetrics" and points out that it's not calculated in micro-case. There seem to be two improvements that can be made:
- Avoid using
sdk.Coindirectly in theMakeQueriedPositionsfunction to reduce coupling with the Cosmos SDK; instead, create a new type for representing profit/loss and effective margin amounts. - Use the
types.NormalToMicroIntfunction to convert from normal integers to microintegers consistently for profit/loss amounts and effective margin in thePositionfunction.
| @@ -153,6 +154,7 @@ func (suite *KeeperTestSuite) TestClosePerpetualFuturesPosition() { | |||
| QuoteDenom: "uusdc", | |||
| } | |||
|
|
|||
| // TODO: Check the returning amount to the owner | |||
| positions := []struct { | |||
| positionId string | |||
| margin sdk.Coin | |||
There was a problem hiding this comment.
Based on the code patch provided, here are some observations and suggestions:
Observations:
- The code patch seems to be a test suite written in Golang.
- There are two test functions,
TestOpenPerpetualFuturesPositionandTestClosePerpetualFuturesPosition, which contain test cases for opening and closing of perpetual futures positions. - There are two newly added comments which contain
TODOstatements.
Suggestions:
- The
TODOstatement inTestOpenPerpetualFuturesPositionsuggests adding a failure case when there is a lack of available asset in the pool. It is important to add such cases as it helps ensure proper handling of edge cases and can improve the robustness of the code. - The
TODOstatement inTestClosePerpetualFuturesPositionsuggests checking the returning amount to the owner. Adding this check would help ensure that the correct amount is returned to the owner after the position is closed. - It is recommended to perform thorough testing of the code to identify any potential bugs or issues before deployment to production.
jununifi
reviewed
Apr 27, 2023
| @@ -16,6 +16,7 @@ func (suite *KeeperTestSuite) TestOpenPerpetualFuturesPosition() { | |||
| QuoteDenom: "uusdc", | |||
| } | |||
|
|
|||
| // TODO: add failure case due to the lack of the available asset in the pool | |||
There was a problem hiding this comment.
Can you implement all TODOs on this PR?
There was a problem hiding this comment.
I asked @Senna46 to implement in the other branch.
So, please skip now.
jununifi
reviewed
Apr 27, 2023
| t.Errorf("expected %v, got %v", tc.exp, err) | ||
| } | ||
| } | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| // TODO: impl TestRemaingMarginInMetrics |
There was a problem hiding this comment.
Could remove this since it's implemented
jununifi
approved these changes
Apr 27, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
close issue #504