Skip to content
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

Jump to bottom

fix!: remove stopTime from MsgRemoveConsumer #2208

Merged
merged 14 commits into from
Sep 4, 2024
Merged

fix!: remove stopTime from MsgRemoveConsumer #2208

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
0870cb0
init commit
insumity Sep 3, 2024
78a1459
renamed stop time to removal time
insumity Sep 3, 2024
f8e8829
resolve merge conflict
mpoke Sep 3, 2024
48175e8
fix errors in tests
mpoke Sep 3, 2024
a34b9b5
resolve merge conflict
mpoke Sep 3, 2024
ad70394
Merge branch 'feat/permissionless' into insumity/fix-stop-time
insumity Sep 4, 2024
1186fa9
add GetAllConsumerWithIBCClients
mpoke Sep 4, 2024
060bf60
use GetAllConsumerWithIBCClients for efficiency
mpoke Sep 4, 2024
8457336
fix UTs
mpoke Sep 4, 2024
2f7e04c
remove GetAllLaunchedConsumerIds as not needed
mpoke Sep 4, 2024
e41f7fd
typo in GetAllConsumersWithIBCClients
mpoke Sep 4, 2024
ef5ca2b
improve comment
mpoke Sep 4, 2024
eef6bb8
Apply suggestions from code review
insumity Sep 4, 2024
e1adcc5
took into account comments
insumity Sep 4, 2024
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
2 changes: 2 additions & 0 deletions proto/interchain_security/ccv/provider/v1/genesis.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,8 @@ message ConsumerState {
repeated interchain_security.ccv.v1.ValidatorSetChangePacketData
pending_valset_changes = 6 [ (gogoproto.nullable) = false ];
repeated string slash_downtime_ack = 7;
// the phase of the consumer chain
ConsumerPhase phase = 9;
mpoke marked this conversation as resolved.
Show resolved Hide resolved
}

