fix(provider): move MsgChangeRewardDenoms validation to msg server handler

[Aboudjem]

Closes #2149

Moves the denom validation logic from MsgChangeRewardDenoms.ValidateBasic() into the ChangeRewardDenoms msg server handler, following the cosmos-sdk v0.50 pattern where ValidateBasic is soft-deprecated.

The validation checks (empty denoms, invalid denom format, denom in both add and remove sets) are now performed in the handler. ValidateBasic stubs to nil. Also swapped sdk.NewCoin().IsValid() for sdk.ValidateDenom() to avoid a panic on malformed input.

Tests added to msg_server_test.go covering the moved validation paths.

[greptile-apps]

Greptile Summary

This PR moves the MsgChangeRewardDenoms validation logic (empty-set check, denom format validation, add/remove overlap check) from ValidateBasic() into the ChangeRewardDenoms msg server handler, following the cosmos-sdk v0.50 soft-deprecation of ValidateBasic. It also replaces the panic-prone sdk.NewCoin().IsValid() call with the safer sdk.ValidateDenom(). New unit tests cover the moved validation paths.

Confidence Score: 5/5

Safe to merge — the refactoring is correct, the panic fix is an improvement, and all remaining findings are non-blocking style/coverage suggestions.

All P0/P1 behaviour is preserved: authority check still runs first, all three validation rules are enforced in the handler, and sdk.ValidateDenom is a strict superset of the old IsValid() denom check. The two findings (missing t.Run and a missing DenomsToRemove test case) are P2 quality-of-life improvements that do not affect correctness.

msg_server_test.go — minor test-isolation and coverage gaps, no correctness impact.

Vulnerabilities

No security concerns identified. The authority check remains the first guard in the handler, and sdk.ValidateDenom safely returns an error rather than panicking on malformed input.

Important Files Changed

Filename	Overview
x/ccv/provider/keeper/msg_server.go	Validation logic (empty check, denom format via sdk.ValidateDenom, overlap check) correctly moved into handler before the keeper call; order with the authority check is preserved.
x/ccv/provider/types/msg.go	ValidateBasic stubbed to nil per cosmos-sdk v0.50 pattern; sdk.HasValidateBasic interface assertion retained; no import issues.
x/ccv/provider/keeper/msg_server_test.go	New TestChangeRewardDenomsValidation covers 4 paths but lacks t.Run isolation and a test for invalid denom in DenomsToRemove.
CHANGELOG.md	Entry correctly added under Unreleased > IMPROVEMENTS with issue reference.

Sequence Diagram

sequenceDiagram
    participant Client
    participant MsgServer as ChangeRewardDenoms Handler
    participant Keeper

    Client->>MsgServer: MsgChangeRewardDenoms
    MsgServer->>MsgServer: Check authority
    alt authority mismatch
        MsgServer-->>Client: ErrUnauthorized
    end
    MsgServer->>MsgServer: Check DenomsToAdd and DenomsToRemove not both empty
    alt both empty
        MsgServer-->>Client: ErrInvalidMsgChangeRewardDenoms
    end
    MsgServer->>MsgServer: sdk.ValidateDenom each denom in DenomsToAdd
    alt invalid denom
        MsgServer-->>Client: ErrInvalidMsgChangeRewardDenoms
    end
    MsgServer->>MsgServer: sdk.ValidateDenom each denom in DenomsToRemove + overlap check
    alt invalid denom or overlap
        MsgServer-->>Client: ErrInvalidMsgChangeRewardDenoms
    end
    MsgServer->>Keeper: ChangeRewardDenoms(ctx, add, remove)
    Keeper-->>MsgServer: eventAttributes
    MsgServer-->>Client: MsgChangeRewardDenomsResponse

Loading

_{Reviews (1): Last reviewed commit: "fix(provider): move MsgChangeRewardDenom..." | Re-trigger Greptile}

[greptile-apps]

Test cases lack subtest isolation

Without t.Run, a require failure in any earlier test case halts the entire function, meaning later cases (including the success path) are silently skipped. This also means the -run flag and test output can't identify individual cases. Consider wrapping each case:

Suggested change

	for _, tc := range testCases {
	_, err := msgServer.ChangeRewardDenoms(ctx, tc.msg)
	if tc.expectError {
	require.Error(t, err, tc.name)
	} else {
	require.NoError(t, err, tc.name)
	}
	}
	for _, tc := range testCases {
	t.Run(tc.name, func(t *testing.T) {
	_, err := msgServer.ChangeRewardDenoms(ctx, tc.msg)
	if tc.expectError {
	require.Error(t, err)
	} else {
	require.NoError(t, err)
	}
	})
	}

[greptile-apps]

Missing test case for invalid denom in DenomsToRemove

The "invalid denom" case only exercises the DenomsToAdd path. The symmetric DenomsToRemove validation branch (lines 123–126 of msg_server.go) has no dedicated test, leaving a gap in coverage. Consider adding:

{
    name: "invalid denom in remove",
    msg: &providertypes.MsgChangeRewardDenoms{
        Authority:      authority,
        DenomsToRemove: []string{"!!!bad"},
    },
    expectError: true,
},

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!