Feature/reserve tokens to pay profit #503

taiki1frsh · 2023-04-25T15:05:47Z

close #451

Besides solving 451, I added queries for the available asset.

Created KVStore to store the information of the reserved asset in the pool for the position
Validate opening position to check if the liquidity is enough
Validate burning DLP to check if the available asset in the pool is enough

github-actions · 2023-04-25T15:06:12Z

proto/derivatives/derivatives.proto

-  repeated Breakdown breakdown = 3 [
-    (gogoproto.moretags) = "yaml:\"breakdown\"",
+  repeated AssetInfo asset_info = 3 [
+    (gogoproto.moretags) = "yaml:\"asset_info\"",
    (gogoproto.nullable) = false
  ];
 }


This code patch modifies the PoolMarketCap message in a protocol buffer file. It renames the Breakdown nested message to AssetInfo, adds a new reserved field of type github.com/cosmos/cosmos-sdk/types.Int, and changes the repeated breakdown field to asset_info. The changes seem reasonable and do not appear to introduce any obvious bugs. One potential improvement suggestion could be to provide more descriptive names for the fields and messages to improve code readability.

github-actions · 2023-04-25T15:06:20Z

proto/derivatives/query.proto

+    (gogoproto.moretags)  = "yaml:\"available_assets\"",
+    (gogoproto.nullable) = false
+  ];
+}


Based on the code patch provided, it appears that additions are being made to a service called "Query." Two new RPC methods have been added: "AvailableAssetInPoolByDenom" and "AvailableAssetsInPool".

"AvailableAssetInPoolByDenom" takes in a query parameter, "denom," and returns a single coin named "available_asset" from an endpoint specified in the HTTP GET option. Meanwhile, "AvailableAssetsInPool" has no parameters and returns a list of coins named "available_assets" from another endpoint specified in the HTTP GET option.

Without additional information, it is difficult to identify potential bugs or improvements. However, it is important to thoroughly test these changes to ensure compatibility with any other related code and to implement any necessary error handling.

github-actions · 2023-04-25T15:07:15Z

x/derivatives/client/cli/query.go

+
+	return cmd
+
+}


Based on the provided code patch, here are some suggestions that can be considered for potential improvements:

CmdQueryAvailableAssetsInPool() has an unnecessary Args check that can be removed since it does not take any arguments.

The Comments at line 7 and 11 should be removed to avoid any confusion.

It is recommended to add some test cases for these commands to ensure smooth functioning.

Review the flags in AddQueryFlagsToCmd function to determine if better options exist.

github-actions · 2023-04-25T15:07:25Z

x/derivatives/keeper/grpc_query.go

+	return &types.QueryAvailableAssetsInPoolResponse{
+		AvailableAssets: availableAssets,
+	}, nil
+}


This code patch shows two new functions (AvailableAssetInPoolByDenom and AvailableAssetsInPool) being added to an existing Keeper struct.

The new functions appear to be related to querying the available assets in a pool. Both functions check for a nil request object and return an error with an appropriate message if one is found.

In AvailableAssetInPoolByDenom, the function makes a call to AvailableAssetInPool passing in the context and the denomination extracted from the request object. If there is an error, it returns an error with an appropriate message as well. Otherwise, it returns a response object containing the available asset.

The AvailableAssetsInPool function calls AllAvailableAssetsInPool with the context object and returns a response with all the available assets or an error message if there was any problem.

There does not appear to be any bug risk in these two new functions. As for improvements, it would be good to document these new functions properly and update any relevant tests or documentation accordingly.

github-actions · 2023-04-25T15:10:50Z

x/derivatives/keeper/lpt.go

+	if availableAsset.IsLTE(redeemAmount) {
+		return types.ErrInsufficientAssetBalance
+	}
+
 	if redeemFee.IsPositive() {
 		// send redeem fee to fee pool
 		err = k.bankKeeper.SendCoinsFromModuleToModule(ctx, types.ModuleName, types.DerivativeFeeCollector, sdk.Coins{redeemFee})


The code patch looks good. One improvement could be adding more comments to explain the intent and logic of the code. As for the bug risks, one possible issue is with the AvailableAssetInPool function implementation - if it doesn't correctly calculate the available asset balance in the pool, it could result in incorrect validation of the redeemable amount. Another issue could arise if there are concurrency issues with the pool's asset balance being updated by other transactions while this function is executed, which could result in race conditions.

github-actions · 2023-04-25T15:11:05Z

x/derivatives/keeper/pool.go

+	}
+
+	return availableCoins, nil
+}


Code Review:

The code seems to be adding new features and updating existing ones based on the given patch. Here are some suggestions:

In the GetPoolMarketCap function, the variable breakdowns has been renamed to assetInfoList, which is more appropriate because it's a list of PoolMarketCap_AssetInfo.

It would be good to add validation checks to the SetReservedCoin function to ensure that the reserve amount being set is valid.

