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: export InitTimeoutTimestamps and VscSendTimestamps to genesis #1076

Merged
merged 11 commits into from
Jul 19, 2023
Merged

Fix: export InitTimeoutTimestamps and VscSendTimestamps to genesis #1076

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
142efee
fix: add InitTimeoutTimestamps and VscSendTimestamps to genesis
Jun 23, 2023
27a50f2
merge main
Jun 23, 2023
9fc44d5
fix: add chainID to vscSendTimestamp
Jun 23, 2023
7099903
test: fix tests
Jun 23, 2023
6d94704
test: fixgenesis test
Jun 23, 2023
b845cbb
test: fix test TestInitAndExportGenesis
Jun 26, 2023
cb6588b
feat: add exportedVscSendTimestamps
Jun 26, 2023
ad6bf75
docs: update changelog
Jun 26, 2023
9f80b08
feat: update exportedVscSendTimestamps
Jun 26, 2023
0344988
fix: lint
Jun 27, 2023
36a8a6b
Merge branch 'main' into fix-export
yaruwangway Jul 19, 2023
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: 1 addition & 1 deletion proto/interchain_security/ccv/consumer/v1/genesis.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,4 +55,4 @@ message HeightToValsetUpdateID {

// OutstandingDowntime defines the genesis information for each validator
// flagged with an outstanding downtime slashing.
message OutstandingDowntime { string validator_consensus_address = 1; }
message OutstandingDowntime { string validator_consensus_address = 1; }
6 changes: 6 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 @@ -44,6 +44,12 @@ message GenesisState {
// empty for a new chain
repeated ConsumerAddrsToPrune consumer_addrs_to_prune = 11
[ (gogoproto.nullable) = false ];

repeated interchain_security.ccv.provider.v1.InitTimeoutTimestamp init_timeout_timestamps = 12
[ (gogoproto.nullable) = false ];

repeated interchain_security.ccv.provider.v1.VscSendTimestamp vsc_send_timestamps = 13
[ (gogoproto.nullable) = false ];
}

// consumer chain
Expand Down
5 changes: 3 additions & 2 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 @@ -216,8 +216,9 @@ message InitTimeoutTimestamp {
}

message VscSendTimestamp {
uint64 vsc_id = 1;
google.protobuf.Timestamp timestamp = 2
string chain_id = 1;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@MSalopek MSalopek Jun 26, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@yaruwangway yaruwangway Jun 26, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

adding chainID to VscSendTimestamp is api breaking ?

anway, can you review this PR ? Thank you!

Copy link
Contributor

Choose a reason for hiding this comment

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

If possible, let's not reimplement this.

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

uint64 vsc_id = 2;
google.protobuf.Timestamp timestamp = 3
[ (gogoproto.stdtime) = true, (gogoproto.nullable) = false ];
}

Expand Down
2 changes: 1 addition & 1 deletion 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 @@ -44,4 +44,4 @@ message MsgRegisterConsumerRewardDenom {

// MsgRegisterConsumerRewardDenomResponse defines the
// Msg/RegisterConsumerRewardDenom response type.
message MsgRegisterConsumerRewardDenomResponse {}
message MsgRegisterConsumerRewardDenomResponse {}
2 changes: 1 addition & 1 deletion proto/interchain_security/ccv/v1/ccv.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,4 +84,4 @@ enum ConsumerPacketDataType {
// VSCMatured packet
CONSUMER_PACKET_TYPE_VSCM = 2
[ (gogoproto.enumvalue_customname) = "VscMaturedPacket" ];
}
}
12 changes: 12 additions & 0 deletions x/ccv/provider/keeper/genesis.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,14 @@ func (k Keeper) InitGenesis(ctx sdk.Context, genState *types.GenesisState) {
}
}

for _, item := range genState.InitTimeoutTimestamps {
k.SetInitTimeoutTimestamp(ctx, item.ChainId, item.Timestamp)
}

