Feature/fix pool asset #491
Conversation
![cr-gpt[bot]](https://avatars.githubusercontent.com/in/292659?s=60&v=4){kind=small}
| @@ -14436,7 +14436,7 @@ definitions: | |||
| type: string | |||
| report_levy_period_reward_rate: | |||
| type: string | |||
| accepted_assets: | |||
| accepted_assets_conf: | |||
| type: array | |||
| items: | |||
| type: object | |||
There was a problem hiding this comment.
with basic code review
- The patch is to rename the accepted_assets as accepted_assets_conf, it will avoid confusion and make the code more clear.
- We can also check if the type of accepted_assets_conf is correct, it should be an array of objects.
- We can also check if all the references to accepted_assets are correctly updated to accepted_assets_conf.
- We can also check if there is any logic changes in the patch, such as the logic related to accepted_assets should be same with accepted_assets_conf.
- Finally, we can also check if there are any other potential bugs or improvement suggestion.
| | ----- | ---- | ----- | ----------- | | ||
| | `denom` | [string](#string) | | | | ||
| | `target_weight` | [string](#string) | | | | ||
| | `accepted_assets_conf` | [PoolAssetConf](#ununifi.derivatives.PoolAssetConf) | repeated | | | ||
|
|
||
|
|
||
|
|
There was a problem hiding this comment.
.
Code Review:
The patch adds a new type, PoolAssetConf, to keep track of the accepted assets in a PoolParams. It also adds a field to PoolParams, accepted_assets_conf, to store the PoolAssetConf values. This should provide better clarity and flexibility when managing the accepted assets in the PoolParams.
However, there is a risk that the new fields will cause incompatibility with existing code and introduce bugs. To minimize this risk, it is important to ensure that all existing code is thoroughly tested with the new fields. Additionally, it would be useful to add documentation for the new fields to make it easier for developers to understand how they should be used.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| @@ -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 | |||
There was a problem hiding this comment.
the code review
This patch looks like it is meant to change the accepted assets from params.PoolParams.AcceptedAssets to params.PoolParams.AcceptedAssetsConf. The code looks mostly fine, but there are a couple of potential issues.
First, it is possible that params.PoolParams.AcceptedAssetsConf could be empty or contain invalid entries. This could lead to an unexpected behavior or errors if not handled properly. It would be good to add some kind of validation check here to ensure that the list contains valid entries.
Second, it would be good to add some logging statement to track any errors that may occur. This could help with debugging any potential issues in the future.
Finally, it would be a good idea to add some unit tests to ensure that the code behaves as expected. This could help prevent any bugs from slipping through in the future.
Overall, this looks like a solid patch. With a few minor changes, it should be ready for deployment.
|
@taiki1frsh |
Feature/fix issue492
close #386
The changes lies in the various part.
What I did simply: