Skip to content

fix(rewards): skip ReleasedAmount denom check when amount is zero #355

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.

Sign up for GitHub

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

Open
amathxbt wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix(rewards): skip ReleasedAmount denom check when amount is zero #355

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
185 changes: 89 additions & 96 deletions x/rewards/keeper/validation.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,127 +1,120 @@
package keeper

import (
"context"
"fmt"
"time"
\t"context"

Check failure on line 4 in x/rewards/keeper/validation.go

View workflow job for this annotation

GitHub Actions / test-e2e 

illegal character U+005C '\'

Check failure on line 4 in x/rewards/keeper/validation.go

View workflow job for this annotation

GitHub Actions / golangci-lint 

illegal character U+005C '\' (typecheck)

Check failure on line 4 in x/rewards/keeper/validation.go

View workflow job for this annotation

GitHub Actions / liveness-test 

illegal character U+005C '\'

Check failure on line 4 in x/rewards/keeper/validation.go

View workflow job for this annotation

GitHub Actions / tests 

illegal character U+005C '\'

Check failure on line 4 in x/rewards/keeper/validation.go

View workflow job for this annotation

GitHub Actions / tests 

illegal character U+005C '\'

Check failure on line 4 in x/rewards/keeper/validation.go

View workflow job for this annotation

GitHub Actions / tests 

illegal character U+005C '\'

Check failure on line 4 in x/rewards/keeper/validation.go

View workflow job for this annotation

GitHub Actions / tests 

illegal character U+005C '\'

Check failure on line 4 in x/rewards/keeper/validation.go

View workflow job for this annotation

GitHub Actions / tests 

illegal character U+005C '\'

Check failure on line 4 in x/rewards/keeper/validation.go

View workflow job for this annotation

GitHub Actions / tests 

illegal character U+005C '\'
\t"fmt"

Check failure on line 5 in x/rewards/keeper/validation.go

View workflow job for this annotation

GitHub Actions / golangci-lint 

illegal character U+005C '\' (typecheck)
\t"time"

Check failure on line 6 in x/rewards/keeper/validation.go

View workflow job for this annotation

GitHub Actions / golangci-lint 

illegal character U+005C '\' (typecheck)

"cosmossdk.io/errors"
\t"cosmossdk.io/errors"

Check failure on line 8 in x/rewards/keeper/validation.go

View workflow job for this annotation

GitHub Actions / golangci-lint 

illegal character U+005C '\' (typecheck)

sdk "github.com/cosmos/cosmos-sdk/types"
sdkerrors "github.com/cosmos/cosmos-sdk/types/errors"
govtypes "github.com/cosmos/cosmos-sdk/x/gov/types"
\tsdk "github.com/cosmos/cosmos-sdk/types"

Check failure on line 10 in x/rewards/keeper/validation.go

View workflow job for this annotation

GitHub Actions / golangci-lint 

illegal character U+005C '\' (typecheck)
\tsdkerrors "github.com/cosmos/cosmos-sdk/types/errors"

Check failure on line 11 in x/rewards/keeper/validation.go

View workflow job for this annotation

GitHub Actions / golangci-lint 

illegal character U+005C '\' (typecheck)
\tgovtypes "github.com/cosmos/cosmos-sdk/x/gov/types"

Check failure on line 12 in x/rewards/keeper/validation.go

View workflow job for this annotation

GitHub Actions / golangci-lint 

illegal character U+005C '\' (typecheck)

"github.com/kiichain/kiichain/v7/x/rewards/types"
\t"github.com/kiichain/kiichain/v7/x/rewards/types"

Check failure on line 14 in x/rewards/keeper/validation.go

View workflow job for this annotation

GitHub Actions / golangci-lint 

illegal character U+005C '\' (typecheck)
)

// validateAuthority checks if address authority is valid and same as expected
func (k *Keeper) validateAuthority(authority string) error {
if _, err := sdk.AccAddressFromBech32(authority); err != nil {
return sdkerrors.ErrInvalidAddress.Wrapf("invalid authority address: %s", err)
}
\tif _, err := sdk.AccAddressFromBech32(authority); err != nil {

Check failure on line 19 in x/rewards/keeper/validation.go

View workflow job for this annotation

GitHub Actions / golangci-lint 

illegal character U+005C '\' (typecheck)
\t return sdkerrors.ErrInvalidAddress.Wrapf("invalid authority address: %s", err)

Check failure on line 20 in x/rewards/keeper/validation.go

View workflow job for this annotation

GitHub Actions / golangci-lint 

illegal character U+005C '\' (typecheck)
\t}

if k.authority != authority {
return errors.Wrapf(govtypes.ErrInvalidSigner, "invalid authority; expected %s, got %s", k.authority, authority)
}
\tif k.authority != authority {
\t return errors.Wrapf(govtypes.ErrInvalidSigner, "invalid authority; expected %s, got %s", k.authority, authority)
\t}

return nil
\treturn nil
}

// validateAmount check if amount is a valid coin
func validateAmount(amount sdk.Coin) error {
if err := amount.Validate(); err != nil {
return errors.Wrap(sdkerrors.ErrInvalidCoins, amount.String())
}
\tif err := amount.Validate(); err != nil {
\t return errors.Wrap(sdkerrors.ErrInvalidCoins, amount.String())
\t}

return nil
\treturn nil
}

// validateEndTime checks if time is in the past
func validateEndTime(ctx sdk.Context, endTime time.Time) error {
if endTime.Before(ctx.BlockTime()) {
return fmt.Errorf("end time %s is not in the future", endTime)
}
\tif endTime.Before(ctx.BlockTime()) {
\t return fmt.Errorf("end time %s is not in the future", endTime)
\t}

return nil
\treturn nil
}

// fundsAvailable checks if the asked funds are available in the pool
func (k Keeper) fundsAvailable(ctx context.Context, amount sdk.Coin) error {
// Get reward pool
rewardPool, err := k.RewardPool.Get(ctx)
if err != nil {
return err
}

// Check if it is trying to use more funds than available
poolAmount := rewardPool.CommunityPool.AmountOf(amount.Denom)
if sdk.NewDecCoinFromCoin(amount).Amount.GT(poolAmount) {
return fmt.Errorf("reward pool (%s) has less funds than requested (%s)", poolAmount, amount)
}

return nil
\trewardPool, err := k.RewardPool.Get(ctx)
\tif err != nil {
\t return err
\t}
\tpoolAmount := rewardPool.CommunityPool.AmountOf(amount.Denom)
\tif sdk.NewDecCoinFromCoin(amount).Amount.GT(poolAmount) {
\t return fmt.Errorf("reward pool (%s) has less funds than requested (%s)", poolAmount, amount)
\t}
\treturn nil
}

