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

client: upgraded consensus state #82

Merged
merged 9 commits into from
Mar 22, 2021
Merged

client: upgraded consensus state #82

merged 9 commits into from
Mar 22, 2021

Conversation

fedekunze
Copy link
Contributor

@fedekunze fedekunze commented Mar 12, 2021

closes #5

@fedekunze fedekunze marked this pull request as ready for review March 12, 2021 17:27
@fedekunze fedekunze requested a review from AdityaSripal as a code owner March 12, 2021 17:27
Copy link
Contributor

@colin-axner colin-axner left a comment

Choose a reason for hiding this comment

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

Looks great! Thanks @fedekunze. Left some minor requests

Comment on lines 162 to 164
// client id and plan height field have been deprecated. Use gRPC header for setting the height instead.
reserved 1, 2;
reserved "client_id", "plan_height";
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can just remove these. I don't think we have done a release with this proto definition (since it was added when removing IBC from x/upgrade)

@@ -240,3 +234,40 @@ func (q Keeper) UpgradedClientState(c context.Context, req *types.QueryUpgradedC
UpgradedClientState: any,
}, nil
}

// UpgradedConsensusState implements the Query/UpgradedConsensusState gRPC method
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you add some comments on this function indicating that the context height should be set for the expected plan height (otherwise this will always return a consensus state not found)

Copy link
Member

Choose a reason for hiding this comment

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

Also this will only return a valid consensus state on the last block of the chain and not for any other height

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also this will only return a valid consensus state on the last block of the chain and not for any other height

you can set the height on the grpc req metadata

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this still needs godoc?

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.

looks good @fedekunze , have some nits below

@@ -240,3 +234,40 @@ func (q Keeper) UpgradedClientState(c context.Context, req *types.QueryUpgradedC
UpgradedClientState: any,
}, nil
}

// UpgradedConsensusState implements the Query/UpgradedConsensusState gRPC method
Copy link
Member

Choose a reason for hiding this comment

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

Also this will only return a valid consensus state on the last block of the chain and not for any other height

modules/core/02-client/keeper/grpc_query.go Outdated Show resolved Hide resolved
proto/ibc/core/client/v1/query.proto Show resolved Hide resolved
proto/ibc/core/client/v1/query.proto Show resolved Hide resolved
@codecov-io
Copy link

Codecov Report

Merging #82 (b36300c) into main (97f8985) will increase coverage by 0.03%.
The diff coverage is 52.63%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main      #82      +/-   ##
==========================================
+ Coverage   65.19%   65.22%   +0.03%     
==========================================
  Files         127      127              
  Lines        8392     8405      +13     
==========================================
+ Hits         5471     5482      +11     
+ Misses       2558     2557       -1     
- Partials      363      366       +3     
Impacted Files Coverage Δ
modules/core/02-client/keeper/grpc_query.go 61.70% <50.00%> (+1.23%) ⬆️
modules/core/02-client/keeper/keeper.go 83.53% <100.00%> (+0.71%) ⬆️

@mergify mergify bot merged commit 9c6edc4 into main Mar 22, 2021
@mergify mergify bot deleted the fedekunze/upg-consstate branch March 22, 2021 22:16
CosmosCar pushed a commit to caelus-labs/ibc-go that referenced this pull request Nov 6, 2023
Transaction aggregation.

Resolves cosmos#69.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add gRPC for upgraded consensus state
4 participants