simplify consumer addition and removal propsal handling #460
Conversation
MSalopek
requested review from
jtremback,
mpoke,
sainoe and
danwt
as code owners
|
Hey, looks like excellent work Any chance we can incorporate Or perhps #463 should go in a different pr |
| // 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. |
There was a problem hiding this comment.
| // 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. |
|
|
||
| err := k.SetPendingConsumerAdditionProp(ctx, p) | ||
| if err != nil { | ||
| if err := k.SetPendingConsumerAdditionProp(ctx, p); err != nil { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
I'm not sure what you mean? The behaviour is the same with regards to error handling.
| // 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. |
There was a problem hiding this comment.
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.
| found := providerKeeper.GetPendingConsumerRemovalProp(ctx, tc.prop.ChainId, tc.prop.StopTime) | ||
| require.False(t, found) | ||
|
|
||
| testProviderStateIsCleaned(t, ctx, providerKeeper, tc.prop.ChainId, "channelID") |
There was a problem hiding this comment.
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.
| 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") |
There was a problem hiding this comment.
Similiar to another comment, an equivalent check should probably be tested
BeginBlocker()
testCreatedConsumerClient
|
Let's keep this on ice for a little bit. It is implemented per the spec. The issue remains open for further investigation. Closing. |
Immediate execution of CreateConsumerClient and StopConsumerClient is not necessary because all pending proposals are already handled in BeginBlock.
Closes: #458
Closes: #390