for _, item := range genState.VscSendTimestamps {
k.SetVscSendTimestamp(ctx, item.ChainId, item.VscId, item.Timestamp)
}

k.SetParams(ctx, genState.Params)
k.InitializeSlashMeter(ctx)
}
Expand All @@ -99,6 +107,7 @@ func (k Keeper) ExportGenesis(ctx sdk.Context) *types.GenesisState {
// get a list of all registered consumer chains
registeredChains := k.GetAllConsumerChains(ctx)

var vscSendTimestamps []types.VscSendTimestamp
// export states for each consumer chains
var consumerStates []types.ConsumerState
for _, chain := range registeredChains {
Expand Down Expand Up @@ -129,6 +138,7 @@ func (k Keeper) ExportGenesis(ctx sdk.Context) *types.GenesisState {
cs.PendingValsetChanges = k.GetPendingVSCPackets(ctx, chain.ChainId)
consumerStates = append(consumerStates, cs)

vscSendTimestamps = append(vscSendTimestamps, k.GetAllVscSendTimestamps(ctx, chain.ChainId)...)
}

// ConsumerAddrsToPrune are added only for registered consumer chains
Expand All @@ -151,5 +161,7 @@ func (k Keeper) ExportGenesis(ctx sdk.Context) *types.GenesisState {
k.GetAllValidatorConsumerPubKeys(ctx, nil),
k.GetAllValidatorsByConsumerAddr(ctx, nil),
consumerAddrsToPrune,
k.GetAllInitTimeoutTimestamps(ctx),
vscSendTimestamps,
)
}
2 changes: 2 additions & 0 deletions x/ccv/provider/keeper/keeper.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -986,6 +986,7 @@ func (k Keeper) GetAllVscSendTimestamps(ctx sdk.Context, chainID string) (vscSen
}

vscSendTimestamps = append(vscSendTimestamps, types.VscSendTimestamp{
ChainId: chainID,
VscId: vscID,
Timestamp: ts,
})
Expand Down Expand Up @@ -1032,6 +1033,7 @@ func (k Keeper) GetFirstVscSendTimestamp(ctx sdk.Context, chainID string) (vscSe
}

return types.VscSendTimestamp{
ChainId: chainID,
VscId: vscID,
Timestamp: ts,
}, true
Expand Down
2 changes: 1 addition & 1 deletion x/ccv/provider/keeper/keeper_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -357,7 +357,7 @@ func TestVscSendTimestamp(t *testing.T) {
expectedGetAllOrder := []types.VscSendTimestamp{}
for _, tc := range testCases {
if tc.chainID == chainID {
expectedGetAllOrder = append(expectedGetAllOrder, types.VscSendTimestamp{VscId: tc.vscID, Timestamp: tc.ts})
expectedGetAllOrder = append(expectedGetAllOrder, types.VscSendTimestamp{ChainId: tc.chainID, VscId: tc.vscID, Timestamp: tc.ts})
}
}
// sorting by vscID
Expand Down
4 changes: 4 additions & 0 deletions x/ccv/provider/types/genesis.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@ func NewGenesisState(
validatorConsumerPubkeys []ValidatorConsumerPubKey,
validatorsByConsumerAddr []ValidatorByConsumerAddr,
consumerAddrsToPrune []ConsumerAddrsToPrune,
initTimeoutTimestamps []InitTimeoutTimestamp,
vscSendTimestamps []VscSendTimestamp,
) *GenesisState {
return &GenesisState{
ValsetUpdateId: vscID,
Expand All @@ -35,6 +37,8 @@ func NewGenesisState(
ValidatorConsumerPubkeys: validatorConsumerPubkeys,
ValidatorsByConsumerAddr: validatorsByConsumerAddr,
ConsumerAddrsToPrune: consumerAddrsToPrune,
InitTimeoutTimestamps: initTimeoutTimestamps,
VscSendTimestamps: vscSendTimestamps,
}
}

Expand Down
Loading
Loading