refactor!: Split out consumer genesis state #1324

bermuell · 2023-09-22T13:48:43Z

Description

Closes: #1214

Refacoring shared data between provider and consumer.

Note: This breaks json format used by augmenting Genesis files of consumer chains with consumer genesis content exported from provider chain. Consumer Genesis content exported from a provider chain using major version 1,2 or3 of the provider module needs to be transformed with the transformation command introduced by this PR:

Transform the consumer genesis file from a provider version v1,v2 or v3 to a version supported by this consumer. Result is printed to STDOUT.

Example:
$ <appd> transform /path/to/ccv_consumer_genesis.json

Usage:
  interchain-security-cd genesis transform [genesis-file] [flags]

Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

confirmed the correct type prefix in the PR title
confirmed ! in the type prefix if API or client breaking change
confirmed this PR does not introduce changes requiring state migrations, OR confirmed migration code has been added to consumer and/or provider modules
confirmed all author checklist items have been addressed
reviewed state machine logic
reviewed API design and naming
reviewed documentation is accurate
reviewed tests and test coverage

MSalopek · 2023-09-25T17:05:56Z

The refactor looks good, thanks for the awesome work!

What is a bit unclear to me is if we should maintain backward compatibility with regards to responses from the consumer-genesis provider query:

interchain-security-pd query provider consumer-genesis <chain-id>

The structure returned by this query is an integral part of starting a consumer chain, so maybe we should address the issue of provider-consumer version compatibility.

This is the scenario I'm trying to solve:

consumer is on the newest version of ICS (ICS@latest)
provider is on an old version of ICS (lCS@v3.1)
after spawn_time the consumer_genesis becomes available on the provider
the consumer queries the provider for the consumer_genesis
the consumer cannot parse the consumer_genesis in the format sent by the provider chain when doing interchain-security-pd query provider consumer-genesis <chain-id> since the provider is sending an old version of the genesis

It seems to me that this issue can be solved by parsing the "old" genesis into the "new" genesis:

interchain-security-cd parse-genesis v1 <path_to_v1_genesis>

# output of this cmd is a genesis that the consumer can process (ICS@latest)

If the provider and consumer are on the same ICS version there are no compatibility issues.

bermuell · 2023-09-26T11:47:32Z

thanks @MSalopek for your comment.
Your scenario is the exactly the one we need to provide a way to deal with it.

I'll look into that and provide migration code for my changes

shaspitz · 2023-09-26T16:16:14Z

Matija's suggestion of creating a new parsing function sounds solid to me with appropriate docs for the procedure 👍

proto/interchain_security/ccv/consumer/v1/genesis.proto

Rename NewRestartConsumerGenesisState to NewRestartGenesisState

shaspitz · 2023-10-06T01:53:40Z

proto/interchain_security/ccv/consumer/v1/genesis.proto

+//
+// Note: this type is only used on consumer side and references shared types with
+// provider
+message GenesisState {


In #1288 we renamed the consumer's GenesisState struct to ConsumerGenesisState, is there a reason why we change it back in this PR?

We changed it in the shared type namespace to make it clear that it's consumer related. This type is the result of the split and resides in the consumer namespaces so no need to repeat that in the name. The shared one kept its name

shaspitz · 2023-10-06T01:56:50Z

app/consumer/genesis.go

+ TransformationMap map[string]TransformationCallback
+)
+
+// Migration of consumer genesis content as it is exported from a provider version v2


Prob need some clarification on the use of "v2" and "v3" here, it seems like the term is applied to both consumer and provider. Are we referring to ICS major versions?

Yes this is the major version of ICS modules we refer here.

MSalopek · 2023-10-10T12:50:17Z

@bermuell docs are needed for the change to CLI.

app/consumer/genesis.go

…merGenesis

MSalopek

LGTM!

sainoe · 2023-10-13T13:49:45Z

app/consumer/genesis.go

+ cmd := &cobra.Command{
+ Use: "transform [genesis-file]",
+ Short: "Transform CCV consumer genesis from an older provider version not supporting current format",
+ Long: fmt.Sprintf(`Transform the consumer genesis file from a provider version v1,v2 or v3 to a version supported by this consumer. Result is printed to STDOUT.


nit: perhaps add a strings.TrimSpace() call here

sainoe

LGTM! See my comment.

Refactor consumer genesis state * Split ConsumerGenesis datatype * Move other non-shared types to consumer * fix some protbuf lint issues * make proto-format * Rename PrivateConsumerGenesisState to GenesisState * Fix linter issues * Correct some proto buf * Protobuf use deprecated instead of reserved * Add genesis transformation command to interchain-security-cd * Remove unused field from test data * Added some basic checks * Fix linter warnings * Added test * Minor test fixes + lint warnings * Fix lint warning * Updated comment * Addressed comments * Update docs * Addressed comments

github-actions bot added C:Testing Assigned automatically by the PR labeler C:x/consumer Assigned automatically by the PR labeler C:x/provider Assigned automatically by the PR labeler C:x/types Assigned automatically by the PR labeler labels Sep 22, 2023

bermuell force-pushed the bernd/1214-splitConsumerGenesis branch from 92d02bd to 32ad930 Compare September 22, 2023 13:50

bermuell assigned shaspitz and MSalopek Sep 22, 2023

MSalopek assigned bermuell and unassigned MSalopek and shaspitz Sep 22, 2023

bermuell requested review from shaspitz and MSalopek September 25, 2023 07:10

shaspitz reviewed Sep 26, 2023

View reviewed changes

proto/interchain_security/ccv/consumer/v1/genesis.proto Outdated Show resolved Hide resolved

bermuell force-pushed the bernd/1214-splitConsumerGenesis branch 2 times, most recently from 8c2a751 to 6731b7c Compare October 3, 2023 12:29

bermuell and others added 12 commits October 3, 2023 15:09

Split ConsumerGenesis datatype

0189212

Move other non-shared types to consumer

8ef2167

fix some protbuf lint issues

f60fc54

make proto-format

bff3d09

Rename PrivateConsumerGenesisState to GenesisState

1b0a565

Rename NewRestartConsumerGenesisState to NewRestartGenesisState

Fix linter issues

ae9aa50

Correct some proto buf

3635fa0

Protobuf use deprecated instead of reserved

320bb50

Add genesis transformation command to interchain-security-cd

cf80174

Remove unused field from test data

52bf055

Added some basic checks

e99770a

Fix linter warnings

0948a6e

bermuell force-pushed the bernd/1214-splitConsumerGenesis branch 3 times, most recently from ed13e20 to 9d7c31f Compare October 5, 2023 14:53

Added test

8326811

bermuell force-pushed the bernd/1214-splitConsumerGenesis branch from 9d7c31f to 8326811 Compare October 5, 2023 14:59

bermuell changed the title ~~refactor! Split out consumer genesis state~~ refactor Split out consumer genesis state Oct 5, 2023

bermuell added 2 commits October 5, 2023 17:46

Minor test fixes + lint warnings

ea77c6e

Fix lint warning

1de4699

bermuell marked this pull request as ready for review October 5, 2023 16:20

bermuell requested a review from a team as a code owner October 5, 2023 16:20

shaspitz reviewed Oct 6, 2023

View reviewed changes

Updated comment

043219f

bermuell requested a review from shaspitz October 10, 2023 07:44

MSalopek changed the title ~~refactor Split out consumer genesis state~~ refactor!: Split out consumer genesis state Oct 10, 2023

MSalopek reviewed Oct 10, 2023

View reviewed changes

app/consumer/genesis.go Outdated Show resolved Hide resolved

app/consumer/genesis.go Outdated Show resolved Hide resolved

app/consumer/genesis.go Outdated Show resolved Hide resolved

github-actions bot added the C:Docs Assigned automatically by the PR labeler label Oct 11, 2023

Addressed comments

6d94ef8

bermuell force-pushed the bernd/1214-splitConsumerGenesis branch from f8b2524 to 6d94ef8 Compare October 11, 2023 09:24

bermuell requested a review from sainoe October 11, 2023 15:53

Merge remote-tracking branch 'origin/main' into bernd/1214-splitConsu…

0e31c3b

…merGenesis

MSalopek approved these changes Oct 12, 2023

View reviewed changes

Update docs

a60d199

sainoe reviewed Oct 13, 2023

View reviewed changes

sainoe self-requested a review October 13, 2023 13:57

sainoe approved these changes Oct 13, 2023

View reviewed changes

Addressed comments

2713327

bermuell merged commit 946f6ec into main Oct 13, 2023
13 of 15 checks passed

bermuell deleted the bernd/1214-splitConsumerGenesis branch October 13, 2023 15:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor!: Split out consumer genesis state #1324

refactor!: Split out consumer genesis state #1324

bermuell commented Sep 22, 2023 •

edited

Loading

MSalopek commented Sep 25, 2023 •

edited

Loading

bermuell commented Sep 26, 2023 •

edited

Loading

shaspitz commented Sep 26, 2023

shaspitz Oct 6, 2023

bermuell Oct 6, 2023 •

edited

Loading

shaspitz Oct 6, 2023

bermuell Oct 6, 2023 •

edited

Loading

MSalopek commented Oct 10, 2023

MSalopek left a comment

sainoe Oct 13, 2023

sainoe left a comment

refactor!: Split out consumer genesis state #1324

refactor!: Split out consumer genesis state #1324

Conversation

bermuell commented Sep 22, 2023 • edited Loading

Description

Author Checklist

Reviewers Checklist

MSalopek commented Sep 25, 2023 • edited Loading

bermuell commented Sep 26, 2023 • edited Loading

shaspitz commented Sep 26, 2023

shaspitz Oct 6, 2023

Choose a reason for hiding this comment

bermuell Oct 6, 2023 • edited Loading

Choose a reason for hiding this comment

shaspitz Oct 6, 2023

Choose a reason for hiding this comment

bermuell Oct 6, 2023 • edited Loading

Choose a reason for hiding this comment

MSalopek commented Oct 10, 2023

MSalopek left a comment

Choose a reason for hiding this comment

sainoe Oct 13, 2023

Choose a reason for hiding this comment

sainoe left a comment

Choose a reason for hiding this comment

bermuell commented Sep 22, 2023 •

edited

Loading

MSalopek commented Sep 25, 2023 •

edited

Loading

bermuell commented Sep 26, 2023 •

edited

Loading

bermuell Oct 6, 2023 •

edited

Loading

bermuell Oct 6, 2023 •

edited

Loading