The AvailableAssetInPool function can benefit from some optimization as it queries the reserved coin value for each asset separately. It may be possible to get the reserved coins for all assets at once and then iterate through the assets to calculate their available balance.

The AllAvailableAssetsInPool function provides a useful feature but should also have some validation checks to handle errors when querying the available balance for an asset in the pool.

Overall, the code changes seem to be well-implemented and don't introduce any obvious bugs.

github-actions · 2023-04-25T15:11:14Z

x/derivatives/keeper/positions.go

+	if err := k.bankKeeper.SendCoinsFromAccountToModule(ctx, msg.Sender.AccAddress(), types.ModuleName, sdk.NewCoins(msg.Margin)); err != nil {
+		return err
+	}
+
 	return nil
 }



The code review suggests the following:

The code segment moves position ID generation before the error checking, which could result in wasted IDs or inconsistent state.

The comment "fixme check first bank.send last" should be updated to reflect the actual change.

The SendCoinsFromAccountToModule function should be called after the Position instance is validated and saved.

The addition at line 174 should cover the possibility of negative margin amounts to prevent errors.

Overall, the code changes appear safe and do not seem to introduce any new bugs.

github-actions · 2023-04-25T15:11:23Z

x/derivatives/keeper/yield.go

 		afterPrices[breakdown.Denom] = breakdown.Price
 	}