// validateSchedule checks if the asked funds are available in the pool
func (k Keeper) validateSchedule(ctx sdk.Context, schedule types.ReleaseSchedule) error {
// Validate TotalAmount
if err := validateAmount(schedule.TotalAmount); err != nil {
return fmt.Errorf("invalid total amount: %w", err)
}

// Validate against module params
params, err := k.Params.Get(ctx)
if err != nil {
return fmt.Errorf("failed to get module params: %w", err)
}
if params.TokenDenom != schedule.TotalAmount.Denom {
return fmt.Errorf("denom %s does not match expected denom: %s",
schedule.TotalAmount.Denom, params.TokenDenom)
}

// Validate ReleasedAmount
if schedule.ReleasedAmount.Denom != schedule.TotalAmount.Denom {
return fmt.Errorf("released amount denom %s doesn't match total amount denom %s",
schedule.ReleasedAmount.Denom, schedule.TotalAmount.Denom)
}
if !schedule.ReleasedAmount.IsZero() {
if err := validateAmount(schedule.ReleasedAmount); err != nil {
return fmt.Errorf("invalid released amount: %w", err)
}
if schedule.ReleasedAmount.Amount.GT(schedule.TotalAmount.Amount) {
return fmt.Errorf("released amount %s cannot exceed total amount %s",
schedule.ReleasedAmount, schedule.TotalAmount)
}
}

// Time validations
if schedule.EndTime.IsZero() {
return fmt.Errorf("end time cannot be zero")
}
if err = validateEndTime(ctx, schedule.EndTime); err != nil {
return err
}

currentTime := ctx.BlockTime()
if !schedule.LastReleaseTime.IsZero() {
if schedule.LastReleaseTime.After(currentTime) {
return fmt.Errorf("last release time %s cannot be in the future",
schedule.LastReleaseTime)
}
if schedule.LastReleaseTime.After(schedule.EndTime) {
return fmt.Errorf("last release time %s cannot be after end time %s",
schedule.LastReleaseTime, schedule.EndTime)
}
}

// 6. Active state consistency
if schedule.Active {
if schedule.TotalAmount.IsZero() {
return fmt.Errorf("active schedule cannot have zero total amount")
}
if schedule.EndTime.IsZero() {
return fmt.Errorf("active schedule must have an end time")
}
}
return nil
\tif err := validateAmount(schedule.TotalAmount); err != nil {
\t return fmt.Errorf("invalid total amount: %w", err)
\t}

\tparams, err := k.Params.Get(ctx)
\tif err != nil {
\t return fmt.Errorf("failed to get module params: %w", err)
\t}
\tif params.TokenDenom != schedule.TotalAmount.Denom {
\t return fmt.Errorf("denom %s does not match expected denom: %s",
\t schedule.TotalAmount.Denom, params.TokenDenom)
\t}

\t// Validate ReleasedAmount only when non-zero. The denom check was previously
\t// run unconditionally, which rejected zero-value coins (sdk.Coin{} with empty
\t// denom) used to represent a fresh schedule with no released tokens yet.
\tif !schedule.ReleasedAmount.IsZero() {
\t if schedule.ReleasedAmount.Denom != schedule.TotalAmount.Denom {
\t return fmt.Errorf("released amount denom %s doesn't match total amount denom %s",
\t schedule.ReleasedAmount.Denom, schedule.TotalAmount.Denom)
\t }
\t if err := validateAmount(schedule.ReleasedAmount); err != nil {
\t return fmt.Errorf("invalid released amount: %w", err)
\t }
\t if schedule.ReleasedAmount.Amount.GT(schedule.TotalAmount.Amount) {
\t return fmt.Errorf("released amount %s cannot exceed total amount %s",
\t schedule.ReleasedAmount, schedule.TotalAmount)
\t }
\t}
Comment on lines +76 to +91

@coderabbitai coderabbitai Bot Jul 6, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
rg -n "denom mismatch" -A 10 x/rewards/keeper/msg_server_test.go

Repository: KiiChain/kiichain

Length of output: 482

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo '--- validation.go ---'
cat -n x/rewards/keeper/validation.go | sed -n '1,180p'

echo
echo '--- release_schedule.go ---'
cat -n x/rewards/types/release_schedule.go | sed -n '1,220p'

echo
echo '--- msg_server_test.go relevant section ---'
cat -n x/rewards/keeper/msg_server_test.go | sed -n '180,230p'

Repository: KiiChain/kiichain

Length of output: 8631

🏁 Script executed:

#!/bin/bash
set -euo pipefail

rg -n "ReleasedAmount|ReleasedAmount\." x/rewards -g'*.go'

Repository: KiiChain/kiichain

Length of output: 6967

🏁 Script executed:

#!/bin/bash
set -euo pipefail

rg -n "ReleasedAmount|ReleasedAmount\\." x/rewards -g'*.go'

Repository: KiiChain/kiichain

Length of output: 6967

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo '--- x/rewards/types/release_schedule_test.go ---'
cat -n x/rewards/types/release_schedule_test.go | sed -n '1,140p'

echo
echo '--- x/rewards/types/genesis_test.go ---'
cat -n x/rewards/types/genesis_test.go | sed -n '1,160p'

echo
echo '--- x/rewards/types/reward.go ---'
cat -n x/rewards/types/reward.go | sed -n '1,120p'

Repository: KiiChain/kiichain

Length of output: 10885

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo '--- x/rewards/keeper/abci.go ---'
cat -n x/rewards/keeper/abci.go | sed -n '1,160p'

echo
echo '--- sdk coin subtraction semantics references ---'
rg -n "func \\(c Coin\\) Sub|Sub\\(" "$(go env GOPATH 2>/dev/null)/pkg/mod" -g'coin.go' -g'*.go' 2>/dev/null | head -n 40

Repository: KiiChain/kiichain

Length of output: 5723

Zero-amount released coins should still reject mismatched denoms.

ReleasedAmount.IsZero() only checks Amount, so sdk.NewCoin("otherdenom", 0) now skips the denom check and makes the msg_server_test.go “denom mismatch” case pass unexpectedly. If zero released amounts are only meant to allow the canonical empty coin, keep validating Denom when it is set; otherwise update the test and callers to the new rule.

🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@x/rewards/keeper/validation.go` around lines 76 - 91, The released-amount
validation in validateSchedule currently skips denom checks whenever
ReleasedAmount.IsZero(), which lets zero-amount coins with a non-empty,
mismatched denom slip through. Update the logic in validateSchedule so
zero-amount released coins still validate the denom unless it is the canonical
empty coin, and keep the amount/limit checks guarded appropriately. Use
ReleasedAmount, TotalAmount, and validateAmount as the key symbols when
adjusting the condition, and align msg_server_test.go expectations if the rule
changes.


\tif schedule.EndTime.IsZero() {
\t return fmt.Errorf("end time cannot be zero")
\t}
\tif err = validateEndTime(ctx, schedule.EndTime); err != nil {
\t return err
\t}

\tcurrentTime := ctx.BlockTime()
\tif !schedule.LastReleaseTime.IsZero() {
\t if schedule.LastReleaseTime.After(currentTime) {
\t return fmt.Errorf("last release time %s cannot be in the future", schedule.LastReleaseTime)
\t }
\t if schedule.LastReleaseTime.After(schedule.EndTime) {
\t return fmt.Errorf("last release time %s cannot be after end time %s",
\t schedule.LastReleaseTime, schedule.EndTime)
\t }
\t}

\tif schedule.Active {
\t if schedule.TotalAmount.IsZero() {
\t return fmt.Errorf("active schedule cannot have zero total amount")
\t }
\t if schedule.EndTime.IsZero() {
\t return fmt.Errorf("active schedule must have an end time")
\t }
\t}
\treturn nil
}
Loading