ensure fee denom is supported in escrow before creating a chainlet stack

[lukitsbrian]

No description provided.

[Copilot]

Pull Request Overview

This PR adds denom validation for escrow deposits by introducing a new GetSupportedDenoms method in the escrow keeper and enforcing validation in chainlet stack operations.

Key changes:

• Added GetSupportedDenoms method to retrieve supported denominations from escrow parameters
• Implemented denom validation in CreateChainletStack and updateChainletStackFees to ensure only supported denoms are accepted for escrow deposits
• Updated test mocks to return supported denoms ("utsaga", "utagas")

Reviewed Changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
x/escrow/keeper/keeper.go	Adds GetSupportedDenoms method to expose supported denoms from params
x/chainlet/types/expected_keepers.go	Updates EscrowKeeper interface with new GetSupportedDenoms method
x/chainlet/testutil/expected_keepers_mocks.go	Generates mock implementation for GetSupportedDenoms
x/chainlet/keeper/msg_server_create_chainlet_stack.go	Adds denom validation when creating chainlet stacks
x/chainlet/keeper/chainlet_stack.go	Adds denom validation when updating chainlet stack fees
x/chainlet/keeper/keeper_test.go	Configures mock to return supported denoms in test setup
x/chainlet/keeper/ccv_test.go	Reorders imports (cosmetic change)
x/chainlet/types/tx.pb.go	Reorganizes imports (cosmetic change)
proto/ssc/peers/data.proto	Reformats Counter message definition (cosmetic change)
proto/ssc/chainlet/tx.proto	Reformats RPC definition across multiple lines (cosmetic change)

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

@lukitsbrian I've opened a new pull request, #76, to work on those changes. Once the pull request is ready, I'll request review from you.

[ashishchandr70]

@lukitsbrian same thing with this PR. Had Cursor AI do a detailed analysis and here is the result:

Detailed Review Points

1. ✅ Error Handling Consistency

Current Implementation in CreateChainletStack:

if !ok {
    return nil, fmt.Errorf("denom %s not supported for escrow deposits, supported %v", msg.Fees.Denom, denoms)
}

Current Implementation in updateChainletStackFees:

if _, ok := supported[fee.Denom]; !ok {
    return cosmossdkerrors.Wrapf(
        types.ErrInvalidDenom,
        "denom %s not supported for escrow deposits",
        fee.Denom,
    )
}

Issue: Inconsistent error handling between the two functions.

Recommendation: Use structured error types consistently. Update CreateChainletStack to match the pattern in updateChainletStackFees:

// In CreateChainletStack
ok := false
denoms := k.escrowKeeper.GetSupportedDenoms(ctx)
for _, denom := range denoms {
    if denom == msg.Fees.Denom {
        ok = true
        break
    }
}

if !ok {
    return nil, cosmossdkerrors.Wrapf(
        types.ErrInvalidDenom,
        "denom %s not supported for escrow deposits, supported denoms: %v",
        msg.Fees.Denom,
        denoms,
    )
}

Why:

• Consistent error handling across the codebase
• Better error codes for client-side handling
• Follows Cosmos SDK error patterns
• More informative error messages

Priority: 🟡 Medium (Code quality improvement)

Note: Copilot has already opened PR #76 to address this. Consider coordinating with that PR.

---

2. ✅ Validation Timing in CreateChainletStack

Current Order:

1. ACL check
2. Denom validation
3. Stack creation

Recommendation: The current order is good - validation happens before any state changes. However, consider moving denom validation earlier, right after ValidateBasic():

func (k msgServer) CreateChainletStack(goCtx context.Context, msg *types.MsgCreateChainletStack) (*types.MsgCreateChainletStackResponse, error) {
    err := msg.ValidateBasic()
    if err != nil {
        return &types.MsgCreateChainletStackResponse{}, err
    }
    ctx := sdk.UnwrapSDKContext(goCtx)

    // Validate denom support early (before ACL check to fail fast)
    supportedDenoms := k.escrowKeeper.GetSupportedDenoms(ctx)
    denomSupported := false
    for _, denom := range supportedDenoms {
        if denom == msg.Fees.Denom {
            denomSupported = true
            break
        }
    }
    if !denomSupported {
        return nil, cosmossdkerrors.Wrapf(
            types.ErrInvalidDenom,
            "denom %s not supported for escrow deposits, supported denoms: %v",
            msg.Fees.Denom,
            supportedDenoms,
        )
    }

    // ACL check
    p := k.GetParams(ctx)
    if p.ChainletStackProtections {
        // ... rest of ACL check
    }
    
    // ... rest of function
}

