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

Remove solo machine deprecated fields #2761

Merged
merged 9 commits into from
Nov 17, 2022
Merged

Remove solo machine deprecated fields #2761

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
ea52a04
deprecated(api)!: removed deprecated `ClientId` field from `Misbehavi…
chatton Nov 15, 2022
d00dbb5
chore: removing references to ClientID
chatton Nov 15, 2022
b244874
chore: removing test which references AllowUpdateAfterProposal
chatton Nov 15, 2022
d7d0362
chore: removing test referencing ClientID
chatton Nov 15, 2022
dfe69ab
Merge branch 'main' into cian/issue#1866-remove-solo-machine-deprecat…
chatton Nov 15, 2022
5a9f72a
Merge branch 'main' into cian/issue#1866-remove-solo-machine-deprecat…
chatton Nov 16, 2022
157736d
chore: adding manual CHANGELOG entry
chatton Nov 16, 2022
9a36981
chore: adding reserved field for client_id
chatton Nov 16, 2022
025f7c3
chore: changed ordering of fields in Misbehaviour
chatton Nov 16, 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
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
* (modules/apps/27-interchain-accounts) [\#2433](https://github.com/cosmos/ibc-go/pull/2450) Renamed icatypes.PortPrefix to icatypes.ControllerPortPrefix & icatypes.PortID to icatypes.HostPortID
* (core/02-client) [\#2573](https://github.com/cosmos/ibc-go/pull/2573) Renames `ClientParams` gRPC query method to `Params`.
* (testing) [\#2567](https://github.com/cosmos/ibc-go/pull/2567) Modify `SendPacket` API of `Endpoint` to match the API of `SendPacket` in 04-channel.
* (06-solomachine) [\#2761](https://github.com/cosmos/ibc-go/pull/2761) Removed deprecated `ClientId` field from `Misbehaviour` and `allow_update_after_proposal` field from `ClientState`.

### State Machine Breaking

Expand Down
5 changes: 0 additions & 5 deletions modules/light-clients/06-solomachine/misbehaviour.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ import (
sdkerrors "github.com/cosmos/cosmos-sdk/types/errors"

clienttypes "github.com/cosmos/ibc-go/v6/modules/core/02-client/types"
host "github.com/cosmos/ibc-go/v6/modules/core/24-host"
"github.com/cosmos/ibc-go/v6/modules/core/exported"
)

Expand All @@ -24,10 +23,6 @@ func (misbehaviour Misbehaviour) Type() string {

// ValidateBasic implements Misbehaviour interface.
func (misbehaviour Misbehaviour) ValidateBasic() error {
if err := host.ClientIdentifierValidator(misbehaviour.ClientId); err != nil {
return sdkerrors.Wrap(err, "invalid client identifier for solo machine")
}

if misbehaviour.Sequence == 0 {
return sdkerrors.Wrap(clienttypes.ErrInvalidMisbehaviour, "sequence cannot be 0")
}
Expand Down
7 changes: 0 additions & 7 deletions modules/light-clients/06-solomachine/misbehaviour_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,13 +26,6 @@ func (suite *SoloMachineTestSuite) TestMisbehaviourValidateBasic() {
func(*solomachine.Misbehaviour) {},
true,
},
{
"invalid client ID",
func(misbehaviour *solomachine.Misbehaviour) {
misbehaviour.ClientId = "(badclientid)"
},
false,
},
{
"sequence is zero",
func(misbehaviour *solomachine.Misbehaviour) {
Expand Down
5 changes: 0 additions & 5 deletions modules/light-clients/06-solomachine/proposal_handle_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,11 +23,6 @@ func (suite *SoloMachineTestSuite) TestCheckSubstituteAndUpdateState() {
malleate func()
expPass bool
}{
{
"valid substitute", func() {
subjectClientState.AllowUpdateAfterProposal = true
}, true,
},
{
"substitute is not the solo machine", func() {
substituteClientState = &ibctm.ClientState{}
Expand Down
193 changes: 54 additions & 139 deletions modules/light-clients/06-solomachine/solomachine.pb.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

11 changes: 3 additions & 8 deletions proto/ibc/lightclients/solomachine/v3/solomachine.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,6 @@ message ClientState {
// frozen sequence of the solo machine
bool is_frozen = 2 [(gogoproto.moretags) = "yaml:\"is_frozen\""];
ConsensusState consensus_state = 3 [(gogoproto.moretags) = "yaml:\"consensus_state\""];
// when set to true, will allow governance to update a solo machine client.
// The client will be unfrozen if it is frozen.
bool allow_update_after_proposal = 4 [(gogoproto.moretags) = "yaml:\"allow_update_after_proposal\""];
Copy link
Member

Choose a reason for hiding this comment

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

ditto

Copy link
Contributor

Choose a reason for hiding this comment

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

Is this still possible since the package version changed and this was the last field? I think if it is attempted to be used, the codec would fail on an unknown field?

}

// ConsensusState defines a solo machine consensus state. The sequence of a
Expand Down Expand Up @@ -51,11 +48,9 @@ message Header {
message Misbehaviour {
option (gogoproto.goproto_getters) = false;

// ClientID is deprecated
string client_id = 1 [deprecated = true, (gogoproto.moretags) = "yaml:\"client_id\""];
Copy link
Member

Choose a reason for hiding this comment

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

It may be useful to reserve these fields. cc. @colin-axner

https://developers.google.com/protocol-buffers/docs/proto3#reserved

Copy link
Contributor

Choose a reason for hiding this comment

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

Make sense to me. Would this still be a concern if we shifted all the field numbers?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

is this the usage you had in mind @damiannolan

message Misbehaviour {
  option (gogoproto.goproto_getters) = false;

  uint64           sequence      = 2;
  SignatureAndData signature_one = 3 [(gogoproto.moretags) = "yaml:\"signature_one\""];
  SignatureAndData signature_two = 4 [(gogoproto.moretags) = "yaml:\"signature_two\""];

  reserved 1;
}

Copy link
Member

Choose a reason for hiding this comment

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

hmm, perhaps we should just shift all the field numbers considering its going to be API breaking anyways and require migrations. I think its probably safe to shift them then. Any thoughts or reservations on doing that?

Copy link
Contributor

@colin-axner colin-axner Nov 16, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I think it should be fine. I don't see any security considerations since this is for providing misbehavior and it isn't stored within the state machine (no internal migrations are actually needed for this type)

uint64 sequence = 2;
SignatureAndData signature_one = 3 [(gogoproto.moretags) = "yaml:\"signature_one\""];
SignatureAndData signature_two = 4 [(gogoproto.moretags) = "yaml:\"signature_two\""];
uint64 sequence = 1;
SignatureAndData signature_one = 2 [(gogoproto.moretags) = "yaml:\"signature_one\""];
SignatureAndData signature_two = 3 [(gogoproto.moretags) = "yaml:\"signature_two\""];
}

// SignatureAndData contains a signature and the data signed over to create that
Expand Down
1 change: 0 additions & 1 deletion testing/solomachine.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -212,7 +212,6 @@ func (solo *Solomachine) CreateMisbehaviour() *solomachine.Misbehaviour {
}

return &solomachine.Misbehaviour{
ClientId: solo.ClientID,
Sequence: solo.Sequence,
SignatureOne: &signatureOne,
SignatureTwo: &signatureTwo,
Expand Down