Feature/fix mint lpt #459
Feature/fix mint lpt #459
Conversation
|
I think this conflicts with |
|
Seems you are using me but didn't get OPENAI_API_KEY seted in Variables for this repo. you could follow readme for more information |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| @@ -2005,7 +2005,6 @@ 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) | | | | |||
|
|
|||
|
|
|||
|
|
|||
There was a problem hiding this comment.
Based on the code patch you provided, it seems that a field called fee of type cosmos.base.v1beta1.Coin has been removed from the GenesisState 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.
| if err != nil { | ||
| return nil, status.Error(codes.InvalidArgument, err.Error()) | ||
| } | ||
|
|
||
| return &types.QueryEstimateDLPTokenAmountResponse{ | ||
| Amount: mintAmount, | ||
| Fee: mintFee, | ||
| }, nil | ||
| } | ||
|
|
There was a problem hiding this comment.
This code patch removes the mintFee variable and its assignment from the EstimateDLPTokenAmount() function. It means that the function does not return the estimated minting fees anymore. Make sure that this change is intended and will not break any other parts of the system that rely on the mintFee value.
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.
|
resolved conflicts regarding lpt functions. |
add complete validation for position
add max leverage params
|
Seems you are using me but didn't get OPENAI_API_KEY seted in Variables for this repo. you could follow readme for more information |
|
Seems you are using me but didn't get OPENAI_API_KEY seted in Variables for this repo. you could follow readme for more information |
chore: derivatives rename
add max leverage params
add max leverage params
add max leverage params
add max leverage params
add max leverage params
![cr-gpt[bot]](https://avatars.githubusercontent.com/in/292659?s=60&v=4){kind=small}
| | `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.
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.
| 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.
Code Review:
-
The code patch renames the field
amounttoestimated_dlp_amountand the fieldfeetodeposit_fee, which makes the code more descriptive and easier to understand. -
In the code patch, the
gogoproto.nullableis set to false, which implies that the fields are required and cannot be set to null. -
The code patch also adds a
yamltag to the fields, which may be useful when mapping the fields to YAML configuration files.
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.
| Amount: mintAmount, | ||
| Fee: mintFee, | ||
| EstimatedDlpAmount: mintAmount, | ||
| DepositFee: depositFee, | ||
| }, nil | ||
| } | ||
|
|
There was a problem hiding this comment.
with the Code review
- In line 8, the variable "deposit" should be used instead of "validCoin" to make the code more readable and understandable.
- Starting on line 11, a new variable should be created to store the deposit fee which can then be used in the next few lines instead of using the deposit amount directly.
- Line 14 should use the new depositFee variable instead of calling the CalcDepositingFee method again.
- Line 17 should use the new depositFeeDeducted variable instead of the deposit variable.
- Line 19 should use the EstimatedDlpAmount instead of Amount to make the code more readable.
- The Fee field should also be updated to DepositFee for consistency.
| | `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.
This code patch changes two field names in the GenesisState struct of the derivatives module: amount is renamed to estimated_dlp_amount and fee is renamed to deposit_fee. The types and labels of both fields remain unchanged.
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.
| 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.
The code patch seems to be a protobuf definition for QueryEstimateDLPTokenAmountRequest and QueryEstimateDLPTokenAmountResponse.
Regarding the changes, it looks like the field amount in the QueryEstimateDLPTokenAmountResponse message has been renamed to estimated_dlp_amount, and a new field deposit_fee has been added. This change seems reasonable based on the context.
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.
| Amount: mintAmount, | ||
| Fee: mintFee, | ||
| EstimatedDlpAmount: mintAmount, | ||
| DepositFee: depositFee, | ||
| }, nil | ||
| } | ||
|
|
There was a problem hiding this comment.
Based on the code patch provided, here are my observations:
- This code block is an implementation of the EstimateDLPTokenAmount function in Go Programming Language.
- The function takes a context c and a request type QueryEstimateDLPTokenAmountRequest as input parameters and returns an estimate of the DLP Token amount along with a deposit fee.
- It starts by validating the inputs to ensure that they are valid.
- A deposit variable is created, which is an instance of the sdk.Coin struct, and it contains the values passed in the request.
- A deposit fee is then calculated using the CalcDepositingFee method.
- After calculating the deposit fee, it is deducted from the deposit, and the DetermineMintingLPTokenAmount method is called to calculate the minting amount of LPTokens.
- Finally, the function returns the estimated minting amount of DLP tokens along with the deposit fee.
Here are some suggestions for improvement:
- Ensure that the function handles errors correctly and provides appropriate error messages to the user.
- Check if the fees being charged are reasonable, to avoid overcharging or undercharging users.
- Consider adding unit tests or integration tests to ensure that the code behaves as expected in different scenarios.
Issue: #456 , #392
The purpose of this PR is mostly on #456.
Please refer there.