feat(x/protocolpool)!: allow any coins in continuous funds #21916
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -6,7 +6,6 @@ import ( | |||||||||||||
| "fmt" | ||||||||||||||
| "time" | ||||||||||||||
|
|
||||||||||||||
| "cosmossdk.io/x/protocolpool/types" | ||||||||||||||
|
|
||||||||||||||
| sdk "github.com/cosmos/cosmos-sdk/types" | ||||||||||||||
|
|
@@ -56,19 +55,19 @@ func (k Keeper) InitGenesis(ctx context.Context, data *types.GenesisState) error | |||||||||||||
| return fmt.Errorf("failed to set last balance: %w", err) | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| totalToBeDistributed := sdk.NewCoins() | ||||||||||||||
| for _, distribution := range data.Distributions { | ||||||||||||||
| totalToBeDistributed = totalToBeDistributed.Add(distribution.Amount.Amount...) | ||||||||||||||
| if err := k.Distributions.Set(ctx, *distribution.Time, distribution.Amount); err != nil { | ||||||||||||||
| return fmt.Errorf("failed to set distribution: %w", err) | ||||||||||||||
| } | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| // sanity check to avoid trying to distribute more than what is available | ||||||||||||||
|
|
||||||||||||||
| if data.LastBalance.Amount.IsAnyGT(totalToBeDistributed) || !totalToBeDistributed.DenomsSubsetOf(data.LastBalance.Amount) { | ||||||||||||||
| return errors.New("total to be distributed is greater than the last balance" + fmt.Sprint(data.LastBalance.Amount, totalToBeDistributed)) | ||||||||||||||
| } | ||||||||||||||
|
There was a problem hiding this comment. Fix the logical inconsistency in the condition and error message There's a mismatch between the condition being checked and the error message returned. The condition To correct this, the condition should check if Apply this diff to fix the condition: -if data.LastBalance.Amount.IsAnyGT(totalToBeDistributed) || !totalToBeDistributed.DenomsSubsetOf(data.LastBalance.Amount) {
+if totalToBeDistributed.IsAnyGT(data.LastBalance.Amount) || !totalToBeDistributed.DenomsSubsetOf(data.LastBalance.Amount) {
return errors.New("total to be distributed is greater than the last balance" + fmt.Sprint(data.LastBalance.Amount, totalToBeDistributed))
}This change ensures that the error is returned when the total to be distributed exceeds the last balance, aligning the condition with the error message. 📝 Committable suggestion
Suggested change
There was a problem hiding this comment. I'm confused a bit about the 1st condition, should return an error when There was a problem hiding this comment. @hieuvubk, you're correct. The error should be returned when if data.LastBalance.Amount.IsAnyGT(totalToBeDistributed) || !totalToBeDistributed.DenomsSubsetOf(data.LastBalance.Amount) {
return errors.New("total to be distributed is greater than the last balance" + fmt.Sprint(data.LastBalance.Amount, totalToBeDistributed))
}checks if To fix this inconsistency, the condition should be: if totalToBeDistributed.IsAnyGT(data.LastBalance.Amount) || !totalToBeDistributed.DenomsSubsetOf(data.LastBalance.Amount) {
return errors.New("total to be distributed is greater than the last balance" + fmt.Sprint(data.LastBalance.Amount, totalToBeDistributed))
}This change ensures that an error is returned when /_/\
There was a problem hiding this comment. Why not simply go with fmt.Errorf("an amount to be distributed is greater than the last balance: %s > %s", totalToBeDistributed, data.LastBalance.Amount))There was a problem hiding this comment.
There was a problem hiding this comment. This was actually for testing, but I think it can be useful if we ever encounter this, I'll modify it. |
||||||||||||||
| return nil | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
|
|
@@ -112,12 +111,14 @@ func (k Keeper) ExportGenesis(ctx context.Context) (*types.GenesisState, error) | |||||||||||||
|
|
||||||||||||||
| genState := types.NewGenesisState(cf, budget) | ||||||||||||||
|
|
||||||||||||||
| lastBalance, err := k.LastBalance.Get(ctx) | ||||||||||||||
| if err != nil { | ||||||||||||||
|
Comment on lines
+119
to
120
There was a problem hiding this comment. 🛠️ Refactor suggestion Wrap error when retrieving last balance for better context When returning errors, it's recommended to wrap them with additional context to aid in debugging and provide clarity. Apply this diff to wrap the error: if err != nil {
- return nil, err
+ return nil, fmt.Errorf("failed to get last balance: %w", err)
}
|
||||||||||||||
| return nil, err | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| genState.LastBalance = lastBalance | ||||||||||||||
|
|
||||||||||||||
| err = k.Distributions.Walk(ctx, nil, func(key time.Time, value types.DistributionAmount) (stop bool, err error) { | ||||||||||||||
| genState.Distributions = append(genState.Distributions, &types.Distribution{ | ||||||||||||||
| Time: &key, | ||||||||||||||
|
Comment on lines
+126
to
128
There was a problem hiding this comment. 🛠️ Refactor suggestion Wrap error when walking through distributions for enhanced clarity Wrapping the error from Apply this diff to wrap the error: if err != nil {
- return nil, err
+ return nil, fmt.Errorf("failed to walk through distributions: %w", err)
}
|
||||||||||||||
| Amount: value, | ||||||||||||||
|
|
||||||||||||||
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.
last balance should always be >= total to be distributed.
Simplest thing would be to flip the check:
totalToBeDistributed.IsAnyGT(last balance)This includes the denoms implicite so that you can drop the
DenomsSubsetOfcheckThere 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.
You are right about the inverted comparison, but I still need
DenomsSubsetOf, see:This case
{}.IsAnyGT{5C} = falseshows that if totalToBeDistributed has 1foo but LastBalance contains1bar, it will be false.