x/upgrade gRPC methods for VersionMap #9073

technicallyty · 2021-04-07T19:41:44Z

Description

+Adds gRPC definitions to query the VersionMap from x/upgrade store
+Adds CLI method for VersionMap gRPC query
+Adds cli_test.go file for testing VersionMap cli query
+Updates the swagger file with the new gRPC endpoint

This PR includes changes on unrelated proto files as a result of the proto formatter

closes: #8941

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

-add optional field to get specific module from version map -added cli test for version map query from cli

-return better errors in grpc methods

codecov · 2021-04-07T19:51:31Z

Codecov Report

Merging #9073 (e05c9c9) into master (ad49ec1) will decrease coverage by 0.17%.
The diff coverage is 87.27%.

❗ Current head e05c9c9 differs from pull request most recent head 40749ce. Consider uploading reports for the commit 40749ce to get more accurate results

@@            Coverage Diff             @@
##           master    #9073      +/-   ##
==========================================
- Coverage   60.30%   60.12%   -0.18%     
==========================================
  Files         588      590       +2     
  Lines       37001    37170     +169     
==========================================
+ Hits        22313    22349      +36     
- Misses      12711    12841     +130     
- Partials     1977     1980       +3

Impacted Files	Coverage Δ
x/upgrade/client/cli/query.go	`22.22% <72.00%> (ø)`
x/upgrade/keeper/grpc_query.go	`74.07% <100.00%> (+15.25%)`	⬆️
x/upgrade/keeper/keeper.go	`79.60% <100.00%> (+3.09%)`	⬆️
x/staking/types/msg.go	`54.92% <0.00%> (-2.40%)`	⬇️
x/staking/client/cli/tx.go	`17.98% <0.00%> (ø)`
x/upgrade/client/cli/tx.go	`0.00% <0.00%> (ø)`
x/staking/keeper/msg_server.go	`0.45% <0.00%> (+0.01%)`	⬆️

amaury1093

looks good, but we should probably avoid maps

proto/cosmos/upgrade/v1beta1/query.proto

amaury1093 · 2021-04-08T10:43:03Z

proto/cosmos/upgrade/v1beta1/query.proto

