Fix: export InitTimeoutTimestamps and VscSendTimestamps to genesis

[yaruwangway]

Description

Closes: #612

InitTimeoutTimestamps and ExportedVscSendTimestamps are exported to genesis.
ExportedVscSendTimestamps is VscSendTimestamps with chainID.

type ExportedVscSendTimestamp struct {
	ChainId          string          
	VscSendTimestamp VscSendTimestamp 
}

exported Genesis:

 "provider": {
      "consumer_addition_proposals": [],
      "consumer_addrs_to_prune": [],
      "consumer_removal_proposals": [],
      "consumer_states": [],
      "exported_vsc_send_timestamps": [],
      "init_timeout_timestamps": [],
      "mature_unbonding_ops": {
        "ids": []
      },
...
}

---

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...

• Included the correct type prefix in the PR title
• Added ! to the type prefix if state-machine breaking change (i.e., requires coordinated upgrade)
• Confirmed this PR does not introduce changes requiring state migrations, OR migration code has been added to consumer and/or provider modules
• Targeted the correct branch (see PR Targeting)
• Provided a link to the relevant issue or specification
• Followed the guidelines for building SDK modules
• Included the necessary unit and integration tests
• Added a changelog entry to CHANGELOG.md
• Included comments for documenting Go code
• Updated the relevant documentation or specification
• Reviewed "Files changed" and left comments if necessary
• Confirmed all CI checks have passed

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

[yaruwangway]

a new field ChainId is added, so that when exporting VscSendTimestamp to genesis, the chainID info is carried.

[MSalopek]

Changing the order of fields inside a proto message is state breaking since this proto is used for storing state.

If you are adding a chain_id it should be done by adding it to the end (so it becomes string chain_id = 3).

I would advise against changing that structure, and instead change GenesisState of the provider like this:

type GenesisState struct {
	// strictly positive and set to 1 (DefaultValsetUpdateID) for a new chain
	ValsetUpdateId uint64 `protobuf:"varint,1,opt,name=valset_update_id,json=valsetUpdateId,proto3" json:"valset_update_id,omitempty"`
	// empty for a new chain
	ConsumerStates []ConsumerState `protobuf:"bytes,2,rep,name=consumer_states,json=consumerStates,proto3" json:"consumer_states" yaml:"consumer_states"`
	// empty for a new chain
	UnbondingOps []UnbondingOp `protobuf:"bytes,3,rep,name=unbonding_ops,json=unbondingOps,proto3" json:"unbonding_ops"`
	// empty for a new chain
	MatureUnbondingOps *types.MaturedUnbondingOps `protobuf:"bytes,4,opt,name=mature_unbonding_ops,json=matureUnbondingOps,proto3" json:"mature_unbonding_ops,omitempty"`
	// empty for a new chain
	ValsetUpdateIdToHeight []ValsetUpdateIdToHeight `protobuf:"bytes,5,rep,name=valset_update_id_to_height,json=valsetUpdateIdToHeight,proto3" json:"valset_update_id_to_height"`
	// empty for a new chain
	ConsumerAdditionProposals []ConsumerAdditionProposal `protobuf:"bytes,6,rep,name=consumer_addition_proposals,json=consumerAdditionProposals,proto3" json:"consumer_addition_proposals"`
	// empty for a new chain
	ConsumerRemovalProposals []ConsumerRemovalProposal `protobuf:"bytes,7,rep,name=consumer_removal_proposals,json=consumerRemovalProposals,proto3" json:"consumer_removal_proposals"`
	Params                   Params                    `protobuf:"bytes,8,opt,name=params,proto3" json:"params"`
	// empty for a new chain
	ValidatorConsumerPubkeys []ValidatorConsumerPubKey `protobuf:"bytes,9,rep,name=validator_consumer_pubkeys,json=validatorConsumerPubkeys,proto3" json:"validator_consumer_pubkeys"`
	// empty for a new chain
	ValidatorsByConsumerAddr []ValidatorByConsumerAddr `protobuf:"bytes,10,rep,name=validators_by_consumer_addr,json=validatorsByConsumerAddr,proto3" json:"validators_by_consumer_addr"`
	// empty for a new chain
	ConsumerAddrsToPrune []ConsumerAddrsToPrune `protobuf:"bytes,11,rep,name=consumer_addrs_to_prune,json=consumerAddrsToPrune,proto3" json:"consumer_addrs_to_prune"`
        InitTimeoutTImestamps ... // use the correct type
        VscSendTimestamps ... // use the correct type
}

That way we don't introduce state breaking changes, but have in mind that you would need to change the provider's Export genesis too

// x/ccv/provider/keeper
func (k Keeper) ExportGenesis(ctx sdk.Context) *types.GenesisState {...}

[yaruwangway]

Hi, this way, we lost the chainID info for VscSendTimestamps ?
how about create a new struct to wrap the original VscSendTimestamps with chainID for the genesis exporting.

[mpoke]

Yeah, unless it's necessary, try not to change the original structures as it will require state migration.

[MSalopek]

We don't lose it, it's stored as part of the key in the state.

You can have a structure like this when exporting/importing:

type GenesisState struct {
        InitTimeoutTImestamps ... // use the correct type
        VscSendTimestamps []ExportedVscSendTimestamps

Where ExportedVscSendTimestamps looks like:

struct ExportedVscSendTimestamps {
    ChainId string
    Timestamps []VscSendTimestamps
}

[yaruwangway]

adding chainID to VscSendTimestamp is api breaking ?

anway, can you review this PR ? Thank you!

[MSalopek]

If possible, let's not reimplement this.

I'm suggesting to keep the changes already made. The benefit of the refactor is minimal.

[shaspitz]

adding chainID to VscSendTimestamp is api breaking ?

If API breaking means that the genesis JSON schema would change, then yes. That's not a problem tho

Re @MSalopek's comment, imo for tech debt reasons we should avoid adding shims/wrapper types unless they're absolutely needed to avoid a state migration. In this case there is no state migration to avoid.

I'll approve as to not slow everyone down, but since this is a good first issue, I just wanna make sure we understand the mechanics behind things being state breaking

[yaruwangway]

im just not sure about the impact of the api breaking change, so please let me know, i could change this PR if adding chainID to VscSendTimestamp is better.

[shaspitz]

People use "API breaking" in different ways, so I can't comment on impact of an api breaking change. But I can say it's ok to change the JSON schema for import/export genesis, and I believe any solution #612 would require we change that JSON schema

@MSalopek @mpoke lmk if I'm incorrect here

Re best solution for the PR, I'll leave that up to the other approver, as this thread has gotten long

[shaspitz]

Tests look solid 👍 imo it's cleaner to not introduce a new type, and add a chain ID field to VscSendTimestamp for import/export. Note I don't believe this would require migrations due to the fact that we only store the timestamp field in value bytes (see SetVscSendTimestamp). ChainID is already stored in key bytes.

[MSalopek]

@yaruwangway Can you please resolve the conflicts and we can go over this once more?

[MSalopek]

Thanks!

All the comments are valid but would require re-writing the entire PR.

Since these changes are not breaking anything, I'm in favor of keeping them and moving on.