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

simplify consumer addition and removal propsal handling #460

Closed
wants to merge 2 commits into from
Closed

simplify consumer addition and removal propsal handling #460

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
27e7a73
simplify consumer addition and removal propsal handling
MSalopek Nov 9, 2022
aee04a2
fix linter errors in proposal tests
MSalopek Nov 9, 2022
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
25 changes: 3 additions & 22 deletions x/ccv/provider/keeper/proposal.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,20 +20,9 @@ import (
)

// HandleConsumerAdditionProposal will receive the consumer chain's client state from the proposal.
// If the spawn time has already passed, then set the consumer chain. Otherwise store the client
// as a pending client, and set once spawn time has passed.
//
// Note: This method implements SpawnConsumerChainProposalHandler in spec.
// See: https://github.com/cosmos/ibc/blob/main/spec/app/ics-028-cross-chain-validation/methods.md#ccv-pcf-spccprop1
// Spec tag: [CCV-PCF-SPCCPROP.1]
// The proposal is stored into state and if spawn time has passed the client will be created set in the next BeginBlocker.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// The proposal is stored into state and if spawn time has passed the client will be created set in the next BeginBlocker.
// The proposal is stored into state and will be executed in BeginBlocker once the spawn time has passed.

func (k Keeper) HandleConsumerAdditionProposal(ctx sdk.Context, p *types.ConsumerAdditionProposal) error {
if !ctx.BlockTime().Before(p.SpawnTime) {
// lockUbdOnTimeout is set to be false, regardless of what the proposal says, until we can specify and test issues around this use case more thoroughly
return k.CreateConsumerClient(ctx, p.ChainId, p.InitialHeight, false)
}

err := k.SetPendingConsumerAdditionProp(ctx, p)
if err != nil {
if err := k.SetPendingConsumerAdditionProp(ctx, p); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

We should be a little careful about the error semantics because the err is used in the gov module when the proposal is submitted.

See #463

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure what you mean? The behaviour is the same with regards to error handling.

return err
}

Expand Down Expand Up @@ -99,16 +88,8 @@ func (k Keeper) CreateConsumerClient(ctx sdk.Context, chainID string,

// HandleConsumerRemovalProposal stops a consumer chain and released the outstanding unbonding operations.
// If the stop time hasn't already passed, it stores the proposal as a pending proposal.
//
// This method implements StopConsumerChainProposalHandler from spec.
// See: https://github.com/cosmos/ibc/blob/main/spec/app/ics-028-cross-chain-validation/methods.md#ccv-pcf-stccprop1
// Spec tag: [CCV-PCF-STCCPROP.1]
// If the stop time has passed the proposal will be executed in BeginBlocker.
Copy link
Contributor

Choose a reason for hiding this comment

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

We could consolidate comments on 90-91

The proposal is stored into state and will be executed in BeginBlocker once the spawn time has passed.

func (k Keeper) HandleConsumerRemovalProposal(ctx sdk.Context, p *types.ConsumerRemovalProposal) error {

if !ctx.BlockTime().Before(p.StopTime) {
return k.StopConsumerChain(ctx, p.ChainId, false, true)
}

k.SetPendingConsumerRemovalProp(ctx, p.ChainId, p.StopTime)
return nil
}
Expand Down
68 changes: 19 additions & 49 deletions x/ccv/provider/keeper/proposal_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,15 +29,15 @@ import (
// Tests the HandleConsumerAdditionProposal method against the SpawnConsumerChainProposalHandler spec.
// See: https://github.com/cosmos/ibc/blob/main/spec/app/ics-028-cross-chain-validation/methods.md#ccv-pcf-spccprop1
// Spec tag: [CCV-PCF-SPCCPROP.1]
//
// NOTE: immediate handling of proposals is not necessary - all proposals are executed in BeginBlock.
func TestHandleConsumerAdditionProposal(t *testing.T) {

type testCase struct {
description string
prop *providertypes.ConsumerAdditionProposal
// Time when prop is handled
blockTime time.Time
// Whether it's expected that the spawn time has passed and client should be created
expCreatedClient bool
}

// Snapshot times asserted in tests
Expand All @@ -46,7 +46,7 @@ func TestHandleConsumerAdditionProposal(t *testing.T) {

tests := []testCase{
{
description: "ctx block time is after proposal's spawn time, expected that client is created",
description: "ctx block time is after proposal's spawn time, expected that client is persisted as pending - execution is in BeginBlocker",
prop: providertypes.NewConsumerAdditionProposal(
"title",
"description",
Expand All @@ -56,8 +56,7 @@ func TestHandleConsumerAdditionProposal(t *testing.T) {
[]byte("bin_hash"),
now, // Spawn time
).(*providertypes.ConsumerAdditionProposal),
blockTime: hourFromNow,
expCreatedClient: true,
blockTime: hourFromNow,
},
{
description: `ctx block time is before proposal's spawn time,
Expand All @@ -71,41 +70,29 @@ func TestHandleConsumerAdditionProposal(t *testing.T) {
[]byte("bin_hash"),
hourFromNow, // Spawn time
).(*types.ConsumerAdditionProposal),
blockTime: now,
expCreatedClient: false,
blockTime: now,
},
}

for _, tc := range tests {
// Common setup
keeperParams := testkeeper.NewInMemKeeperParams(t)
providerKeeper, ctx, ctrl, mocks := testkeeper.GetProviderKeeperAndCtx(t, keeperParams)
providerKeeper, ctx, ctrl, _ := testkeeper.GetProviderKeeperAndCtx(t, keeperParams)
providerKeeper.SetParams(ctx, providertypes.DefaultParams())
ctx = ctx.WithBlockTime(tc.blockTime)

if tc.expCreatedClient {
// Mock calls are only asserted if we expect a client to be created.
gomock.InOrder(
testkeeper.GetMocksForCreateConsumerClient(ctx, &mocks, "chainID", clienttypes.NewHeight(2, 3))...,
)
}

tc.prop.LockUnbondingOnTimeout = false // Full functionality not implemented yet.

err := providerKeeper.HandleConsumerAdditionProposal(ctx, tc.prop)
require.NoError(t, err)

if tc.expCreatedClient {
testCreatedConsumerClient(t, ctx, providerKeeper, tc.prop.ChainId, "clientID")
Copy link
Contributor

Choose a reason for hiding this comment

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

Similiar to another comment, an equivalent check should probably be tested

BeginBlocker()
testCreatedConsumerClient

} else {
// check that stored pending prop is exactly the same as the initially instantiated prop
gotProposal, found := providerKeeper.GetPendingConsumerAdditionProp(ctx, tc.prop.SpawnTime, tc.prop.ChainId)
require.True(t, found)
require.Equal(t, *tc.prop, gotProposal)
// double check that a client for this chain does not exist
_, found = providerKeeper.GetConsumerClientId(ctx, tc.prop.ChainId)
require.False(t, found)
}
// check that stored pending prop is exactly the same as the initially instantiated prop
gotProposal, found := providerKeeper.GetPendingConsumerAdditionProp(ctx, tc.prop.SpawnTime, tc.prop.ChainId)
require.True(t, found)
require.Equal(t, *tc.prop, gotProposal)
// double check that a client for this chain does not exist
_, found = providerKeeper.GetConsumerClientId(ctx, tc.prop.ChainId)
require.False(t, found)
ctrl.Finish()
}
}
Expand Down Expand Up @@ -311,6 +298,8 @@ func TestPendingConsumerAdditionPropOrder(t *testing.T) {
//
// See: https://github.com/cosmos/ibc/blob/main/spec/app/ics-028-cross-chain-validation/methods.md#ccv-pcf-stccprop1
// Spec tag: [CCV-PCF-STCCPROP.1]
//
// NOTE: immediate handling of proposals is not necessary - all proposals are executed in BeginBlock.
func TestHandleConsumerRemovalProposal(t *testing.T) {

type testCase struct {
Expand All @@ -319,8 +308,6 @@ func TestHandleConsumerRemovalProposal(t *testing.T) {
prop *types.ConsumerRemovalProposal
// Time when prop is handled
blockTime time.Time
// Whether consumer chain should have been stopped
expStop bool
}

// Snapshot times asserted in tests
Expand All @@ -337,7 +324,6 @@ func TestHandleConsumerRemovalProposal(t *testing.T) {
now,
).(*providertypes.ConsumerRemovalProposal),
blockTime: hourFromNow, // After stop time.
expStop: true,
},
{
description: "valid proposal: stop time has not yet been reached",
Expand All @@ -348,39 +334,23 @@ func TestHandleConsumerRemovalProposal(t *testing.T) {
hourFromNow,
).(*providertypes.ConsumerRemovalProposal),
blockTime: now, // Before proposal's stop time
expStop: false,
},
}

for _, tc := range tests {

// Common setup
keeperParams := testkeeper.NewInMemKeeperParams(t)
providerKeeper, ctx, ctrl, mocks := testkeeper.GetProviderKeeperAndCtx(t, keeperParams)
providerKeeper, ctx, ctrl, _ := testkeeper.GetProviderKeeperAndCtx(t, keeperParams)
providerKeeper.SetParams(ctx, providertypes.DefaultParams())
ctx = ctx.WithBlockTime(tc.blockTime)

// Mock expectations and setup for stopping the consumer chain, if applicable
if tc.expStop {
testkeeper.SetupForStoppingConsumerChain(t, ctx, &providerKeeper, mocks)
}
// Note: when expStop is false, no mocks are setup,
// meaning no external keeper methods are allowed to be called.

err := providerKeeper.HandleConsumerRemovalProposal(ctx, tc.prop)
require.NoError(t, err)

if tc.expStop {
// Expect no pending proposal to exist
found := providerKeeper.GetPendingConsumerRemovalProp(ctx, tc.prop.ChainId, tc.prop.StopTime)
require.False(t, found)

testProviderStateIsCleaned(t, ctx, providerKeeper, tc.prop.ChainId, "channelID")
Comment on lines -375 to -378
Copy link
Contributor

Choose a reason for hiding this comment

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

In replacing these lines we want to maintain the same amount of coverage. Here the proposal actually was processed so testProviderStateIsCleaned checks that the consumer was removed. An equivalent should be added. ie.

BeginBlocker()
testProviderStateIsCleaned

to check the same thing.

} else {
// Proposal should be stored as pending
found := providerKeeper.GetPendingConsumerRemovalProp(ctx, tc.prop.ChainId, tc.prop.StopTime)
require.True(t, found)
}
// Proposal should be stored as pending
found := providerKeeper.GetPendingConsumerRemovalProp(ctx, tc.prop.ChainId, tc.prop.StopTime)
require.True(t, found)

// Assert mock calls from setup function
ctrl.Finish()
Expand Down
30 changes: 10 additions & 20 deletions x/ccv/provider/proposal_handler_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ package provider_test
import (
sdk "github.com/cosmos/cosmos-sdk/types"
distributiontypes "github.com/cosmos/cosmos-sdk/x/distribution/types"
"github.com/golang/mock/gomock"
"github.com/stretchr/testify/require"

clienttypes "github.com/cosmos/ibc-go/v3/modules/core/02-client/types"
Expand All @@ -27,26 +26,25 @@ func TestConsumerChainProposalHandler(t *testing.T) {
hourFromNow := now.Add(time.Hour).UTC()

testCases := []struct {
name string
content govtypes.Content
blockTime time.Time
expValidConsumerAddition bool
expValidConsumerRemoval bool
name string
content govtypes.Content
blockTime time.Time
expValidProposal bool
}{
{
name: "valid consumer addition proposal",
content: types.NewConsumerAdditionProposal(
"title", "description", "chainID",
clienttypes.NewHeight(2, 3), []byte("gen_hash"), []byte("bin_hash"), now),
blockTime: hourFromNow, // ctx blocktime is after proposal's spawn time
expValidConsumerAddition: true,
blockTime: hourFromNow, // ctx blocktime is after proposal's spawn time
expValidProposal: true,
},
{
name: "valid consumer removal proposal",
content: types.NewConsumerRemovalProposal(
"title", "description", "chainID", now),
blockTime: hourFromNow,
expValidConsumerRemoval: true,
blockTime: hourFromNow,
expValidProposal: true,
},
{
name: "nil proposal",
Expand All @@ -65,23 +63,15 @@ func TestConsumerChainProposalHandler(t *testing.T) {

// Setup
keeperParams := testkeeper.NewInMemKeeperParams(t)
providerKeeper, ctx, ctrl, mocks := testkeeper.GetProviderKeeperAndCtx(t, keeperParams)
providerKeeper, ctx, ctrl, _ := testkeeper.GetProviderKeeperAndCtx(t, keeperParams)
providerKeeper.SetParams(ctx, providertypes.DefaultParams())
ctx = ctx.WithBlockTime(tc.blockTime)

// Mock expectations depending on expected outcome
if tc.expValidConsumerAddition {
gomock.InOrder(testkeeper.GetMocksForCreateConsumerClient(ctx, &mocks, "chainID", clienttypes.NewHeight(2, 3))...)
}
if tc.expValidConsumerRemoval {
testkeeper.SetupForStoppingConsumerChain(t, ctx, &providerKeeper, mocks)
}

// Execution
proposalHandler := provider.NewConsumerChainProposalHandler(providerKeeper)
err := proposalHandler(ctx, tc.content)

if tc.expValidConsumerAddition || tc.expValidConsumerRemoval {
if tc.expValidProposal {
require.NoError(t, err)
} else {
require.Error(t, err)
Expand Down