+// RPC method.
+message QueryVersionMapResponse {
+  // version_map is a map of module name to consensus version.
+  map<string, uint64> version_map = 1;


Hmm, i'm a bit wary of this. With ADR-033, Query services are part of the state machine, so I would like to avoid maps altogether (looping through them is not deterministic).

Could we do repeated ModuleConsensusVersion, where is ModuleConsensusVersion is a message with 2 fields string and uint64? And then this array would be sorted by module name (similarly to how we sort sdk.Coins).

I think there ends up being a trade-off here in terms of efficiency vs. safety against the novice module developer.

so if a module developer converts the map to an array for later storage, there is 100% chance that the next queried map won't have the same ordering.

we do add some overhead to the query though, as we have to convert the map to a slice, and then alphabetically sort the slice by module name. (i think thats like O(nlogn*m). this may be negligible for a small query like this though.

if we did go with returning a slice, it would probably be better to change the query name from VersionMap to something else (ModuleConsensusVersions?), since the underlying return type is no longer a Map.

curious if anyone else has thoughts - @aaronc ?

I would love to use protobuf maps, and I think it's safer to just use a repeated field instead. Using protobuf maps in APIs is something that will require its whole own "safety study" or something.

sounds good to me. the current commit on this PR has the repeated field implemented. That just leaves the issue of the naming:

QueryVersionMap returning a slice would be a bit odd imo. I am thinking we can either

rename it

convert slice to map when request resolves in client

thoughts @aaronc @AmauryM ?

Let's call it QueryModuleVersions

QueryModuleVersions sounds good to me too

renamed
@aaronc @AmauryM

x/upgrade/client/cli/query.go

x/upgrade/keeper/grpc_query.go

proto/cosmos/upgrade/v1beta1/upgrade.proto

Co-authored-by: Aaron Craelius <aaron@regen.network>

…into ty-8941-grpc_vm

amaury1093

lgtm

x/upgrade/client/cli/query.go

Co-authored-by: Amaury <1293565+amaurym@users.noreply.github.com>

aleem1314 · 2021-05-19T11:12:04Z

x/upgrade/client/cli/cli_test.go

@@ -0,0 +1,114 @@
+package cli_test


IntegrationTestSuite needs to be updated as per #6711

aleem1314

Preapproving

atheeshp

lgtm

* setup version map query * query methods * grpc methods * cleanup * grpc for VersionMap * swagger update * swagger sync * cleanup * reset docs * clean * grpc with field * daily * -add grpc methods -add optional field to get specific module from version map -added cli test for version map query from cli * -consolidate grpc test to one test function -return better errors in grpc methods * consolidate tests * swagger update * this breaks * Try stringer on individual msgs * change map to slice in proto * cleanup * add comments to proto fields * cleanup * regen proto files * jsoncodec * rename gRPC methods * * add fetch method for module version slice * add method to return version given a module name * remove sorting functions * lint * Update proto/cosmos/upgrade/v1beta1/query.proto Co-authored-by: Aaron Craelius <aaron@regen.network> * Update proto/cosmos/upgrade/v1beta1/upgrade.proto Co-authored-by: Aaron Craelius <aaron@regen.network> * fix up comments and regen proto/swagger * Update x/upgrade/client/cli/query.go Co-authored-by: Amaury <1293565+amaurym@users.noreply.github.com> * conform to pr 6711 * lint Co-authored-by: technicallyty <48813565+tytech3@users.noreply.github.com> Co-authored-by: Amaury M <1293565+amaurym@users.noreply.github.com> Co-authored-by: Aaron Craelius <aaron@regen.network>

technicallyty added 22 commits March 27, 2021 15:01

setup version map query

11e8a8d

Merge branch 'master' into ty-8941-grpc_vm

c66a53f

query methods

0f01311

grpc methods

c539eb5

Merge branch 'master' into ty-8941-grpc_vm

7dc8c58

cleanup

21ce91f

Merge branch 'master' into ty-8941-grpc_vm

8dd1f67

grpc for VersionMap

9c902df

swagger update

e967b8b

swagger sync

e610a69

cleanup

abaaf3f

reset docs

89f742a

clean

d5191d2

grpc with field

e462738

Merge branch 'master' into ty-8941-grpc_vm

f29fe4a

daily

5542d8b

Merge branch 'master' into ty-8941-grpc_vm

9002a9f

-add grpc methods

cca9b64

-add optional field to get specific module from version map -added cli test for version map query from cli

Merge branch 'master' into ty-8941-grpc_vm

be052ef

-consolidate grpc test to one test function

b123771

-return better errors in grpc methods

consolidate tests

7af53dc

swagger update

7123308

technicallyty requested review from aaronc, alessio and alexanderbez as code owners April 7, 2021 19:41

technicallyty requested a review from amaury1093 April 7, 2021 19:42

amaury1093 suggested changes Apr 8, 2021

View reviewed changes

alessio requested a review from fdymylja April 8, 2021 11:35

Merge branch 'master' into ty-8941-grpc_vm

05c853b

aaronc reviewed May 13, 2021

View reviewed changes

proto/cosmos/upgrade/v1beta1/upgrade.proto Outdated Show resolved Hide resolved

technicallyty and others added 10 commits May 13, 2021 12:14

Update proto/cosmos/upgrade/v1beta1/query.proto

3d0c62a

Co-authored-by: Aaron Craelius <aaron@regen.network>

Update proto/cosmos/upgrade/v1beta1/upgrade.proto

5fea65c

Co-authored-by: Aaron Craelius <aaron@regen.network>

Merge branch 'master' into ty-8941-grpc_vm

ec2a14b

Merge branch 'ty-8941-grpc_vm' of https://github.com/cosmos/cosmos-sdk …

1e85e9e

…into ty-8941-grpc_vm

fix up comments and regen proto/swagger

5ba15d5

Merge branch 'master' into ty-8941-grpc_vm

f243aa0

Merge branch 'master' into ty-8941-grpc_vm

67d5e54

Merge branch 'ty-8941-grpc_vm' of https://github.com/cosmos/cosmos-sdk …

7b66d99

…into ty-8941-grpc_vm

Merge branch 'master' into ty-8941-grpc_vm

d765070

Merge branch 'master' into ty-8941-grpc_vm

934d351

amaury1093 approved these changes May 17, 2021

View reviewed changes

x/upgrade/client/cli/query.go Outdated Show resolved Hide resolved

technicallyty and others added 2 commits May 17, 2021 08:13

Update x/upgrade/client/cli/query.go

ec7de4b

Co-authored-by: Amaury <1293565+amaurym@users.noreply.github.com>

Merge branch 'master' into ty-8941-grpc_vm

70c093e

technicallyty requested a review from aaronc May 17, 2021 15:38

technicallyty and others added 3 commits May 17, 2021 10:21

Merge branch 'master' into ty-8941-grpc_vm

9906312

Merge branch 'master' into ty-8941-grpc_vm

418a455

Merge branch 'master' into ty-8941-grpc_vm

e05c9c9

aleem1314 reviewed May 19, 2021

View reviewed changes

aleem1314 approved these changes May 19, 2021

View reviewed changes

atheeshp approved these changes May 19, 2021

View reviewed changes

technicallyty and others added 3 commits May 19, 2021 08:11

Merge branch 'master' into ty-8941-grpc_vm

c29959e

conform to pr 6711

2e399db

lint

40749ce

technicallyty added the A:automerge Automatically merge PR once all prerequisites pass. label May 19, 2021

mergify bot merged commit 53fe03e into master May 19, 2021

mergify bot deleted the ty-8941-grpc_vm branch May 19, 2021 17:04

amaury1093 mentioned this pull request Jun 22, 2021

Manual Testing for v0.43 regen-network/regen-ledger#352

Closed

56 tasks

technicallyty mentioned this pull request Jul 21, 2021

Add parameter to track credit types and associated precision regen-network/regen-ledger#424

Closed

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

x/upgrade gRPC methods for VersionMap #9073

x/upgrade gRPC methods for VersionMap #9073

technicallyty commented Apr 7, 2021 •

edited

Loading

codecov bot commented Apr 7, 2021 •

edited

Loading

amaury1093 left a comment

amaury1093 Apr 8, 2021

technicallyty Apr 9, 2021

aaronc May 7, 2021

technicallyty May 7, 2021

aaronc May 7, 2021

amaury1093 May 10, 2021

technicallyty May 11, 2021

amaury1093 left a comment

aleem1314 May 19, 2021

technicallyty May 19, 2021

aleem1314 left a comment

atheeshp left a comment

x/upgrade gRPC methods for VersionMap #9073

x/upgrade gRPC methods for VersionMap #9073

Conversation

technicallyty commented Apr 7, 2021 • edited Loading

Description

codecov bot commented Apr 7, 2021 • edited Loading

Codecov Report

amaury1093 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amaury1093 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aleem1314 left a comment

Choose a reason for hiding this comment

atheeshp left a comment

Choose a reason for hiding this comment

technicallyty commented Apr 7, 2021 •

edited

Loading

codecov bot commented Apr 7, 2021 •

edited

Loading