Problem: Protobuf code generation is not consistent with cosmos-sdk #851

devashishdxt · 2022-09-02T02:20:27Z

Solution: Generate proto code using buf with similar configuration as cosmos-sdk

tomtau

it seems the third party Git sub-modules are no longer needed after this (the needed protobuf definitions are fetched via buf?)? so could they be removed?

tomtau · 2022-09-02T02:26:39Z

proto/buf.yaml

@@ -0,0 +1,19 @@
+# This module represents the "proto" root.
+version: v1
+name: buf.build/crypto-org-chain/chain-main


this name/url gives 404 -- do we need to register it on buf.build?

https://docs.buf.build/bsr/usage#push-a-module

ok, for releases, we can perhaps specify an action to do it: https://docs.buf.build/ci-cd/github-actions

Created #852

codecov · 2022-09-02T02:29:40Z

Codecov Report

Merging #851 (eb72988) into master (a3ec407) will increase coverage by 0.51%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master     #851      +/-   ##
==========================================
+ Coverage   18.36%   18.88%   +0.51%     
==========================================
  Files          96       92       -4     
  Lines       12361    11896     -465     
==========================================
- Hits         2270     2246      -24     
+ Misses       9596     9168     -428     
+ Partials      495      482      -13

Flag	Coverage Δ
integration_tests	`16.57% <0.00%> (-0.06%)`	⬇️
unit_tests	`6.89% <0.00%> (-2.81%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
x/icaauth/types/query.pb.gw.go	`0.00% <0.00%> (ø)`
x/icaauth/types/tx.pb.go	`0.80% <ø> (ø)`
x/nft/types/query.pb.gw.go	`0.22% <0.00%> (+0.06%)`	⬆️
x/supply/types/query.pb.gw.go	`1.06% <0.00%> (-0.08%)`	⬇️
app/export.go	`0.00% <0.00%> (ø)`
app/test_helpers.go	`0.00% <0.00%> (ø)`
x/icaauth/types/params.pb.go	`1.20% <0.00%> (ø)`
app/docs/statik/statik.go
x/nft/client/rest/rest.go
x/nft/client/rest/tx.go
... and 11 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

devashishdxt · 2022-09-02T02:31:59Z

it seems the third party Git sub-modules are no longer needed after this (the needed protobuf definitions are fetched via buf?)? so could they be removed?

They're still needed for swagger documentation.

yihuang · 2022-09-02T02:49:36Z

Shall we update the config.json too? similar to https://github.com/crypto-org-chain/cronos/pull/677/files#diff-b8ef6a5786bfd5df3147645f82844be943e1fe6d68de3b6b4681a941e077b8d7
The legacy APIs are removed.

sonatype-lift · 2022-09-02T02:51:22Z

scripts/protocgen.sh

-    --grpc-gateway_out=logtostderr=true,allow_colon_final_segments=true:. \
-  $(find "${dir}" -maxdepth 1 -name '*.proto')
-
+  for file in $(find "${dir}" -maxdepth 1 -name '*.proto'); do


SC2044: For loops over find output are fragile. Use find -exec or a while read loop.

ℹ️ Learn about @sonatype-lift commands

You can reply with the following commands. For example, reply with @sonatype-lift ignoreall to leave out all findings.

Command Usage

@sonatype-lift ignore Leave out the above finding from this PR

@sonatype-lift ignoreall Leave out all the existing findings from this PR

@sonatype-lift exclude <file|issue|path|tool> Exclude specified file|issue|path|tool from Lift findings by updating your config.toml file

Note: When talking to LiftBot, you need to refresh the page to see its response.
_{Click here to add LiftBot to another repo.}

Was this a good recommendation?
[ 🙁 Not relevant ] - [ 😕 Won't fix ] - [ 😑 Not critical, will fix ] - [ 🙂 Critical, will fix ] - [ 😊 Critical, fixing now ]

sonatype-lift · 2022-09-02T02:51:23Z

scripts/protocgen.sh

-  $(find "${dir}" -maxdepth 1 -name '*.proto')
-
+  for file in $(find "${dir}" -maxdepth 1 -name '*.proto'); do
+    if grep "option go_package" $file &> /dev/null ; then


💬 3 similar findings have been found in this PR

SC2086: Double quote to prevent globbing and word splitting.

🔎 Expand here to view all instances of this finding

File Path Line Number

scripts/protocgen.sh 28

scripts/protoc-swagger-gen.sh 13

scripts/protoc-swagger-gen.sh 14

Visit the Lift Web Console to find more details in your report.

ℹ️ Learn about @sonatype-lift commands

You can reply with the following commands. For example, reply with @sonatype-lift ignoreall to leave out all findings.

Command Usage

@sonatype-lift ignore Leave out the above finding from this PR

@sonatype-lift ignoreall Leave out all the existing findings from this PR

@sonatype-lift exclude <file|issue|path|tool> Exclude specified file|issue|path|tool from Lift findings by updating your config.toml file

Note: When talking to LiftBot, you need to refresh the page to see its response.
_{Click here to add LiftBot to another repo.}

Was this a good recommendation?
[ 🙁 Not relevant ] - [ 😕 Won't fix ] - [ 😑 Not critical, will fix ] - [ 🙂 Critical, will fix ] - [ 😊 Critical, fixing now ]

devashishdxt · 2022-09-02T03:16:24Z

Shall we update the config.json too? similar to https://github.com/crypto-org-chain/cronos/pull/677/files#diff-b8ef6a5786bfd5df3147645f82844be943e1fe6d68de3b6b4681a941e077b8d7 The legacy APIs are removed.

Done

yihuang · 2022-09-02T03:24:40Z

Shall we update the config.json too? similar to https://github.com/crypto-org-chain/cronos/pull/677/files#diff-b8ef6a5786bfd5df3147645f82844be943e1fe6d68de3b6b4681a941e077b8d7 The legacy APIs are removed.

Done

you need to regenerate the swagger to make it up to date.

devashishdxt · 2022-09-02T05:00:53Z

Shall we update the config.json too? similar to https://github.com/crypto-org-chain/cronos/pull/677/files#diff-b8ef6a5786bfd5df3147645f82844be943e1fe6d68de3b6b4681a941e077b8d7 The legacy APIs are removed.

Done

you need to regenerate the swagger to make it up to date.

done

yihuang · 2022-09-02T05:02:14Z

Shall we update the config.json too? similar to https://github.com/crypto-org-chain/cronos/pull/677/files#diff-b8ef6a5786bfd5df3147645f82844be943e1fe6d68de3b6b4681a941e077b8d7 The legacy APIs are removed.

Done

you need to regenerate the swagger to make it up to date.

done

and statik needs to be updated too ;D

devashishdxt · 2022-09-02T05:08:32Z

and statik needs to be updated too ;D

Done

yihuang

LGTM

…rypto-org-chain#851) * Problem: Protobuf code generation is not consistent with cosmos-sdk Solution: Generate proto code using buf with similar configuration as cosmos-sdk * update config.json * Regenerate swagger.yaml * Update statik.go * Update gomod2nix.toml * Generate gomod2nix.toml * update statik.go

…851) * Problem: Protobuf code generation is not consistent with cosmos-sdk Solution: Generate proto code using buf with similar configuration as cosmos-sdk * update config.json * Regenerate swagger.yaml * Update statik.go * Update gomod2nix.toml * Generate gomod2nix.toml * update statik.go

Problem: Protobuf code generation is not consistent with cosmos-sdk

b64e360

Solution: Generate proto code using buf with similar configuration as cosmos-sdk

devashishdxt requested a review from a team as a code owner September 2, 2022 02:20

tomtau approved these changes Sep 2, 2022

View reviewed changes

sonatype-lift bot reviewed Sep 2, 2022

View reviewed changes

update config.json

fd5cb2c

devashishdxt requested a review from yihuang September 2, 2022 03:16

Regenerate swagger.yaml

6527941

Update statik.go

f9395c7

devashishdxt added 3 commits September 2, 2022 13:21

Update gomod2nix.toml

3c48cdf

Merge branch 'master' into proto-gen-fix

c3d6ee3

Generate gomod2nix.toml

6e7d465

yihuang approved these changes Sep 5, 2022

View reviewed changes

update statik.go

eb72988

devashishdxt merged commit 83212f4 into crypto-org-chain:master Sep 5, 2022

devashishdxt deleted the proto-gen-fix branch September 5, 2022 03:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Problem: Protobuf code generation is not consistent with cosmos-sdk #851

Problem: Protobuf code generation is not consistent with cosmos-sdk #851

devashishdxt commented Sep 2, 2022

tomtau left a comment

tomtau Sep 2, 2022

devashishdxt Sep 2, 2022

tomtau Sep 2, 2022

devashishdxt Sep 2, 2022

codecov bot commented Sep 2, 2022 •

edited

Loading

devashishdxt commented Sep 2, 2022

yihuang commented Sep 2, 2022 •

edited

Loading

sonatype-lift bot Sep 2, 2022

sonatype-lift bot Sep 2, 2022

devashishdxt commented Sep 2, 2022

yihuang commented Sep 2, 2022

devashishdxt commented Sep 2, 2022

yihuang commented Sep 2, 2022

devashishdxt commented Sep 2, 2022

yihuang left a comment

Command	Usage
`@sonatype-lift ignore`	Leave out the above finding from this PR
`@sonatype-lift ignoreall`	Leave out all the existing findings from this PR
`@sonatype-lift exclude <file\|issue\|path\|tool>`	Exclude specified `file\|issue\|path\|tool` from Lift findings by updating your config.toml file

File Path	Line Number
scripts/protocgen.sh	28
scripts/protoc-swagger-gen.sh	13
scripts/protoc-swagger-gen.sh	14

Problem: Protobuf code generation is not consistent with cosmos-sdk #851

Problem: Protobuf code generation is not consistent with cosmos-sdk #851

Conversation

devashishdxt commented Sep 2, 2022

tomtau left a comment

Choose a reason for hiding this comment

tomtau Sep 2, 2022

Choose a reason for hiding this comment

devashishdxt Sep 2, 2022

Choose a reason for hiding this comment

tomtau Sep 2, 2022

Choose a reason for hiding this comment

devashishdxt Sep 2, 2022

Choose a reason for hiding this comment

codecov bot commented Sep 2, 2022 • edited Loading

Codecov Report

devashishdxt commented Sep 2, 2022

yihuang commented Sep 2, 2022 • edited Loading

sonatype-lift bot Sep 2, 2022

Choose a reason for hiding this comment

sonatype-lift bot Sep 2, 2022

Choose a reason for hiding this comment

devashishdxt commented Sep 2, 2022

yihuang commented Sep 2, 2022

devashishdxt commented Sep 2, 2022

yihuang commented Sep 2, 2022

devashishdxt commented Sep 2, 2022

yihuang left a comment

Choose a reason for hiding this comment

codecov bot commented Sep 2, 2022 •

edited

Loading

yihuang commented Sep 2, 2022 •

edited

Loading