Why:

• Fail fast on invalid input
• Don't waste ACL check if denom is invalid
• Better user experience (clearer error messages)

Priority: 🟢 Low (Optimization, current order is acceptable)

---

3. ✅ Performance Optimization in CreateChainletStack

Current Implementation:

ok := false
denoms := k.escrowKeeper.GetSupportedDenoms(ctx)
for _, denom := range denoms {
    if denom == msg.Fees.Denom {
        ok = true
        break
    }
}

Recommendation: Use a map for O(1) lookup, similar to updateChainletStackFees:

supported := make(map[string]struct{})
for _, denom := range k.escrowKeeper.GetSupportedDenoms(ctx) {
    supported[denom] = struct{}{}
}

if _, ok := supported[msg.Fees.Denom]; !ok {
    return nil, cosmossdkerrors.Wrapf(
        types.ErrInvalidDenom,
        "denom %s not supported for escrow deposits, supported denoms: %v",
        msg.Fees.Denom,
        k.escrowKeeper.GetSupportedDenoms(ctx),
    )
}

Why:

• More efficient for larger denom lists
• Consistent with updateChainletStackFees implementation
• O(1) lookup vs O(n) linear search

Priority: 🟢 Low (Performance optimization, current implementation is fine for small lists)

---

4. ✅ Edge Case: Empty Supported Denoms List

Current Implementation: No explicit check for empty supported denoms list.

Recommendation: Add a defensive check in GetSupportedDenoms or handle empty list in validation:

func (k Keeper) GetSupportedDenoms(ctx sdk.Context) []string {
    params := k.GetParams(ctx)
    if len(params.SupportedDenoms) == 0 {
        // Return default or log warning
        return []string{"utsaga", "utagas"} // or return empty and handle in caller
    }
    return params.SupportedDenoms
}

Or handle in the validation:

supportedDenoms := k.escrowKeeper.GetSupportedDenoms(ctx)
if len(supportedDenoms) == 0 {
    return nil, cosmossdkerrors.Wrapf(
        types.ErrInvalidDenom,
        "no supported denoms configured in escrow module",
    )
}

Why:

• Prevents confusing errors if params are misconfigured
• Better error messages for debugging

Priority: 🟡 Medium (Edge case handling)

---

5. ✅ Error Message Clarity

Current Error Message:

fmt.Errorf("denom %s not supported for escrow deposits, supported %v", msg.Fees.Denom, denoms)

Recommendation: Make error message more user-friendly:

cosmossdkerrors.Wrapf(
    types.ErrInvalidDenom,
    "denom %s is not supported for escrow deposits. Supported denoms: %v",
    msg.Fees.Denom,
    supportedDenoms,
)

Why:

• Clearer message for end users
• Consistent formatting
• Better capitalization and punctuation

Priority: 🟢 Low (Cosmetic improvement)

---

6. ✅ Validation in ValidateBasic() Consideration

Question: Should denom validation also happen in ValidateBasic()?

Current: Validation only happens in the handler.

Consideration: ValidateBasic() typically validates message structure, not external state. Since supported denoms come from escrow params (external state), it's correct to validate in the handler.

Recommendation: ✅ Current approach is correct - keep validation in handler since it requires external state.

Priority: ✅ Already correct

---

7. ✅ Test Coverage

Recommendation: Add comprehensive tests for the new validation:

File: x/chainlet/keeper/msg_server_create_chainlet_stack_test.go

func TestCreateChainletStack_UnsupportedDenom(t *testing.T) {
    // Setup: Create keeper with mock escrow keeper that returns ["utsaga", "utagas"]
    // Execute: Try to create stack with "unsupported" denom
    // Verify: Returns ErrInvalidDenom error
}

func TestCreateChainletStack_SupportedDenom(t *testing.T) {
    // Setup: Create keeper with mock escrow keeper
    // Execute: Create stack with "utsaga" denom
    // Verify: Stack created successfully
}

func TestUpdateChainletStackFees_UnsupportedDenom(t *testing.T) {
    // Setup: Create keeper with mock escrow keeper
    // Execute: Try to update fees with unsupported denom
    // Verify: Returns ErrInvalidDenom error
}

func TestUpdateChainletStackFees_MultipleDenoms(t *testing.T) {
    // Setup: Create keeper with mock escrow keeper
    // Execute: Update fees with multiple denoms (some supported, some not)
    // Verify: All denoms are validated
}

Why:

• Ensures validation works correctly
• Prevents regressions
• Documents expected behavior

Priority: 🟡 Medium (Important for correctness)

---

8. ✅ Code Documentation

