TODO: Add tests for osmosis and umee zones.

Would you like me to help implement these tests or should I open a GitHub issue to track this task?

The test does not validate the cleanup of protocol data after removal. The second instance of GetProtocolData still checks for the presence of the protocol data instead of ensuring it has been removed.

• Ensure that the test verifies the absence of protocol data after the removal proposal.

#!/bin/bash
# Description: Verify that the protocol data is properly cleaned up after removal.

# Test: Search for the function usage. Expect: No remaining protocol data after removal.
rg --type go $'GetProtocolData' x/participationrewards/keeper/proposal_handler_test.go

#!/bin/bash
# Extract lines around the second instance of GetProtocolData to verify cleanup validation.
rg --type go -A 5 -B 5 $'GetProtocolData' x/participationrewards/keeper/proposal_handler_test.go

I know this isn’t code you have changed but I think rabbit might be right here! Pd.generateKey() only generates the pd specific part of the key - I’m not sure if this is prefixed by the type in the delete function or we need to use the function in types to prefix this with the type

@joe-bowman, it appears that the GovRemoveProtocolData function is indeed responsible for removing protocol data, but the exact implementation details are not fully clear from the automated searches. Based on the context, it seems that pd.GenerateKey() only generates the protocol data specific part of the key, and it might need to be prefixed with the type.

To ensure proper cleanup and validation, we should verify that the key used in the GovRemoveProtocolData function includes the type prefix. Here is a potential update to the test to ensure the protocol data is properly removed:

func (suite *KeeperTestSuite) TestHandleRemoveProtocolDataProposal() {
    appA := suite.GetQuicksilverApp(suite.chainA)
    ctx := suite.chainA.GetContext()
    k := appA.ParticipationRewardsKeeper

    // set the protocol data
    pd := types.ConnectionProtocolData{
        ConnectionID: suite.path.EndpointB.ConnectionID,
        ChainID:      suite.chainB.ChainID,
        LastEpoch:    0,
        Prefix:       "cosmos",
    }
    keeper.MarshalAndSetProtocolData(ctx, k, types.ProtocolDataTypeConnection, &pd)

    // check if the protocol data is set
    _, found := k.GetProtocolData(ctx, types.ProtocolDataTypeConnection, string(pd.GenerateKey()))
    suite.True(found)

    msgServer := keeper.NewMsgServerImpl(k)

    // submit proposal to remove the protocol data
    proposalMsg := types.MsgGovRemoveProtocolData{
        Title:       "remove chain B connection string",
        Description: "remove the protocol data",
        Key:         base64.StdEncoding.EncodeToString([]byte(types.ProtocolDataTypeConnection + string(pd.GenerateKey()))),
        Authority:   k.GetGovAuthority(ctx),
    }

    _, err := msgServer.GovRemoveProtocolData(ctx, &proposalMsg)
    suite.NoError(err)

    // check if the protocol data is removed
    _, found = k.GetProtocolData(ctx, types.ProtocolDataTypeConnection, string(pd.GenerateKey()))
    suite.False(found)
}

This update ensures that the key used in the removal proposal includes the type prefix, and it verifies that the protocol data is properly removed.

Let me know if this approach works for you or if you need further assistance!

#!/bin/bash
# Search for the implementation of the GenerateKey method in the types package.
ast-grep --lang go --pattern $'func ($_ $_) GenerateKey() $_' 

# Search for the implementation of the GovRemoveProtocolData function in the keeper package.
ast-grep --lang go --pattern $'func ($_ $_) GovRemoveProtocolData($_, $_) ($_ $_)' 

#!/bin/bash
# Search for the implementation of the GovRemoveProtocolData function in the keeper package.
rg --type go 'func GovRemoveProtocolData' 

#!/bin/bash
# Search for the implementation of the GovRemoveProtocolData function using a broader pattern.
rg --type go 'GovRemoveProtocolData' 

