Feature/fix issue492 #493

taiki1frsh · 2023-04-17T04:34:40Z

close #492

Modification based on 492
Fix tests
Add account keeper to handle query against module account

cr-gpt · 2023-04-17T04:34:49Z

app/app.go

@@ -612,6 +612,7 @@ func NewApp(
 		keys[derivativestypes.StoreKey],
 		keys[derivativestypes.MemStoreKey],
 		app.GetSubspace(derivativestypes.ModuleName),
+		app.AccountKeeper,
 		app.BankKeeper,
 		app.PricefeedKeeper,
 	)


:

It looks like this code patch is adding an AccountKeeper to the NewApp function. The AccountKeeper is required to access accounts and balances in the app, so this addition appears to be necessary for the proper functioning of the application.

From a code review perspective, the code looks well-structured and easy to read. There are no obvious bugs that jump out, but it would be good to double check that the AccountKeeper is properly configured and initialized. Additionally, it would be a good idea to test that the AccountKeeper is properly integrated with the rest of the application by running tests on the application after the patch is applied.

cr-gpt · 2023-04-17T04:35:02Z

docs/client/swagger.yaml

@@ -14436,7 +14436,7 @@ definitions:
                type: string
              report_levy_period_reward_rate:
                type: string
-              accepted_assets:
+              accepted_assets_conf:
                type: array
                items:
                  type: object


with brief code review

In line 6114, the variable name accepted_assets has been changed to accepted_assets_conf. It is a good practice to give meaningful and descriptive names to variables as it helps in code readability and understanding.

In line 13171, the same variable name accepted_assets has been changed to accepted_assets_conf.

In line 13262, a new object ununifi.derivatives.PoolAssetConf has been added which has two properties denom and target_weight. This new object is used in the following lines.

In line 13304, the variable name accepted_assets has been changed to accepted_assets_conf.

In line 13313-13320, the object ununifi.derivatives.PoolParams.Asset has been removed.

In line 14436, the variable name accepted_assets has been changed to accepted_assets_conf.

Overall, the code patch is making changes to the variable names, adding a new object and removing an existing object. No bug risks have been identified in the code patch.

github-actions · 2023-04-17T04:35:04Z

app/app.go

@@ -612,6 +612,7 @@ func NewApp(
 		keys[derivativestypes.StoreKey],
 		keys[derivativestypes.MemStoreKey],
 		app.GetSubspace(derivativestypes.ModuleName),
+		app.AccountKeeper,
 		app.BankKeeper,
 		app.PricefeedKeeper,
 	)


Based on the code patch provided, it appears that a new parameter ("app.AccountKeeper") has been added to the function call of NewApp(). Assuming that this change is consistent with the overall architecture and design of the application, there do not appear to be any obvious bug risks.

As for improvement suggestions, without additional context about the purpose and requirements of the code, it is difficult to make specific recommendations. However, in general, it's important to ensure that unit tests are updated to account for any changes made to the code, and that all dependencies are properly managed and versioned. Additionally, following best practices for naming conventions, commenting, and code organization can improve the maintainability and readability of the code over time.

cr-gpt · 2023-04-17T04:35:08Z

docs/core/proto-docs.md

