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

Allow localhost to be created #7148

Merged
merged 3 commits into from
Aug 24, 2020
Merged

Allow localhost to be created #7148

merged 3 commits into from
Aug 24, 2020

Conversation

colin-axner
Copy link
Contributor

@colin-axner colin-axner commented Aug 24, 2020

Description

reverts changes in: #6974
closes: #6975

I had a hunch #7000 fixed this so decided to give it a shot. Locally works except for an existing issue in after import. We should not merge this until that is fixed and all sims pass.

A default genesis state creates a localhost. ExportGenesis sets "CreateLocalhost" to false because any created localhost will be exported with the clients. I'm pretty sure an exported/import with this set to true would cause panics since the localhost would be set but then the init genesis would try to create another localhost


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 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.
  • Wrote unit and integration tests
  • Updated relevant documentation (docs/) or specification (x/<module>/spec/)
  • Added relevant godoc comments.
  • Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
  • Re-reviewed Files changed in the Github PR explorer
  • Review Codecov Report in the comment section below once CI passes

@colin-axner colin-axner added x/ibc C:genesis relating to chain genesis labels Aug 24, 2020
@codecov
Copy link

codecov bot commented Aug 24, 2020

Codecov Report

Merging #7148 into master will increase coverage by 0.02%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #7148      +/-   ##
==========================================
+ Coverage   54.72%   54.74%   +0.02%     
==========================================
  Files         563      563              
  Lines       38405    38405              
==========================================
+ Hits        21016    21024       +8     
+ Misses      15681    15671      -10     
- Partials     1708     1710       +2     

Copy link
Collaborator

@fedekunze fedekunze left a comment

Choose a reason for hiding this comment

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

LGTM 👍

x/ibc/02-client/keeper/grpc_query_test.go Outdated Show resolved Hide resolved
@@ -147,7 +156,6 @@ func (suite *KeeperTestSuite) TestQueryClientStates() {
for i := range expClientStates {
suite.Require().Equal(expClientStates[i].ClientId, res.ClientStates[i].ClientId)
suite.Require().NotNil(res.ClientStates[i].ClientState)
expClientStates[i].ClientState.ClearCachedValue()
Copy link
Collaborator

Choose a reason for hiding this comment

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

I had to do this bc the Equal statement would fail if the expected response had the value cached. Using Proto equal would work too

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It passes now when using the testing package. Going to leave as is but we can add back if necessary

@fedekunze fedekunze added the A:automerge Automatically merge PR once all prerequisites pass. label Aug 24, 2020
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.

small question on test case. non-blocking

@@ -13,12 +13,6 @@ func (suite *ClientTestSuite) TestHandleCreateClientLocalHost() {
msg exported.MsgCreateClient
expPass bool
}{
{
"localhost",
Copy link
Member

Choose a reason for hiding this comment

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

Why is this test-case removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

localhost is already created in genesis. This test would fail, changing it to false would just make it a duplication of the later test case "client already exists",

@mergify mergify bot merged commit 49e4d05 into master Aug 24, 2020
@mergify mergify bot deleted the colin/revert-localhost branch August 24, 2020 18:37
@colin-axner
Copy link
Contributor Author

colin-axner commented Aug 25, 2020

Hmm, I don't think we should have automerged this, but I guess its fine. after-import is still breaking from #7135 . cc/ @fedekunze

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A:automerge Automatically merge PR once all prerequisites pass. C:genesis relating to chain genesis
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enable support for IBC to only use proto
3 participants