// ValsetUpdateIdToHeight defines the genesis information for the mapping
Expand Down
2 changes: 2 additions & 0 deletions proto/interchain_security/ccv/provider/v1/provider.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -480,4 +480,6 @@ enum ConsumerPhase {
CONSUMER_PHASE_LAUNCHED = 3;
// STOPPED defines the phase in which a previously-launched chain has stopped.
CONSUMER_PHASE_STOPPED = 4;
// DELETED defines the phase in which the state of a stopped chain has been deleted.
CONSUMER_PHASE_DELETED = 5;
}
2 changes: 1 addition & 1 deletion proto/interchain_security/ccv/provider/v1/query.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -156,7 +156,7 @@ message QueryConsumerGenesisResponse {

message QueryConsumerChainsRequest {
// The phase of the consumer chains returned (optional)
// Registered=1|Initialized=2|Launched=3|Stopped=4
// Registered=1|Initialized=2|Launched=3|Stopped=4|Deleted=5
ConsumerPhase phase = 1;
// The limit of consumer chains returned (optional)
// default is 100
Expand Down
7 changes: 1 addition & 6 deletions proto/interchain_security/ccv/provider/v1/tx.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -223,17 +223,12 @@

// MsgRemoveConsumer defines the message used to remove (and stop) a consumer chain.
// If it passes, all the consumer chain's state is eventually removed from the provider chain.
message MsgRemoveConsumer {

Check failure on line 226 in proto/interchain_security/ccv/provider/v1/tx.proto

View workflow job for this annotation

GitHub Actions / break-check 

Previously present field "3" with name "signer" on message "MsgRemoveConsumer" was deleted.
option (cosmos.msg.v1.signer) = "signer";

// the consumer id of the consumer chain to be stopped
string consumer_id = 1;
// the time on the provider chain at which all validators are responsible to
// stop their consumer chain validator node
google.protobuf.Timestamp stop_time = 2
[ (gogoproto.stdtime) = true, (gogoproto.nullable) = false ];
// signer address
string signer = 3 [(cosmos_proto.scalar) = "cosmos.AddressString"];
string signer = 2 [(cosmos_proto.scalar) = "cosmos.AddressString"];

Check failure on line 231 in proto/interchain_security/ccv/provider/v1/tx.proto

View workflow job for this annotation

GitHub Actions / break-check 

Field "2" with name "signer" on message "MsgRemoveConsumer" changed cardinality from "optional with explicit presence" to "optional with implicit presence".

Check failure on line 231 in proto/interchain_security/ccv/provider/v1/tx.proto

View workflow job for this annotation

GitHub Actions / break-check 

Field "2" with name "signer" on message "MsgRemoveConsumer" changed option "json_name" from "stopTime" to "signer".

Check failure on line 231 in proto/interchain_security/ccv/provider/v1/tx.proto

View workflow job for this annotation

GitHub Actions / break-check 

Field "2" with name "signer" on message "MsgRemoveConsumer" changed type from "message" to "string".

Check failure on line 231 in proto/interchain_security/ccv/provider/v1/tx.proto

View workflow job for this annotation

GitHub Actions / break-check 

Field "2" on message "MsgRemoveConsumer" changed name from "stop_time" to "signer".
}

// MsgRemoveConsumerResponse defines response type for MsgRemoveConsumer messages
Expand Down
9 changes: 4 additions & 5 deletions tests/e2e/action_rapid_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -261,11 +261,10 @@ func GetSubmitConsumerAdditionProposalActionGen() *rapid.Generator[SubmitConsume
func GetSubmitConsumerRemovalProposalActionGen() *rapid.Generator[SubmitConsumerRemovalProposalAction] {
return rapid.Custom(func(t *rapid.T) SubmitConsumerRemovalProposalAction {
return SubmitConsumerRemovalProposalAction{
Chain: GetChainIDGen().Draw(t, "Chain"),
From: GetValidatorIDGen().Draw(t, "From"),
Deposit: rapid.Uint().Draw(t, "Deposit"),
ConsumerChain: GetChainIDGen().Draw(t, "ConsumerChain"),
StopTimeOffset: time.Duration(rapid.Int64().Draw(t, "StopTimeOffset")),
Chain: GetChainIDGen().Draw(t, "Chain"),
From: GetValidatorIDGen().Draw(t, "From"),
Deposit: rapid.Uint().Draw(t, "Deposit"),
ConsumerChain: GetChainIDGen().Draw(t, "ConsumerChain"),
}
})
}
Expand Down
20 changes: 8 additions & 12 deletions tests/e2e/actions.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -740,11 +740,10 @@ func (tr Chain) submitConsumerAdditionLegacyProposal(
}

type SubmitConsumerRemovalProposalAction struct {
Chain ChainID
From ValidatorID
Deposit uint
ConsumerChain ChainID
StopTimeOffset time.Duration // offset from time.Now()
Chain ChainID
From ValidatorID
Deposit uint
ConsumerChain ChainID
}

func (tr Chain) submitConsumerRemovalProposal(
Expand All @@ -762,7 +761,6 @@ func (tr Chain) submitConsumerRemovalProposal(

msg := types.MsgRemoveConsumer{
ConsumerId: consumerId,
StopTime: tr.testConfig.containerConfig.Now.Add(action.StopTimeOffset),
Signer: authority,
}

Expand Down Expand Up @@ -811,13 +809,11 @@ func (tr Chain) submitConsumerRemovalLegacyProposal(
action SubmitConsumerRemovalProposalAction,
verbose bool,
) {
stopTime := tr.testConfig.containerConfig.Now.Add(action.StopTimeOffset)
prop := client.ConsumerRemovalProposalJSON{
Title: fmt.Sprintf("Stop the %v chain", action.ConsumerChain),
Summary: "It was a great chain",
ChainId: string(tr.testConfig.chainConfigs[action.ConsumerChain].ChainId),
StopTime: stopTime,
Deposit: fmt.Sprint(action.Deposit) + `stake`,
Title: fmt.Sprintf("Stop the %v chain", action.ConsumerChain),
Summary: "It was a great chain",
ChainId: string(tr.testConfig.chainConfigs[action.ConsumerChain].ChainId),
Deposit: fmt.Sprint(action.Deposit) + `stake`,
}

bz, err := json.Marshal(prop)
Expand Down
9 changes: 3 additions & 6 deletions tests/e2e/state.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -525,7 +525,6 @@ func (tr Commands) GetProposal(chain ChainID, proposal uint) Proposal {
}
case "/interchain_security.ccv.provider.v1.MsgRemoveConsumer":
consumerId := rawContent.Get("consumer_id").String()
stopTime := rawContent.Get("stop_time").Time().Sub(tr.containerConfig.Now)

var chain ChainID
for i, conf := range tr.chainConfigs {
Expand All @@ -536,10 +535,9 @@ func (tr Commands) GetProposal(chain ChainID, proposal uint) Proposal {
}

return ConsumerRemovalProposal{
Deposit: uint(deposit),
Status: status,
Chain: chain,
StopTime: int(stopTime.Milliseconds()),
Deposit: uint(deposit),
Status: status,
Chain: chain,
}
case "/ibc.applications.transfer.v1.MsgUpdateParams":
var params IBCTransferParams
Expand Down Expand Up @@ -757,7 +755,6 @@ func (tr Commands) GetIBCTransferParams(chain ChainID) IBCTransferParams {
func (tr Commands) GetConsumerChains(chain ChainID) map[ChainID]bool {
binaryName := tr.chainConfigs[chain].BinaryName
cmd := tr.target.ExecCommand(binaryName,

"query", "provider", "list-consumer-chains",
`--node`, tr.GetQueryNode(chain),
`-o`, `json`,
Expand Down
7 changes: 3 additions & 4 deletions tests/e2e/state_rapid_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -203,10 +203,9 @@ func GetConsumerAdditionProposalGen() *rapid.Generator[ConsumerAdditionProposal]
func GetConsumerRemovalProposalGen() *rapid.Generator[ConsumerRemovalProposal] {
return rapid.Custom(func(t *rapid.T) ConsumerRemovalProposal {
return ConsumerRemovalProposal{
Deposit: rapid.Uint().Draw(t, "Deposit"),
Chain: GetChainIDGen().Draw(t, "Chain"),
StopTime: rapid.Int().Draw(t, "StopTime"),
Status: rapid.String().Draw(t, "Status"),
Deposit: rapid.Uint().Draw(t, "Deposit"),
Chain: GetChainIDGen().Draw(t, "Chain"),
Status: rapid.String().Draw(t, "Status"),
}
})
}
Expand Down
50 changes: 21 additions & 29 deletions tests/e2e/steps_stop_chain.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,8 @@
package main

import (
"strconv"
"time"

gov "github.com/cosmos/cosmos-sdk/x/gov/types/v1"
"strconv"
)

// start relayer so that all messages are relayed
Expand All @@ -22,11 +20,10 @@ func stepsStopChain(consumerName string, propNumber uint) []Step {
s := []Step{
{
Action: SubmitConsumerRemovalProposalAction{
Chain: ChainID("provi"),
From: ValidatorID("bob"),
Deposit: 10000001,
ConsumerChain: ChainID(consumerName),
StopTimeOffset: 0 * time.Millisecond,
Chain: ChainID("provi"),
From: ValidatorID("bob"),
Deposit: 10000001,
ConsumerChain: ChainID(consumerName),
},
State: State{
ChainID("provi"): ChainState{
Expand All @@ -35,10 +32,9 @@ func stepsStopChain(consumerName string, propNumber uint) []Step {
},
Proposals: &map[uint]Proposal{
propNumber: ConsumerRemovalProposal{
Deposit: 10000001,
Chain: ChainID(consumerName),
StopTime: 0,
Status: strconv.Itoa(int(gov.ProposalStatus_PROPOSAL_STATUS_VOTING_PERIOD)),
Deposit: 10000001,
Chain: ChainID(consumerName),
Status: strconv.Itoa(int(gov.ProposalStatus_PROPOSAL_STATUS_VOTING_PERIOD)),
},
},
ConsumerChains: &map[ChainID]bool{"consu": true}, // consumer chain not yet removed
Expand All @@ -56,10 +52,9 @@ func stepsStopChain(consumerName string, propNumber uint) []Step {
ChainID("provi"): ChainState{
Proposals: &map[uint]Proposal{
propNumber: ConsumerRemovalProposal{
Deposit: 10000001,
Chain: ChainID(consumerName),
StopTime: 0,
Status: strconv.Itoa(int(gov.ProposalStatus_PROPOSAL_STATUS_PASSED)),
Deposit: 10000001,
Chain: ChainID(consumerName),
Status: strconv.Itoa(int(gov.ProposalStatus_PROPOSAL_STATUS_PASSED)),
},
},
ValBalances: &map[ValidatorID]uint{
Expand All @@ -80,11 +75,10 @@ func stepsConsumerRemovalPropNotPassing(consumerName string, propNumber uint) []
s := []Step{
{
Action: SubmitConsumerRemovalProposalAction{
Chain: ChainID("provi"),
From: ValidatorID("bob"),
Deposit: 10000001,
ConsumerChain: ChainID(consumerName),
StopTimeOffset: 0 * time.Millisecond,
Chain: ChainID("provi"),
From: ValidatorID("bob"),
Deposit: 10000001,
ConsumerChain: ChainID(consumerName),
},
State: State{
ChainID("provi"): ChainState{
Expand All @@ -93,10 +87,9 @@ func stepsConsumerRemovalPropNotPassing(consumerName string, propNumber uint) []
},
Proposals: &map[uint]Proposal{
propNumber: ConsumerRemovalProposal{
Deposit: 10000001,
Chain: ChainID(consumerName),
StopTime: 0,
Status: strconv.Itoa(int(gov.ProposalStatus_PROPOSAL_STATUS_VOTING_PERIOD)),
Deposit: 10000001,
Chain: ChainID(consumerName),
Status: strconv.Itoa(int(gov.ProposalStatus_PROPOSAL_STATUS_VOTING_PERIOD)),
},
},
ConsumerChains: &map[ChainID]bool{"consu": true}, // consumer chain not removed
Expand All @@ -114,10 +107,9 @@ func stepsConsumerRemovalPropNotPassing(consumerName string, propNumber uint) []
ChainID("provi"): ChainState{
Proposals: &map[uint]Proposal{
propNumber: ConsumerRemovalProposal{
Deposit: 10000001,
Chain: ChainID(consumerName),
StopTime: 0,
Status: strconv.Itoa(int(gov.ProposalStatus_PROPOSAL_STATUS_REJECTED)),
Deposit: 10000001,
Chain: ChainID(consumerName),
Status: strconv.Itoa(int(gov.ProposalStatus_PROPOSAL_STATUS_REJECTED)),
},
},
ValBalances: &map[ValidatorID]uint{
Expand Down
7 changes: 3 additions & 4 deletions tests/e2e/testlib/types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -306,10 +306,9 @@ func (p UpgradeProposal) isProposal() {}
func (p ConsumerAdditionProposal) isProposal() {}

type ConsumerRemovalProposal struct {
Deposit uint
Chain ChainID
StopTime int
Status string
Deposit uint
Chain ChainID
Status string
}

func (p ConsumerRemovalProposal) isProposal() {}
Expand Down
14 changes: 6 additions & 8 deletions tests/e2e/trace_handlers_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,10 +27,9 @@ var proposalInStateSteps = []Step{
ChainID("provi"): ChainState{
Proposals: &map[uint]Proposal{
1: ConsumerRemovalProposal{
Deposit: 10000001,
Chain: ChainID("foo"),
StopTime: 0,
Status: strconv.Itoa(int(gov.ProposalStatus_PROPOSAL_STATUS_VOTING_PERIOD)),
Deposit: 10000001,
Chain: ChainID("foo"),
Status: strconv.Itoa(int(gov.ProposalStatus_PROPOSAL_STATUS_VOTING_PERIOD)),
},
},
},
Expand Down Expand Up @@ -136,10 +135,9 @@ func TestMarshalAndUnmarshalChainState(t *testing.T) {
"consumer removal proposal": {ChainState{
Proposals: &map[uint]Proposal{
5: ConsumerRemovalProposal{
Deposit: 10000001,
Chain: ChainID("test123"),
StopTime: 5000000000,
Status: strconv.Itoa(int(gov.ProposalStatus_PROPOSAL_STATUS_PASSED)),
Deposit: 10000001,
Chain: ChainID("test123"),
Status: strconv.Itoa(int(gov.ProposalStatus_PROPOSAL_STATUS_PASSED)),
},
},
ValBalances: &map[ValidatorID]uint{
Expand Down
9 changes: 3 additions & 6 deletions tests/e2e/v4/state.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -317,7 +317,6 @@ func (tr Commands) GetProposal(chain ChainID, proposal uint) Proposal {
}
case "/interchain_security.ccv.provider.v1.ConsumerRemovalProposal":
chainId := gjson.Get(string(bz), `messages.0.content.chain_id`).String()
stopTime := gjson.Get(string(bz), `messages.0.content.stop_time`).Time().Sub(containerConfig.Now)

var chain ChainID
for i, conf := range chainConfigs {
Expand All @@ -328,10 +327,9 @@ func (tr Commands) GetProposal(chain ChainID, proposal uint) Proposal {
}

return ConsumerRemovalProposal{
Deposit: uint(deposit),
Status: status,
Chain: chain,
StopTime: int(stopTime.Milliseconds()),
Deposit: uint(deposit),
Status: status,
Chain: chain,
}
case "/cosmos.params.v1beta1.ParameterChangeProposal":
return ParamsProposal{
Expand Down Expand Up @@ -391,7 +389,6 @@ func (tr Commands) GetParam(chain ChainID, param Param) string {
func (tr Commands) GetConsumerChains(chain ChainID) map[ChainID]bool {
binaryName := tr.ChainConfigs[chain].BinaryName
cmd := tr.Target.ExecCommand(binaryName,

"query", "provider", "list-consumer-chains",
`--node`, tr.GetQueryNode(chain),
`-o`, `json`,
Expand Down
7 changes: 5 additions & 2 deletions tests/integration/stop_consumer.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package integration
import (
"cosmossdk.io/math"
channeltypes "github.com/cosmos/ibc-go/v8/modules/core/04-channel/types"
"github.com/cosmos/interchain-security/v5/x/ccv/provider/types"

sdk "github.com/cosmos/cosmos-sdk/types"
stakingtypes "github.com/cosmos/cosmos-sdk/x/staking/types"
Expand Down Expand Up @@ -88,7 +89,8 @@ func (s *CCVTestSuite) TestStopConsumerChain() {
}

// stop the consumer chain
err = providerKeeper.StopConsumerChain(s.providerCtx(), firstBundle.ConsumerId, true)
providerKeeper.SetConsumerPhase(s.providerCtx(), firstBundle.ConsumerId, types.ConsumerPhase_CONSUMER_PHASE_STOPPED)
err = providerKeeper.DeleteConsumerChain(s.providerCtx(), firstBundle.ConsumerId)
s.Require().NoError(err)

// check all states are removed and the unbonding operation released
Expand All @@ -105,7 +107,8 @@ func (s *CCVTestSuite) TestStopConsumerOnChannelClosed() {
providerKeeper := s.providerApp.GetProviderKeeper()

// stop the consumer chain
err := providerKeeper.StopConsumerChain(s.providerCtx(), s.getFirstBundle().ConsumerId, true)
providerKeeper.SetConsumerPhase(s.providerCtx(), s.getFirstBundle().ConsumerId, types.ConsumerPhase_CONSUMER_PHASE_STOPPED)
err := providerKeeper.DeleteConsumerChain(s.providerCtx(), s.getFirstBundle().ConsumerId)
Comment on lines +110 to +111
Copy link
Contributor

Choose a reason for hiding this comment

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

Complete the test function.

The changes made to stop the consumer chain using the new methods SetConsumerPhase and DeleteConsumerChain are in line with the PR objectives. However, the test function is incomplete and has commented out code that needs to be implemented.

Please complete the test function by implementing the following:

  1. Simulate the relayer behavior by calling OnChanCloseConfirm on the provider chain's channel end.
  2. Check for the panic in the consumer chain's BeginBlock.
  3. Check if the provider's channel is removed.

Do you want me to open a GitHub issue to track this task?

s.Require().NoError(err)

err = s.path.EndpointA.UpdateClient()
Expand Down
5 changes: 2 additions & 3 deletions testutil/keeper/expectations.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,9 +80,8 @@ func GetMocksForSetConsumerChain(ctx sdk.Context, mocks *MockedKeepers,
}
}

// GetMocksForStopConsumerChainWithCloseChannel returns mock expectations needed to call StopConsumerChain() when
// `closeChan` is true.
func GetMocksForStopConsumerChainWithCloseChannel(ctx sdk.Context, mocks *MockedKeepers) []*gomock.Call {
// GetMocksForDeleteConsumerChain returns mock expectations needed to call `DeleteConsumerChain`
func GetMocksForDeleteConsumerChain(ctx sdk.Context, mocks *MockedKeepers) []*gomock.Call {
dummyCap := &capabilitytypes.Capability{}
return []*gomock.Call{
mocks.MockChannelKeeper.EXPECT().GetChannel(gomock.Any(), types.ProviderPortID, "channelID").Return(
Expand Down
Loading
Loading