-| ----- | ---- | ----- | ----------- |
-| `denom` | [string](#string) |  |  |
-| `target_weight` | [string](#string) |  |  |
+| `accepted_assets_conf` | [PoolAssetConf](#ununifi.derivatives.PoolAssetConf) | repeated |  |





with the code review:

This code patch adds a new message type called PoolAssetConf that contains two fields: denom and target_weight. It also adds an accepted_assets_conf field to the PoolParams message. The field type for this is PoolAssetConf, which means that PoolParams will now be able to accept multiple PoolAssetConf elements.

The code looks valid, but it could be improved by adding additional validation checks to ensure that data entered in the denom and target_weight fields is valid. Additionally, it might be beneficial to add comments or documentation to explain what these fields are used for.

github-actions · 2023-04-17T04:35:11Z

docs/client/swagger.yaml

@@ -14436,7 +14436,7 @@ definitions:
                type: string
              report_levy_period_reward_rate:
                type: string
-              accepted_assets:
+              accepted_assets_conf:
                type: array
                items:
                  type: object


This code patch seems to be modifying a YAML file that defines the schema for several data types used in an unspecified software project.

The change is renaming the property name accepted_assets to accepted_assets_conf in several places, and introducing a new object type PoolAssetConf.

Without more context, it's difficult to assess the potential bug risk or whether the changes are improvements without having knowledge about how these data types are used by the project.

cr-gpt · 2023-04-17T04:35:16Z

proto/derivatives/derivatives.proto

-    (gogoproto.moretags) = "yaml:\"accepted_assets\""
+  repeated PoolAssetConf accepted_assets_conf = 7 [
+    (gogoproto.moretags) = "yaml:\"accepted_assets_conf\"",
+    (gogoproto.nullable) = false
  ];
 }



code review:

The code patch provided is a protocol buffer code that defines the structure of a PoolParams message. The patch makes changes to the existing message definition by renaming the Asset message to PoolAssetConf and adding a new field accepted_assets_conf. This looks like a valid change and does not appear to have any bugs or risks.

However, it would be good to consider some improvements for readability and clarity. For example, the field denom and target_weight in PoolAssetConf could be renamed to something more descriptive such as asset_denom and target_asset_weight. This would make the code more readable and easier to understand. Additionally, the accepted_assets_conf field could be given a more descriptive name such as accepted_asset_confs.

Overall, the code patch looks valid and should not pose any risks. Some minor improvements can be made to make it more readable.

github-actions · 2023-04-17T04:35:18Z

docs/core/proto-docs.md

-| ----- | ---- | ----- | ----------- |
-| `denom` | [string](#string) |  |  |
-| `target_weight` | [string](#string) |  |  |
+| `accepted_assets_conf` | [PoolAssetConf](#ununifi.derivatives.PoolAssetConf) | repeated |  |





The patch removes the PoolParams.Asset message and replaces it with a new PoolAssetConf message. The new message has denom and target_weight fields for configuring pool assets. This change removes the possibility of having additional asset attributes in the future that might be required for PoolParams.Asset.

The other changes are related to adding PoolAssetConf and replacing accepted_assets field with accepted_assets_conf.

Without seeing the rest of the code, it's difficult to assess any potential bug risks or provide improvement suggestions.

cr-gpt · 2023-04-17T04:35:21Z

x/derivatives/genesis.go

-	// todo load genesis state when restart
-	for _, asset := range genState.Params.PoolParams.AcceptedAssets {
-		k.AddPoolAsset(ctx, *asset)
-	}

 	if err := k.SetPoolMarketCapSnapshot(ctx, ctx.BlockHeight(), genState.PoolMarketCap); err != nil {
 		panic(err)


our code review:

The missing code should be added back to make sure all accepted assets are loaded when restart the system.

The code should be tested and verified before deployment to make sure it works as expected and no bug risks.

Other improvement suggestions may include adding logging to help with debugging and monitoring.

github-actions · 2023-04-17T04:35:26Z

proto/derivatives/derivatives.proto

-    (gogoproto.moretags) = "yaml:\"accepted_assets\""
+  repeated PoolAssetConf accepted_assets_conf = 7 [
+    (gogoproto.moretags) = "yaml:\"accepted_assets_conf\"",
+    (gogoproto.nullable) = false
  ];
 }



The code patch is modifying protobuf message definitions. The changes seem to be renaming the PoolParams.Asset message to PoolAssetConf, and replacing its usage in PoolParams message with PoolAssetConf. It's also adding a nullable flag to target_weight field of PoolAssetConf message.

As for bug risks and improvement suggestions, there isn't enough context provided to make any specific recommendations. However, in general, it's good practice to thoroughly test any changes to protobuf messages, to ensure that they don't break existing functionality or cause unexpected behavior. Documentation and comments should also be updated to reflect any changes.

cr-gpt · 2023-04-17T04:35:27Z

x/derivatives/keeper/grpc_query.go

@@ -309,7 +309,7 @@ func (k Keeper) DLPTokenRates(c context.Context, req *types.QueryDLPTokenRateReq
 	ctx := sdk.UnwrapSDKContext(c)
 	params := k.GetParams(ctx)
 	var rates sdk.Coins
-	for _, asset := range params.PoolParams.AcceptedAssets {
+	for _, asset := range params.PoolParams.AcceptedAssetsConf {
 		ldpDenomRate, err := k.LptDenomRate(ctx, asset.Denom)
 		if err != nil {
 			// todo error handing


the code review

The change in the code looks good and should not introduce any bugs. However, it would be better to check if the AcceptedAssetsConf is initialized in the params before using it. Also, it would be better to add error handling for when the LptDenomRate() returns an error.

In general, the code looks good, but some additional checks should be added for better safety.

cr-gpt · 2023-04-17T04:35:32Z

x/derivatives/keeper/keeper.go

@@ -41,6 +43,7 @@ func NewKeeper(
 		storeKey:        storeKey,
 		memKey:          memKey,
 		paramstore:      ps,
+		accountKeeper:   accountKeeper,
 		bankKeeper:      bankKeeper,
 		pricefeedKeeper: pricefeedKeeper,
 	}


the code review:

The patch adds a new variable to the keeper struct and initializes it in the NewKeeper function.
The code looks correct and I don't see any potential bugs. However, there may be some improvements that can be made.

For example, it might be a good idea to add a comment explaining why the new variable is needed and how it will be used. Additionally, it might be helpful to use descriptive variable names to make the code more readable.

github-actions · 2023-04-17T04:35:33Z

x/derivatives/genesis.go

-	// todo load genesis state when restart
-	for _, asset := range genState.Params.PoolParams.AcceptedAssets {
-		k.AddPoolAsset(ctx, *asset)
-	}

 	if err := k.SetPoolMarketCapSnapshot(ctx, ctx.BlockHeight(), genState.PoolMarketCap); err != nil {
 		panic(err)


The code patch seems to remove the addition of pool assets during module initialization. If it is intended, then there is no bug risk in this change.

However, if it is not intended, then missing the AddPoolAsset function can potentially result in incorrect module behavior. Thus, it is recommended to double-check whether this removal is intentional or not.

As for improvement suggestions, it would be better to include a comment explaining why the addition of pool assets is removed if it is intended. Additionally, it always helps to run unit tests after changes are made to ensure that module behavior remains consistent.

github-actions · 2023-04-17T04:35:41Z

x/derivatives/keeper/grpc_query.go

@@ -309,7 +309,7 @@ func (k Keeper) DLPTokenRates(c context.Context, req *types.QueryDLPTokenRateReq
 	ctx := sdk.UnwrapSDKContext(c)
 	params := k.GetParams(ctx)
 	var rates sdk.Coins
-	for _, asset := range params.PoolParams.AcceptedAssets {
+	for _, asset := range params.PoolParams.AcceptedAssetsConf {
 		ldpDenomRate, err := k.LptDenomRate(ctx, asset.Denom)
 		if err != nil {
 			// todo error handing


This code patch seems to be a minor change that updates a loop to use the AcceptedAssetsConf field of the PoolParams struct instead of AcceptedAssets. This suggests that there may have been a change in the format of the PoolParams.

There doesn't appear to be any immediate bug risk with this change, but it's difficult to say without seeing more of the code. It would be helpful to ensure that this change is reflected properly throughout the entire codebase.

As for an improvement suggestion, it would be good to see what the "todo error handling" comment actually entails, and to ensure that proper error handling is in place.

github-actions · 2023-04-17T04:36:27Z

x/derivatives/keeper/grpc_query.go

@@ -309,7 +309,7 @@ func (k Keeper) DLPTokenRates(c context.Context, req *types.QueryDLPTokenRateReq
 	ctx := sdk.UnwrapSDKContext(c)
 	params := k.GetParams(ctx)
 	var rates sdk.Coins
-	for _, asset := range params.PoolParams.AcceptedAssets {
+	for _, asset := range params.PoolParams.AcceptedAssetsConf {
 		ldpDenomRate, err := k.LptDenomRate(ctx, asset.Denom)
 		if err != nil {
 			// todo error handing


This code patch seems to be a minor change that updates a loop to use the AcceptedAssetsConf field of the PoolParams struct instead of AcceptedAssets. This suggests that there may have been a change in the format of the PoolParams.

There doesn't appear to be any immediate bug risk with this change, but it's difficult to say without seeing more of the code. It would be helpful to ensure that this change is reflected properly throughout the entire codebase.

As for an improvement suggestion, it would be good to see what the "todo error handling" comment actually entails, and to ensure that proper error handling is in place.

cr-gpt · 2023-04-17T04:36:27Z

x/derivatives/keeper/keeper_test.go

@@ -119,9 +119,6 @@ func (suite *KeeperTestSuite) SetupTest() {

 	keeper.SetParams(suite.ctx, params)

-	for _, asset := range params.PoolParams.AcceptedAssets {
-		keeper.AddPoolAsset(suite.ctx, *asset)
-	}
 	suite.keeper = keeper
 	suite.pricefeedKeeper = pricefeedKeeper
 }


with the code review:

On line 106, you changed the argument list for the NewKeeper function from 6 to 7. This may have unintended consequences and should be tested properly before being deployed.

On line 109, you changed the type of the AcceptedAssets variable from PoolParams_Asset to PoolAssetConf. Make sure this does not break any existing code that relies on the old type.

On lines 118-120, you commented out the loop that adds the accepted assets to the pool. Make sure the pool still functions correctly without these assets being added.

github-actions · 2023-04-17T04:36:33Z

x/derivatives/keeper/keeper.go

@@ -41,6 +43,7 @@ func NewKeeper(
 		storeKey:        storeKey,
 		memKey:          memKey,
 		paramstore:      ps,
+		accountKeeper:   accountKeeper,
 		bankKeeper:      bankKeeper,
 		pricefeedKeeper: pricefeedKeeper,
 	}


This code patch appears to be adding a new parameter called "accountKeeper" of type "types.AccountKeeper" to the NewKeeper constructor function. The accountkeeper instance variable is then initialized with the value passed in as an argument.

There are no obvious bug risks or improvement suggestions based on this code patch without further context about how the newly added "accountKeeper" parameter will be used in the codebase.

cr-gpt · 2023-04-17T04:36:35Z

x/derivatives/keeper/lpt.go

+	if !k.IsAssetAcceptable(ctx, msg.RedeemDenom) {
+		return fmt.Errorf("invalid withdraw token: %s", msg.RedeemDenom)
+	}
+
 	// todo:check validator address,amount,redeem denom
 	// todo: use CacheCtx
 	sender := msg.Sender.AccAddress()


the code review

The code looks good so far. The variable name for redeemAssetBalance is a bit strange, and it is better to use GetAssetBalanceInPoolByDenom() instead of GetAssetBalance(). The DecreaseRedeemDenomAmount() function should be renamed to SubtractRedeemDenomAmount() to make it more clear.

In InitialLiquidityProviderTokenSupply(), it is better to use GetPoolAcceptedAssetConfByDenom() instead of GetPoolAssetByDenom(). In the MintLiquidityProviderToken() function, IsValidDepositForPool() should use params.PoolParams.AcceptedAssetsConf. Finally, in the BurnLiquidityProviderToken() function, IsAssetAcceptable() should be used to check if the withdraw denom is valid.

cr-gpt · 2023-04-17T04:36:43Z

x/derivatives/keeper/lpt_test.go

 	// try after initialization
 	err = suite.keeper.DecreaseRedeemDenomAmount(suite.ctx, sdk.NewInt64Coin("uatom", 10000))
 	suite.Require().NoError(err)
-	assetBalance := suite.keeper.GetAssetBalance(suite.ctx, "uatom")
+	assetBalance := suite.keeper.GetAssetBalanceInPoolByDenom(suite.ctx, "uatom")
 	suite.Require().Equal(assetBalance.String(), "990000uatom")
 }



with the code review

Firstly, it looks like there is some redundant code present in the code patch. It appears that there are three different functions that have similar code blocks that are repeated multiple times. It would be better to reduce code duplication by refactoring the code and putting the repeated code into a separate function. This would improve code readibility and make the code easier to maintain.

Secondly, it is important to ensure that variable types are consistent when accessing and manipulating data. In the code patch, there are instances of variables being casted from one type to another, which can lead to unexpected bugs if not done correctly.

Finally, it would be a good idea to add extra logging and error handling to the code to help debugging and identify potential issues. This would also help to improve the reliability of the code.

github-actions · 2023-04-17T04:36:45Z

x/derivatives/keeper/keeper_test.go

@@ -119,9 +119,6 @@ func (suite *KeeperTestSuite) SetupTest() {

 	keeper.SetParams(suite.ctx, params)

-	for _, asset := range params.PoolParams.AcceptedAssets {
-		keeper.AddPoolAsset(suite.ctx, *asset)
-	}
 	suite.keeper = keeper
 	suite.pricefeedKeeper = pricefeedKeeper
 }


The code changes seem to modify the way pool assets are configured and managed.

In particular, a new field AcceptedAssetsConf has been introduced in the params definition and replaces the previous AcceptedAssets slice of pointers to types.PoolParams_Asset. This new field appears to be of type []types.PoolAssetConf, which might imply a change in how pool asset information is stored and manipulated.

Moreover, the parameter initialization sequence has been shortened and it now includes an additional argument suite.app.AccountKeeper when constructing a new keeper.Keeper instance. This could suggest that the account keeper component is required within the keeper.Keeper object.

Finally, the asset registration loop at lines 15-17 seems to have been removed. However, without further context it is difficult to infer if this change introduces any new bug risk or if it was done in favor of a more appropriate workflow.

cr-gpt · 2023-04-17T04:36:53Z

x/derivatives/keeper/params_test.go

@@ -16,7 +16,7 @@ func (suite *KeeperTestSuite) TestParamsGetSet() {
 		BorrowingFeeRatePerHour:     sdk.MustNewDecFromStr("0.001"),
 		ReportLiquidationRewardRate: sdk.MustNewDecFromStr("0.001"),
 		ReportLevyPeriodRewardRate:  sdk.MustNewDecFromStr("0.001"),
-		AcceptedAssets: []*types.PoolParams_Asset{
+		AcceptedAssetsConf: []types.PoolAssetConf{
 			{
 				Denom:        "uatom",
 				TargetWeight: sdk.OneDec(),


.

Code Review:

There is a potential bug risk in this code patch. The AcceptedAssets field is changed to AcceptedAssetsConf, which implies that the types of parameters have been changed. This could lead to errors if the new type is incompatible with the existing code.

The use of sdk.MustNewDecFromStr() should be avoided if possible. It is better to use sdk.NewDecFromStr() which will return an error if the string is not a valid decimal number.

The variable names should be more descriptive and consistent. In this case, the variable name BorrowingFeeRatePerHour is more descriptive than ReportLevyPeriodRewardRate.

The code can be improved by adding comments to explain the purpose of each variable. This will help other developers understand the intent of the code.

It is also recommended to use constants for values that are used multiple times, such as the "uatom" in the example. This will make the code easier to maintain and understand.

cr-gpt · 2023-04-17T04:38:10Z

x/derivatives/types/keys.go

-func AssetKeyPrefix(denom string) []byte {
-	return append([]byte(KeyPrefixDerivativesPoolAssets), []byte(denom)...)
-}
-
 func AssetDepositKeyPrefix(denom string) []byte {
 	return append([]byte(KeyPrefixPoolDeposit), []byte(denom)...)
 }


the code review.

The code looks generally fine with only a few minor changes suggested. The first change is to rename KeyPrefixDerivativesPoolAssets to KeyPrefixDerivativesSubpoolAssets as it appears to be referring to subpool assets rather than pool assets. Secondly, there are two key prefixes that appear to be unused in this code patch (KeyPrefixDerivativesUserDepositedAssets and KeyPrefixAccumulatedFee). It is suggested that these be removed if they are not necessary. Finally, the AssetKeyPrefix function appears to be unused and can be safely removed as well.

Overall, this code looks good and should not have any major bugs or risks.

github-actions · 2023-04-17T04:38:14Z

x/derivatives/simulation/genesis.go

@@ -28,7 +28,7 @@ func RandomizedGenState(simState *module.SimulationState) {
 				BaseLptRedeemFee:            sdk.NewDecWithPrec(1, 2),
 				BorrowingFeeRatePerHour:     sdk.NewDecWithPrec(1, 6),
 				ReportLiquidationRewardRate: sdk.NewDecWithPrec(1, 6),
-				AcceptedAssets: []*types.PoolParams_Asset{
+				AcceptedAssetsConf: []types.PoolAssetConf{
 					{
 						Denom:        "btc",
 						TargetWeight: sdk.NewDecWithPrec(1, 2),


The code patch looks like a modification to the function RandomizedGenState in the module SimulationState. The change replaces the field AcceptedAssets with AcceptedAssetsConf, which is defined as a slice of PoolAssetConf structs.

From the provided code excerpt, it's difficult to determine any bug risks or improvements without additional context on the purpose and functionality of the code.

cr-gpt · 2023-04-17T04:38:16Z

x/derivatives/types/params.go

@@ -24,7 +24,7 @@ func DefaultPoolParams() PoolParams {
 		BorrowingFeeRatePerHour:     sdk.ZeroDec(),
 		ReportLiquidationRewardRate: sdk.ZeroDec(),
 		ReportLevyPeriodRewardRate:  sdk.ZeroDec(),
-		AcceptedAssets:              []*PoolParams_Asset{},
+		AcceptedAssetsConf:          []PoolAssetConf{},
 	}
 }



with a brief code review

This patch changes the type of the AcceptedAssets field from PoolParams_Asset to PoolAssetConf. This replaces the struct type with a simpler configuration type, which allows for easier customization of accepted assets. This change should be safe as long as PoolAssetConf is populated with valid data and no functionality is lost by changing the type. It would be helpful to check the documentation for both types to ensure that all needed properties are present in PoolAssetConf. Additionally, it would be good to test the code to make sure that it works as expected.

cr-gpt · 2023-04-17T04:38:21Z

x/derivatives/types/pool.go

+// targetAmount = poolMarketCap * weight / price
+func CalcTargetAmountInPool(weight, price, poolMarketCap sdk.Dec) sdk.Int {
+	return poolMarketCap.Mul(weight).Quo(price).TruncateInt()
+}


with the code review
The patch looks good. The changes look reasonable and seem to improve the functionality of the code. There are no obvious bugs or risks, and the code is well formatted and easy to read.

One improvement suggestion would be to add a comment above the new CalcTargetAmountInPool() function to explain what it does and how it works. This will make it easier for other developers to understand the purpose of the function.

github-actions · 2023-04-17T04:38:22Z

x/derivatives/simulation/genesis_test.go

@@ -41,7 +41,7 @@ func TestRandomizedGenState(t *testing.T) {
 	simState.Cdc.MustUnmarshalJSON(simState.GenState[types.ModuleName], &derivativesGenesis)

 	assert.Equal(t, derivativesGenesis.Params.PoolParams.QuoteTicker, "usd")
-	assert.Len(t, derivativesGenesis.Params.PoolParams.AcceptedAssets, 2)
+	assert.Len(t, derivativesGenesis.Params.PoolParams.AcceptedAssetsConf, 2)
 	assert.Equal(t, derivativesGenesis.Params.PoolParams.BaseLptMintFee, sdk.NewDecWithPrec(1, 2))
 	assert.Equal(t, derivativesGenesis.Params.PoolParams.BaseLptRedeemFee, sdk.NewDecWithPrec(1, 2))
 	assert.Equal(t, derivativesGenesis.Params.PoolParams.BorrowingFeeRatePerHour, sdk.NewDecWithPrec(1, 6))


The code seems to be a unit test for the RandomizedGenState function. The changes made in the patch are replacing "AcceptedAssets" with "AcceptedAssetsConf" and adjusting some values accordingly.

Given that this is a unit test, it isn't clear whether there are any bug risks or improvement suggestions without seeing the implementation of the function being tested. However, assuming the implementation is correct, the changes in the patch seem to be reasonable adjustments to the test case based on changes to the actual code.

One suggestion would be to ensure that the new field name is consistent across all relevant files and documentation, to avoid confusion or errors in the future.

cr-gpt · 2023-04-17T04:38:25Z

x/derivatives/types/types.go

@@ -8,5 +8,6 @@ func (marketCap PoolMarketCap) CalculateLPTokenPrice(supply sdk.Int) sdk.Dec {
 	if supply.IsZero() {
 		return sdk.ZeroDec()
 	}
+
 	return marketCap.Total.Quo(sdk.NewDecFromInt(supply))
 }


the code review

The code looks good. There is no obvious bug risk. We can possibly improve the code by adding some additional checks and validations in order to ensure that the inputs are valid before performing the calculation. For example, we can add a check to check if the supplied supply value is greater than zero.

github-actions · 2023-04-17T04:38:29Z

x/derivatives/types/expected_keepers.go

+type AccountKeeper interface {
+	GetModuleAddress(moduleName string) sdk.AccAddress
+	GetModuleAccount(ctx sdk.Context, moduleName string) authtypes.ModuleAccountI
+}


The code patch adds an interface AccountKeeper and imports the package github.com/cosmos/cosmos-sdk/x/auth/types. It seems that this interface is intended to be used for retrieving module accounts. No bugs or risk are apparent in this patch, and it appears to be a straightforward addition of functionality. One possible improvement suggestion is to provide more information about how this interface will be used in the codebase to help with understanding its purpose better.

github-actions · 2023-04-17T04:38:45Z

x/derivatives/types/keys.go

-func AssetKeyPrefix(denom string) []byte {
-	return append([]byte(KeyPrefixDerivativesPoolAssets), []byte(denom)...)
-}
-
 func AssetDepositKeyPrefix(denom string) []byte {
 	return append([]byte(KeyPrefixPoolDeposit), []byte(denom)...)
 }


Overall, the code patch mainly adjusts the const declaration and removes unused functions. Here are some specific comments:

The code uses inconsistent naming conventions for constants. Some constants are in uppercase with underscores, while others adopt camel case.

Two constants, KeyPrefixDerivativesSubpoolAssets and KeyPrefixDerivativesUserDepositedAssets, have a TODO comment suggesting to remove them if they won't be necessary. If these prefixes are unnecessary, the code should indeed remove them to keep it simple and easy to understand.

One constant, KeyPrefixAccumulatedFee, is defined but not used in the current code. The TODO comment suggests removing it if it won't be necessary. If this constant is truly unnecessary, deleting it will make the code cleaner.

The function AssetKeyPrefix is removed from the patch, which seems fine because it is not used in the code shown. However, it might be necessary in other parts of the codebase.

Some comments are excessive and add little value to the reader's understanding. For example, the comment for KeyPrefixPoolDeposit only restates what the constant represents without providing additional context.

In general, the code patch appears to be a small change that does not introduce significant risk. However, improving the naming convention and removing unnecessary code can help maintain the code clarity and simplicity over time.

github-actions · 2023-04-17T04:38:52Z

x/derivatives/types/params.go

@@ -24,7 +24,7 @@ func DefaultPoolParams() PoolParams {
 		BorrowingFeeRatePerHour:     sdk.ZeroDec(),
 		ReportLiquidationRewardRate: sdk.ZeroDec(),
 		ReportLevyPeriodRewardRate:  sdk.ZeroDec(),
-		AcceptedAssets:              []*PoolParams_Asset{},
+		AcceptedAssetsConf:          []PoolAssetConf{},
 	}
 }



The code patch seems to have changed the structure of the DefaultPoolParams() function. The AcceptedAssets variable has been replaced with AcceptedAssetsConf, which is now a slice of PoolAssetConf struct.

Without looking at rest of the code and it's use in other components or blocks, it's hard to comment on any potential bug risks or possible improvements. However, the change appears to be intentional based on its syntax and naming conventions. It would be best to make sure that all other parts of the codebase that utilize this function are updated accordingly to avoid any inconsistencies or errors.

github-actions · 2023-04-17T04:39:02Z

x/derivatives/types/pool.go

+// targetAmount = poolMarketCap * weight / price
+func CalcTargetAmountInPool(weight, price, poolMarketCap sdk.Dec) sdk.Int {
+	return poolMarketCap.Mul(weight).Quo(price).TruncateInt()
+}


Overall, the code patch appears to be in good shape. Here are a few points to note:

The change from AcceptedAssets to AcceptedAssetsConf seems to suggest that there has been a change in how assets are represented. It would be useful to review these changes to ensure they are consistent and accurate.

In IsValidDepositForPool, the argument should be renamed to acceptableAssetConfs to reflect its new type.

There is no error handling in CalcTargetAmountInPool. It might be better to return an error if the price is zero, as division by zero is unexpected behavior.

You may consider adding more comments to fully explain how the code functions and its purpose.

Overall, the code changes seem reasonable, but it could benefit from additional tests to ensure its correctness.

github-actions · 2023-04-17T04:39:12Z

x/derivatives/types/types.go

@@ -8,5 +8,6 @@ func (marketCap PoolMarketCap) CalculateLPTokenPrice(supply sdk.Int) sdk.Dec {
 	if supply.IsZero() {
 		return sdk.ZeroDec()
 	}
+
 	return marketCap.Total.Quo(sdk.NewDecFromInt(supply))
 }


The code patch appears to be a simple function in Go programming language. The function takes a supply parameter of type sdk.Int, calculates the LP token price using the provided marketCap parameter of type PoolMarketCap, and returns it as a value of type sdk.Dec.

There are no obvious bug risks or errors in this code patch. However, some suggestions for improvement would be:

Add comments to clarify the purpose of the function and the intent behind the calculation

Ensure that the marketCap.Total value has been initialized properly prior to calling the function.

Add error handling to gracefully handle any unexpected or invalid input values.

Overall, the code patch is concise and effective but could benefit from additional documentation and code resiliency features.

cr-gpt · 2023-04-17T05:28:54Z

x/derivatives/types/keys.go

-func AssetDepositKeyPrefix(denom string) []byte {
-	return append([]byte(KeyPrefixPoolDeposit), []byte(denom)...)
-}
-
 func PositionWithIdKeyPrefix(posId string) []byte {
 	return append([]byte(KeyPrefixPosition), GetPositionIdByteFromString(posId)...)
 }


with the code review:

This patch removes some redundant code, such as KeyPrefixUserDepositedAssets, KeyPrefixPoolDeposit, and functions related to these key prefixes. It also adds a TODO comment to remind the developer to remove an unused constant if it is not needed.

Overall, this patch appears to be safe and sound, aside from the TODO comment which should be addressed accordingly. However, it is recommended to add comments to explain why the code was removed and/or why the TODO comment was added. Doing so will help future developers understand the code better.

github-actions · 2023-04-17T05:29:09Z

x/derivatives/types/keys.go

-func AssetDepositKeyPrefix(denom string) []byte {
-	return append([]byte(KeyPrefixPoolDeposit), []byte(denom)...)
-}
-
 func PositionWithIdKeyPrefix(posId string) []byte {
 	return append([]byte(KeyPrefixPosition), GetPositionIdByteFromString(posId)...)
 }


The code patch appears to remove two unused constants (KeyPrefixDerivativesSubpoolAssets and KeyPrefixAccumulatedFee) and several unused functions (AddressPoolDepositKeyPrefix(), AddressAssetPoolDepositKeyPrefix(), AssetDepositKeyPrefix()).

There are no new functions or data structures being introduced, so there is no risk of introducing new bugs. However, without seeing the rest of the codebase, it's difficult to comment on whether these changes will have any impact on other parts of the code.

As for improvement suggestions, it would be helpful to add comments explaining the purpose of the remaining constants and functions, especially if they are used in multiple places throughout the codebase.

cr-gpt · 2023-04-17T05:54:07Z

x/derivatives/spec/03_keepers.md

  GetAssetTargetAmount(ctx sdk.Context, denom string) (sdk.Coin, error)
-  GetUserDeposits(ctx sdk.Context, depositor sdk.AccAddress) []sdk.Coin
-  GetUserDenomDepositAmount(ctx sdk.Context, depositer sdk.AccAddress, denom string) sdk.Int
  GetPoolMarketCapSnapshot(ctx sdk.Context, height int64) types.PoolMarketCap
  GetPoolMarketCap(ctx sdk.Context) types.PoolMarketCap
  GetLastPositionId(ctx sdk.Context) string


the code review:

The code patch looks good, but there is a risk of bugs as it removes some methods such as GetPoolAssets and GetUserDeposits.

It would be better to check if the removed methods are being used somewhere else in the code before removing them.

The added method, GetAssetBalanceInPoolByDenom, should also be tested to make sure it works correctly.

It would also be worth checking if any other changes need to be made in order to accommodate the new method.

github-actions · 2023-04-17T05:54:21Z

x/derivatives/spec/03_keepers.md

  GetAssetTargetAmount(ctx sdk.Context, denom string) (sdk.Coin, error)
-  GetUserDeposits(ctx sdk.Context, depositor sdk.AccAddress) []sdk.Coin
-  GetUserDenomDepositAmount(ctx sdk.Context, depositer sdk.AccAddress, denom string) sdk.Int
  GetPoolMarketCapSnapshot(ctx sdk.Context, height int64) types.PoolMarketCap
  GetPoolMarketCap(ctx sdk.Context) types.PoolMarketCap
  GetLastPositionId(ctx sdk.Context) string


The code patch replaces some functions in an interface called Keeper.

It removes the following two functions: GetPoolAssets and GetUserDeposits, and their dependencies.

It also modifies the function GetAssetBalance to GetAssetBalanceInPoolByDenom with only one parameter, and removes GetUserDenomDepositAmount.

There are no obvious bug risks, but potential improvements may depend on your intended use case and overall design.

jununifi · 2023-04-17T07:36:41Z

x/derivatives/types/pool.go

+// The target amount is calculated by the formula:
+// targetAmount = poolMarketCap * weight / price
+func CalcTargetAmountInPool(weight, price, poolMarketCap sdk.Dec) sdk.Int {
+	return poolMarketCap.Mul(weight).Quo(price).TruncateInt()


I think price zero case should be considered

Yes, I def agree in general.
that's why I created this #446.

But, if we use sdk.Int or sdk.Dec for the calculation of the division, it will cause panic if the dividing num is zero, right?
So, I think this issue will be better to be addressed in a more structural way for all the other part at once.
What do you think?

jununifi · 2023-04-17T07:37:17Z

x/derivatives/types/keys.go

+	KeyPrefixPerpetualOptions         = "perpetual_options"
+	KeyPrefixNetPositionAmount        = "net_position_amount"
+	KeyPrefixLastPositionId           = "last_position_id"
+	// TODO: KeyPrefixAccumulatedFee is unused. Remove it if it won't be necesary.
 	KeyPrefixAccumulatedFee        = "accumulated_fee"


@kimurayu45z this item will need to be removed?

Sorry I forgot...

jununifi · 2023-04-17T07:38:21Z

x/derivatives/keeper/pool.go

-	store.Set(types.AssetDepositKeyPrefix(coin.Denom), bz)
+// GetAssetBalanceInPoolByDenom is used to get token balance of "derivatives" module account which is the liquidity pool.
+func (k Keeper) GetAssetBalanceInPoolByDenom(ctx sdk.Context, denom string) sdk.Coin {
+	derivModAddr := k.accountKeeper.GetModuleAddress(types.ModuleName)


Module address can be fetched without account keeper.
Same result can be fetched with authtypes.NewModuleAddress(types.ModuleName)

modified it.
Please check again.

x/derivatives/types/expected_keepers.go

jununifi

LGTM

taiki1frsh added 7 commits April 13, 2023 23:36

replace GetAssetBalance

5c37c19

remove SetAssetBalance

87f2727

remove UserDeposits functions

9931499

remove DepositPoolAsset fn

1ef2b64

clean fn

bd362bc

add accountKeeper in deriv keeper and fix tests

e9ec132

fix TestGetAssetTargetAmount

b3e4f53

taiki1frsh requested review from kimurayu45z, mkXultra and jununifi April 17, 2023 04:34