Recommendation: Add godoc comments to the new method:

// GetSupportedDenoms returns the list of denominations supported by the escrow module.
// These are the denoms that can be used for escrow deposits when creating chainlet stacks.
func (k Keeper) GetSupportedDenoms(ctx sdk.Context) []string {
    params := k.GetParams(ctx)
    return params.SupportedDenoms
}

Why:

• Improves code readability
• Helps other developers understand the purpose
• Follows Go documentation conventions

Priority: 🟢 Low (Documentation improvement)

---

Comparison: CreateChainletStack vs updateChainletStackFees

Aspect	CreateChainletStack	updateChainletStackFees
Validation Method	Linear search (O(n))	Map lookup (O(1))
Error Type	fmt.Errorf	cosmossdkerrors.Wrapf with ErrInvalidDenom
Error Message	Includes supported denoms	Includes denom only
Multiple Denoms	Single denom check	Validates all fees in array

Recommendation: Make both consistent:

• Use map lookup in both
• Use structured errors in both
• Include supported denoms in error message in both

---

Security Considerations

1. ✅ Validation Before State Changes: Good - validation happens before creating/updating stacks
2. ✅ Prevents Invalid Configurations: Prevents creating stacks that can't accept deposits
3. ⚠️ Race Condition: If escrow params change between validation and stack creation, could create invalid stack. Consider if this is a concern.

Note: The race condition is unlikely in practice since params are updated infrequently, but worth noting.

---

Testing Recommendations

Unit Tests Needed

1. CreateChainletStack with unsupported denom:

   func TestCreateChainletStack_UnsupportedDenom(t *testing.T) {
       // Test that creating a stack with unsupported denom fails
   }

2. CreateChainletStack with supported denom:

   func TestCreateChainletStack_SupportedDenom(t *testing.T) {
       // Test that creating a stack with supported denom succeeds
   }

3. UpdateChainletStackFees with unsupported denom:

   func TestUpdateChainletStackFees_UnsupportedDenom(t *testing.T) {
       // Test that updating fees with unsupported denom fails
   }

4. UpdateChainletStackFees with multiple denoms:

   func TestUpdateChainletStackFees_MultipleDenoms(t *testing.T) {
       // Test that all denoms in fee array are validated
   }

5. Edge case: Empty supported denoms list:

   func TestCreateChainletStack_EmptySupportedDenoms(t *testing.T) {
       // Test behavior when no denoms are supported
   }

Integration Tests

1. End-to-end test: Create stack → Try to deposit with same denom → Should work
2. End-to-end test: Create stack with unsupported denom → Should fail early

---

Implementation Checklist

Before merging, please ensure:

• Error handling is consistent between CreateChainletStack and updateChainletStackFees
• Consider using structured errors (ErrInvalidDenom) instead of fmt.Errorf
• Consider using map lookup for better performance in CreateChainletStack
• Add tests for unsupported denom scenarios
• Add tests for supported denom scenarios
• Add tests for multiple denoms in updateChainletStackFees
• Consider edge case: empty supported denoms list
• Add godoc comments to GetSupportedDenoms
• Verify mock updates in test files
• Check that all linter errors are resolved
• Coordinate with PR #76 if addressing error handling

---

Questions for PR Owner

1. Error Handling: Should we coordinate with PR #76 to use structured errors consistently?
2. Performance: Should we optimize CreateChainletStack to use map lookup like updateChainletStackFees?
3. Edge Cases: How should we handle the case where no denoms are supported? (Should this be prevented at the params level?)
4. Validation Timing: Should denom validation happen before ACL check to fail fast?

---

Positive Aspects

1. ✅ Good Problem Identification: Addresses a real issue where unsupported denoms could cause problems
2. ✅ Proper Interface Design: Clean interface method addition
3. ✅ Efficient Implementation: Map lookup in updateChainletStackFees is efficient
4. ✅ Test Updates: Mocks are properly updated
5. ✅ Validation Placement: Validation happens at the right time (before state changes)

---

Summary

This PR adds important validation to prevent creating chainlet stacks with unsupported denoms. The implementation is solid, with the main suggestions being:

1. Consistency: Make error handling consistent between the two validation points
2. Performance: Consider using map lookup in CreateChainletStack for consistency
3. Testing: Add comprehensive tests for the new validation
4. Documentation: Add godoc comments

The core functionality is correct and addresses the problem well. The suggestions are primarily for code quality and consistency improvements.

Recommendation: Address the error handling consistency (coordinate with PR #76), add tests, and this should be ready to merge! 🚀

[ashishchandr70]

Looks good. Most of the suggested changes are cosmetic or low priority.