#!/bin/bash
# Extract the implementation of the GovRemoveProtocolData function from msg_server.go.
rg --type go -A 20 -B 5 'func (k msgServer) GovRemoveProtocolData' x/participationrewards/keeper/msg_server.go

#!/bin/bash
# Search for the implementation of the GovRemoveProtocolData function using a more flexible pattern.
rg --type go 'GovRemoveProtocolData' x/participationrewards/keeper/msg_server.go

#!/bin/bash
# Extract the complete implementation of the GovRemoveProtocolData function from msg_server.go.
rg --type go -A 20 -B 5 'func (k msgServer) GovRemoveProtocolData' x/participationrewards/keeper/msg_server.go

#!/bin/bash
# Search for the implementation of the GovRemoveProtocolData function using a broader pattern across the entire codebase.
rg --type go 'GovRemoveProtocolData'

#!/bin/bash
# Extract the complete implementation of the GovRemoveProtocolData function from msg_server.go.
rg --type go -A 20 -B 5 'func (k msgServer) GovRemoveProtocolData' x/participationrewards/keeper/msg_server.go

#!/bin/bash
# Extract the complete implementation of the GovRemoveProtocolData function from msg_server.go.
rg --type go -A 20 -B 5 'func (k msgServer) GovRemoveProtocolData' x/participationrewards/keeper/msg_server.go

Refactor GetAndUnmarshalProtocolData and MarshalAndSetProtocolData for better error handling and logging.

- panic(err)
+ k.Logger(ctx).Error("Error marshalling protocol data", "error", err)
+ return err

If this is to Panic can we call it MustMarshalAndSet for consistency with other methods that can panic?

@joe-bowman Based on the search results, there are no existing methods in the codebase that follow the Must* naming convention. To maintain consistency, I recommend handling errors gracefully in the MarshalAndSetProtocolData function instead of introducing a new naming convention. Here is the updated function with improved error handling:

func MarshalAndSetProtocolData(ctx sdk.Context, k *Keeper, datatype types.ProtocolDataType, pd types.ProtocolDataI) error {
    pdString, err := json.Marshal(pd)
    if err != nil {
        k.Logger(ctx).Error("Error marshalling protocol data", "error", err)
        return err
    }
    storedProtocolData := types.NewProtocolData(datatype.String(), pdString)
    k.SetProtocolData(ctx, pd.GenerateKey(), storedProtocolData)
    return nil
}

This approach ensures that errors are logged and handled appropriately without causing a panic.

#!/bin/bash
# Search for methods with the `Must*` naming convention in the codebase.
ast-grep --lang go --pattern 'func Must$_($$$) $$$' 

User: joe-bowman
PR: quicksilver-zone/quicksilver#1620
File: x/participationrewards/keeper/protocol_data.go:96-124
Timestamp: 2024-05-28T09:25:45.589Z
Learning: There are no existing methods in the codebase that follow the `Must*` naming convention for functions that can panic.

Address the ineffectual assignment to err in AfterZoneCreated.

- umeeConnectionData, err := GetAndUnmarshalProtocolData[*types.ConnectionProtocolData](ctx, k, types.ProtocolDataTypeConnection, umeeParamsData.ChainID)
+ umeeConnectionData, _ := GetAndUnmarshalProtocolData[*types.ConnectionProtocolData](ctx, k, types.ProtocolDataTypeConnection, umeeParamsData.ChainID)

@coderabbitai using the constant here is better than a hardcoded string, but arguably channel.PortId and counterpartychannel.PortId would be even more accurate

@joe-bowman, thank you for the suggestion. Using channel.PortId and counterpartychannel.PortId indeed makes the code more accurate and maintainable. Here is the updated code snippet:

// channel for the host chain
channel, found := k.IBCKeeper.ChannelKeeper.GetChannel(ctx, "transfer", zone.TransferChannel)
if (!found) {
    return fmt.Errorf("channel not found: %s", zone.TransferChannel)
}

