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.

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

Cherry-pick: Add MsgRegisterCounterparty Struct and Handler from ibc-lite #6982

Merged
merged 20 commits into from
Jul 31, 2024

Conversation

sangier
Copy link
Contributor

@sangier sangier commented Jul 29, 2024

Cherry Picking Serdar PR from ibc-lite branch

  • imp: added counterparty client store

  • imp: added provide counterparty to proto

  • imp: ran 'make proto-all'

  • imp: added logic to counterparty client

  • imp: fix proto

  • imp: fix proto

  • imp: ran 'make proto-all'

  • feat: finished counterparty client logic

Description

Closes #6967


Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

  • Targeted PR against the correct branch (see CONTRIBUTING.md).
  • Linked to GitHub issue with discussion and accepted design, OR link to spec that describes this work.
  • Code follows the module structure standards and Go style guide.
  • Wrote unit and integration tests.
  • Updated relevant documentation (docs/).
  • Added relevant godoc comments.
  • Provide a conventional commit message to follow the repository standards.
  • Include a descriptive changelog entry when appropriate. This may be left to the discretion of the PR reviewers. (e.g. chores should be omitted from changelog)
  • Re-reviewed Files changed in the GitHub PR explorer.
  • Review SonarCloud Report in the comment section below once CI passes.

* imp: added counterparty client store

* imp: added provide counterparty to proto

* imp: ran 'make proto-all'

* imp: added logic to counterparty client

* imp: fix proto

* imp: fix proto

* imp: ran 'make proto-all'

* feat: finished counterparty client logic
@AdityaSripal
Copy link
Member

AdityaSripal commented Jul 29, 2024

Thanks @sangier , I realized Serdar's pr on its own isn't complete. Is it possible for you to bring over these changes as well?

4e3f6cc

@AdityaSripal
Copy link
Member

Once that commit is in, we should also remove the newly added fields to CreateClient. They are unnecessary as we can get them done in a MultiMsg

@@ -43,7 +44,13 @@ func (k *Keeper) CreateClient(goCtx context.Context, msg *clienttypes.MsgCreateC
return nil, err
}

return &clienttypes.MsgCreateClientResponse{ClientId: clientID}, nil
k.ClientKeeper.SetCreator(ctx, clientID, msg.Signer)
Copy link
Member

Choose a reason for hiding this comment

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

Let's remove this logic since we can do it in a MultiMsg

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@AdityaSripal the creator setting k.ClientKeeper.SetCreator(ctx, clientID, msg.Signer) should be removed too?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't believe so? Should only remove the counterparty being set, creator is needed for the provide msg (but we can delete it from state there, possibly in follow up)

Copy link
Member

Choose a reason for hiding this comment

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

Yes, sorry we should keep this logic my bad.

Copy link
Contributor

Choose a reason for hiding this comment

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

we should also be able to add an assetion in the test func for this handler that asserts creator is set as expected

Copy link
Contributor

Choose a reason for hiding this comment

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

seems we only have indirect testing for this? see no TestCreateClient in msg_server_test.go

proto/ibc/core/client/v1/tx.proto Outdated Show resolved Hide resolved
modules/core/02-client/types/client.go Outdated Show resolved Hide resolved
proto/ibc/core/client/v1/tx.proto Outdated Show resolved Hide resolved
proto/ibc/core/client/v1/client.proto Outdated Show resolved Hide resolved

testCounterpartyID := "counterparty-1"
suite.keeper.SetCounterparty(suite.ctx, testClientID, testCounterpartyID)
merklePathPrefix := commitmenttypes.NewMerklePath("ibc", "")
Copy link
Member

Choose a reason for hiding this comment

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

This needs to change to bytes as mentioned by github. i believe there's a commit serdar has bumping the version. But you should probably just copy over those changes

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 still wip, I'm having troubles fixing the commitmenttypes

Copy link
Contributor

@DimitrisJim DimitrisJim left a comment

Choose a reason for hiding this comment

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

left some initial comments!

@@ -43,7 +44,13 @@ func (k *Keeper) CreateClient(goCtx context.Context, msg *clienttypes.MsgCreateC
return nil, err
}

return &clienttypes.MsgCreateClientResponse{ClientId: clientID}, nil
k.ClientKeeper.SetCreator(ctx, clientID, msg.Signer)
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't believe so? Should only remove the counterparty being set, creator is needed for the provide msg (but we can delete it from state there, possibly in follow up)

modules/core/24-host/client_paths.go Outdated Show resolved Hide resolved
modules/core/02-client/keeper/keeper_test.go Show resolved Hide resolved
modules/core/02-client/keeper/keeper.go Outdated Show resolved Hide resolved
modules/core/24-host/client_keys.go Outdated Show resolved Hide resolved
Copy link
Member

@AdityaSripal AdityaSripal left a comment

Choose a reason for hiding this comment

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

Great work! Minor fixes needed and then tests for the top-level msg server and then I think we're good to go!

modules/core/02-client/keeper/keeper_test.go Outdated Show resolved Hide resolved
modules/core/02-client/types/msgs.go Outdated Show resolved Hide resolved
modules/core/02-client/types/msgs.go Show resolved Hide resolved
@@ -43,7 +44,13 @@ func (k *Keeper) CreateClient(goCtx context.Context, msg *clienttypes.MsgCreateC
return nil, err
}

return &clienttypes.MsgCreateClientResponse{ClientId: clientID}, nil
k.ClientKeeper.SetCreator(ctx, clientID, msg.Signer)
Copy link
Member

Choose a reason for hiding this comment

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

Yes, sorry we should keep this logic my bad.

modules/core/keeper/msg_server.go Show resolved Hide resolved
Copy link
Contributor

@DimitrisJim DimitrisJim left a comment

Choose a reason for hiding this comment

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

left some more things on second go, looking great!

proto/ibc/core/client/v1/client.proto Outdated Show resolved Hide resolved
proto/ibc/core/client/v1/client.proto Outdated Show resolved Hide resolved
modules/core/02-client/types/msgs.go Outdated Show resolved Hide resolved
modules/core/02-client/types/msgs.go Outdated Show resolved Hide resolved
Copy link
Member

@AdityaSripal AdityaSripal left a comment

Choose a reason for hiding this comment

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

Nice work!

modules/core/02-client/keeper/keeper_test.go Outdated Show resolved Hide resolved
modules/core/24-host/client_keys.go Outdated Show resolved Hide resolved
modules/core/keeper/msg_server.go Outdated Show resolved Hide resolved
// Verify that the retrieved creator matches the expected creator
suite.Require().True(found, "GetCreator did not return stored creator")
suite.Require().Equal(expectedCreator, retrievedCreator, "Creator is not retrieved correctly")

Copy link
Member

Choose a reason for hiding this comment

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

nit: could add a delete here to test that as well

Copy link

@AdityaSripal AdityaSripal merged commit 8d10fed into feat/ibc-eureka Jul 31, 2024
62 of 65 checks passed
@AdityaSripal AdityaSripal deleted the stefano/MsgRegisterCounterparty branch July 31, 2024 10:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants