-
-
Notifications
You must be signed in to change notification settings - Fork 14
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
Feature/fix mint lpt #459
Feature/fix mint lpt #459
Changes from all commits
87b43fe
4acb785
bf9abae
dcb1b7f
9c77e33
f6fa1d3
5e97a16
9928943
cf05a8b
4990b1b
bf1d9a1
55457f3
6d42bf1
60fdee0
f0d45ed
06324c5
c4dbda5
7106398
64c8045
1e4c119
db87cdd
63b9bc0
ab52846
673a212
9b3c075
cd8322e
a822c82
e26de09
b1f3618
a8b0ac7
8aedff9
9f76a33
643b569
d265854
87a3fcc
113f280
9eaf448
ca1109d
6dd2d0c
6b84460
0d38260
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2006,8 +2006,8 @@ GenesisState defines the derivatives module's genesis state. | |
|
||
| Field | Type | Label | Description | | ||
| ----- | ---- | ----- | ----------- | | ||
| `amount` | [cosmos.base.v1beta1.Coin](#cosmos.base.v1beta1.Coin) | | | | ||
| `fee` | [cosmos.base.v1beta1.Coin](#cosmos.base.v1beta1.Coin) | | | | ||
| `estimated_dlp_amount` | [cosmos.base.v1beta1.Coin](#cosmos.base.v1beta1.Coin) | | | | ||
| `deposit_fee` | [cosmos.base.v1beta1.Coin](#cosmos.base.v1beta1.Coin) | | | | ||
|
||
|
||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. with a quick code review Firstly, it appears that the fields "amount" and "fee" have been changed to "estimated_dlp_amount" and "deposit_fee", respectively. This is a good change as it provides more clarity on the purpose of the fields. Also, it is important to check that the types of the fields have not changed and are still of type cosmos.base.v1beta1.Coin. It is also worth noting that the labels and descriptions of the fields have not changed, which is a good thing, as they provide useful information about the fields. Overall, this patch looks good, however, it is important to test it thoroughly to make sure there are no unexpected bugs or errors. Additionally, if possible, it might be a good idea to add more documentation about these changes for future reference. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This code patch changes two field names in the GenesisState struct of the derivatives module: As this is only a naming change, there should not be any bug risks associated with this code patch. However, it is worth noting that depending on how extensively other parts of the project rely on these field names, there may be some necessary updates to ensure compatibility. Regarding improvements, without more context on the purpose and usage of these fields, it's difficult to provide specific suggestions. Generally speaking, clear and descriptive field names are helpful for code readability and maintainability, so if these new names accurately reflect the meaning of the data being represented, they could be considered an improvement. |
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -301,12 +301,12 @@ message QueryEstimateDLPTokenAmountRequest { | |
} | ||
|
||
message QueryEstimateDLPTokenAmountResponse { | ||
cosmos.base.v1beta1.Coin amount = 1 [ | ||
cosmos.base.v1beta1.Coin estimated_dlp_amount = 1 [ | ||
(gogoproto.moretags) = "yaml:\"amount\"", | ||
(gogoproto.nullable) = false | ||
]; | ||
cosmos.base.v1beta1.Coin fee = 2 [ | ||
(gogoproto.moretags) = "yaml:\"fee\"", | ||
cosmos.base.v1beta1.Coin deposit_fee = 2 [ | ||
(gogoproto.moretags) = "yaml:\"deposit_fee\"", | ||
(gogoproto.nullable) = false | ||
]; | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Code Review:
Overall, the code patch looks good and there don't appear to be any bug risks. However, it would be helpful to add some comments to explain why the changes were made. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The code patch seems to be a protobuf definition for Regarding the changes, it looks like the field As for potential improvements, it's tough to say without more details about the use case or wider codebase. Generally speaking, it would be helpful to ensure that the variable names are clear and descriptive, and that the documentation is accurate and up-to-date. It may also be beneficial to review any relevant API or client requirements to ensure that the changes don't introduce any compatibility issues. |
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -333,23 +333,30 @@ func (k Keeper) EstimateDLPTokenAmount(c context.Context, req *types.QueryEstima | |
return nil, status.Error(codes.InvalidArgument, "invalid request") | ||
} | ||
|
||
validCoin := sdk.Coin{ | ||
deposit := sdk.Coin{ | ||
Denom: req.MintDenom, | ||
Amount: validAmount, | ||
} | ||
if validCoin.Validate() != nil { | ||
if deposit.Validate() != nil { | ||
return nil, status.Error(codes.InvalidArgument, "invalid request") | ||
} | ||
|
||
ctx := sdk.UnwrapSDKContext(c) | ||
mintAmount, mintFee, err := k.DetermineMintingLPTokenAmount(ctx, validCoin) | ||
|
||
depositFee, err := k.CalcDepositingFee(ctx, deposit, k.GetLPTokenBaseMintFee(ctx)) | ||
if err != nil { | ||
return nil, status.Error(codes.InvalidArgument, err.Error()) | ||
} | ||
|
||
depositFeeDeducted := deposit.Sub(depositFee) | ||
mintAmount, err := k.DetermineMintingLPTokenAmount(ctx, depositFeeDeducted) | ||
if err != nil { | ||
return nil, status.Error(codes.InvalidArgument, err.Error()) | ||
} | ||
|
||
return &types.QueryEstimateDLPTokenAmountResponse{ | ||
Amount: mintAmount, | ||
Fee: mintFee, | ||
EstimatedDlpAmount: mintAmount, | ||
DepositFee: depositFee, | ||
}, nil | ||
} | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This code patch removes the Other than that, there are no obvious bug risks in this code patch. However, it's always a good idea to thoroughly test any changes before deploying them to production. As for improvement suggestions, I cannot provide any without seeing the larger context of the codebase and the desired outcomes. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. with the Code review
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Based on the code patch provided, here are my observations:
Here are some suggestions for improvement:
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on the code patch you provided, it seems that a field called
fee
of typecosmos.base.v1beta1.Coin
has been removed from theGenesisState
struct.Without more context or information about how this struct is being used in your codebase, it is difficult to determine if there are any bug risks or potential improvements. However, if the removal of the
fee
field was intended and does not impact any other parts of your codebase, then this change should be safe to commit.