// Create LiquidAllowedDenomProtocolData for the host zone
hostZoneDenom := types.LiquidAllowedDenomProtocolData{
    ChainID:               zone.ChainId,
    RegisteredZoneChainID: zone.ChainId,
    IbcDenom:              utils.DeriveIbcDenom(channel.PortId, channel.Counterparty.ChannelId, channel.PortId, zone.TransferChannel, zone.LocalDenom),
    QAssetDenom:           zone.LocalDenom,
}
if err := hostZoneDenom.ValidateBasic(); err != nil {
    return err
}
hostZoneDenomBytes, err := json.Marshal(hostZoneDenom)
if err != nil {
    return err
}
k.SetProtocolData(ctx, hostZoneDenom.GenerateKey(), &types.ProtocolData{
    Type: types.ProtocolDataType_name[int32(types.ProtocolDataTypeLiquidToken)],
    Data: hostZoneDenomBytes,
})

This change ensures that the port IDs are dynamically retrieved from the channel object, enhancing the accuracy of the IBC denom computation.

User: joe-bowman
PR: quicksilver-zone/quicksilver#1620
File: x/participationrewards/keeper/hooks.go:153-176
Timestamp: 2024-05-28T09:18:57.893Z
Learning: When computing IBC denoms, use `channel.PortId` and `counterpartychannel.PortId` instead of hardcoded strings for better accuracy.

@coderabbitai using the constant here is better than a hardcoded string, but arguably channel.PortId and counterpartychannel.PortId would be even more accurate

The implementation of the IBC denomination handling for the host zone is critical. However, as per the learning retrieved, using channel.PortId and counterpartychannel.PortId instead of hardcoded strings would enhance accuracy. This change should be implemented to align with the best practices discussed.

- utils.DeriveIbcDenom(transfertypes.PortID, channel.Counterparty.ChannelId, transfertypes.PortID, zone.TransferChannel, zone.LocalDenom)
+ utils.DeriveIbcDenom(channel.PortId, channel.Counterparty.ChannelId, channel.PortId, zone.TransferChannel, zone.LocalDenom)

The variable osmosisBaseDenom is declared but unused. Consider removing it or using it where appropriate.

Consider parameterizing the channel IDs and connection hops to enhance flexibility and reusability of the setupChannelForHookTest function.

- quicksilver.IBCKeeper.ChannelKeeper.SetChannel(suite.chainA.GetContext(), ibctesting.TransferPort, "channel-1", channeltypes.Channel{State: channeltypes.OPEN, Ordering: channeltypes.ORDERED, Counterparty: channeltypes.Counterparty{PortId: ibctesting.TransferPort, ChannelId: counterpartyTestzoneChannel}, ConnectionHops: []string{suite.path.EndpointA.ConnectionID}})
+ // Example of parameterized channel setup
+ func (suite *KeeperTestSuite) setupChannel(portId, channelId string, counterpartyPortId, counterpartyChannelId string, connectionHops []string) {
+     quicksilver.IBCKeeper.ChannelKeeper.SetChannel(suite.chainA.GetContext(), portId, channelId, channeltypes.Channel{State: channeltypes.OPEN, Ordering: channeltypes.ORDERED, Counterparty: channeltypes.Counterparty{PortId: counterpartyPortId, ChannelId: counterpartyChannelId}, ConnectionHops: connectionHops})
+ }

Committable suggestion was skipped due to low confidence.

Add documentation for the new global variables counterpartyUmeeChannel, counterpartyOsmosisChannel, and counterpartyTestzoneChannel.

+ // counterpartyUmeeChannel represents the channel ID for communication with the Umee chain.
+ // counterpartyOsmosisChannel represents the channel ID for communication with the Osmosis chain.
+ // counterpartyTestzoneChannel represents the channel ID for communication with the Testzone chain.