-	for _, breakdown := range poolMarketCapBefore.Breakdown {
+	for _, breakdown := range poolMarketCapBefore.AssetInfo {
 		amountBefore := breakdown.Amount
 		priceAfter, ok := afterPrices[breakdown.Denom]
 		if !ok {


The code snippet appears to be incomplete, as it only shows a portion of a function's implementation. However, based on the given code snippet, there is no apparent bug risk.

As for improvement suggestions, it may be helpful to add comments explaining the purpose of the variables and the logic behind the code. Additionally, consider using more descriptive variable names to improve maintainability and readability.

Furthermore, it may be beneficial to handle the case where afterPrices does not contain a particular Denom from poolMarketCapBefore, instead of ignoring it as currently done in the code.

github-actions · 2023-04-25T15:11:34Z

x/derivatives/keeper/yield_test.go

@@ -161,7 +161,7 @@ func (suite *KeeperTestSuite) TestGetLPRealYieldRate() {
 	suite.keeper.SetPoolMarketCapSnapshot(suite.ctx, 11, types.PoolMarketCap{
 		QuoteTicker: "uatom",
 		Total:       sdk.NewDec(200000000),
-		Breakdown: []types.PoolMarketCap_Breakdown{
+		AssetInfo: []types.PoolMarketCap_AssetInfo{
 			{
 				Denom:  "uatom",
 				Amount: sdk.NewInt(20000000),


This code patch seems to be updating the tests for a function that calculates yield rates for liquidity providers in a pool. The changes made seem to be related to updating the types used in the test data.

It's difficult to identify any bug risks or improvement suggestions without seeing the entire function being tested and its dependencies. However, it is worth noting that if the function being tested has been updated, the tests should be checked to ensure they are still valid with the updated code. Additionally, the names of some types could be improved to make them more descriptive and understandable.

github-actions · 2023-04-25T15:11:49Z

x/derivatives/types/errors.go

@@ -14,4 +14,5 @@ var (
 	ErrZeroLpTokenPrice         = sdkerrors.Register(ModuleName, 4, "zero lp token price")
 	ErrorMarginNotEnough        = sdkerrors.Register(ModuleName, 5, "margin is not enough")
 	ErrorInvalidPositionParams  = sdkerrors.Register(ModuleName, 6, "invalid param for position")
+	ErrInsufficientAssetBalance = sdkerrors.Register(ModuleName, 7, "insufficient asset balance")
 )


It appears that a single error type has been added with this code patch, ErrInsufficientAssetBalance, which is registered with sdkerrors.Register using a specific error code (7) and an associated error message.

Without seeing the rest of the codebase, it is difficult to determine if there are any specific bug risks associated with this change. However, in general, you may want to ensure that all possible paths in the code leading up to this error return have been properly audited to ensure that they correctly check for and handle insufficient asset balances. Additionally, you could consider adding more detailed information to the error message, such as which asset balance was insufficient and what action can be taken to remedy the situation.

As far as improvement suggestions go, it may be helpful to add some explicit documentation or comments explaining when and how this error can occur, and what steps should be taken to prevent it. This can help both individual developers who may encounter this error, as well as future readers of the code who may not be familiar with its specific behavior.

github-actions · 2023-04-25T15:11:58Z

x/derivatives/types/keys.go

+
+func ReservedCoinKeyPrefix(denom string) []byte {
+	return append([]byte(KeyPrefixReservedCoin), []byte(denom)...)
+}


The code patch is adding a new constant KeyPrefixReservedCoin and a new function ReservedCoinKeyPrefix, which takes a parameter denom and returns a byte slice. The new constant is self-explanatory, and the implementation of the new function looks fine.

As for improvement suggestions, it would be helpful to have more context on the purpose of this code and how it fits into the broader system before making any specific recommendations. Without that, it's difficult to provide meaningful feedback.

github-actions · 2023-04-25T15:12:10Z

x/derivatives/types/perpetual_futures.go

 	// return position size in the decimal unit
-	return p.Size_.MulInt64(int64(denomUnit)).TruncateInt()
+	return p.Size_.MulInt64(int64(denomExponent)).TruncateInt()
 }


The reviewed code patch seems to be a minor change to the existing codebase. It replaces the function argument name 'denomUnit' with 'denomExponent', and updates the multiplication accordingly.

Overall, the change appears to be a non-risky one in terms of introducing bugs. However, it might be worth reviewing how the 'Size_' variable is initialized and used in further parts of the codebase to ensure overall consistency with the new naming convention.

As for improvements, it might be beneficial to update the name of the function to better reflect its behavior. For example, 'SizeInDenomUnit' could be renamed to 'SizeInDenomExponent' or something more descriptive that better captures the variable being passed as an argument.

github-actions · 2023-04-25T15:12:24Z

x/derivatives/types/positions_test.go

 			if tc.exp {
 				if err != nil {
 					t.Errorf("expected %v, got %v", tc.exp, err)
 				}
 			} else {
-				if err == nil {
+				if err != nil {
 					t.Errorf("expected %v, got %v", tc.exp, err)
 				}
 			}


This code patch appears to be a unit test for a function named IsValid in the Position struct.

Improvement suggestions:

It would be helpful to include some comments or documentation describing the purpose of the function being tested.

Some of the test cases have similar names, making it hard to understand the differences between them. It would be better to have clearer and more descriptive names.

It seems like the params argument is not being used in the IsValid function, so it might be worth removing it to avoid confusion.

One test case checks for lack of margin despite there being plenty available; it would be good to have another test case that checks for a successful validation where there is enough margin available.

Bug risks:

None appear immediately visible.

jununifi

LGTM

taiki1frsh added 7 commits April 23, 2023 13:04

modify proto to add reserved property and define interfaces

e1eacd8

reserve update in opening and closing position

f008d11

validator for redeem to check available amount

acbd2b8

validation for opening position to check if reserve is enough

5b55353

rename fns

3a5327c

impl query for available asset info

f85266e

impl and fix tests for reserve coin

a9270f5

taiki1frsh requested review from kimurayu45z, mkXultra and jununifi April 25, 2023 15:05

github-actions bot reviewed Apr 25, 2023

View reviewed changes

jununifi approved these changes Apr 26, 2023

View reviewed changes

taiki1frsh merged commit cc9b839 into newDevelop Apr 26, 2023

taiki1frsh deleted the feature/reserve_tokens_to_pay_profit branch April 26, 2023 11:50

taiki1frsh mentioned this pull request Apr 28, 2023

add desc about reserve #510

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/reserve tokens to pay profit #503

Feature/reserve tokens to pay profit #503

taiki1frsh commented Apr 25, 2023

github-actions bot Apr 25, 2023

github-actions bot Apr 25, 2023

github-actions bot Apr 25, 2023

github-actions bot Apr 25, 2023

github-actions bot Apr 25, 2023

github-actions bot Apr 25, 2023

github-actions bot Apr 25, 2023

github-actions bot Apr 25, 2023

github-actions bot Apr 25, 2023

github-actions bot Apr 25, 2023

github-actions bot Apr 25, 2023

github-actions bot Apr 25, 2023

github-actions bot Apr 25, 2023

jununifi left a comment

Feature/reserve tokens to pay profit #503

Feature/reserve tokens to pay profit #503

Conversation

taiki1frsh commented Apr 25, 2023

github-actions bot Apr 25, 2023

Choose a reason for hiding this comment

github-actions bot Apr 25, 2023

Choose a reason for hiding this comment

github-actions bot Apr 25, 2023

Choose a reason for hiding this comment

github-actions bot Apr 25, 2023

Choose a reason for hiding this comment

github-actions bot Apr 25, 2023

Choose a reason for hiding this comment

github-actions bot Apr 25, 2023

Choose a reason for hiding this comment

github-actions bot Apr 25, 2023

Choose a reason for hiding this comment

github-actions bot Apr 25, 2023

Choose a reason for hiding this comment

github-actions bot Apr 25, 2023

Choose a reason for hiding this comment

github-actions bot Apr 25, 2023

Choose a reason for hiding this comment

github-actions bot Apr 25, 2023

Choose a reason for hiding this comment

github-actions bot Apr 25, 2023

Choose a reason for hiding this comment

github-actions bot Apr 25, 2023

Choose a reason for hiding this comment

jununifi left a comment

Choose a reason for hiding this comment