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.

Sign up for GitHub

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

Jump to bottom

Problem: default gRPC init configuration bound to 0.0.0.0 #665

Merged
merged 5 commits into from
Oct 25, 2021
Merged

Problem: default gRPC init configuration bound to 0.0.0.0 #665

merged 5 commits into from
Oct 25, 2021

Conversation

damoncro
Copy link
Contributor

👮🏻👮🏻👮🏻 !!!! REFERENCE THE PROBLEM YOUR ARE SOLVING IN THE PR TITLE AND DESCRIBE YOUR SOLUTION HERE !!!! DO NOT FORGET !!!! 👮🏻👮🏻👮🏻
Close #586

PR Checklist:

  • Have you read the CONTRIBUTING.md?
  • Does your PR follow the C4 patch requirements?
  • Have you rebased your work on top of the latest master?
  • Have you checked your code compiles? (make)
  • Have you included tests for any non-trivial functionality?
  • Have you checked your code passes the unit tests? (make test)
  • Have you checked your code formatting is correct? (go fmt)
  • Have you checked your basic code style is fine? (golangci-lint run)
  • If you added any dependencies, have you checked they do not contain any known vulnerabilities? (go list -json -m all | nancy sleuth)
  • If your changes affect the client infrastructure, have you run the integration test?
  • If your changes affect public APIs, does your PR follow the C4 evolution of public contracts?
  • If your code changes public APIs, have you incremented the crate version numbers and documented your changes in the CHANGELOG.md?
  • If you are contributing for the first time, please read the agreement in CONTRIBUTING.md now and add a comment to this pull request stating that your PR is in accordance with the Developer's Certificate of Origin.

Thank you for your code, it's appreciated! :)

@codecov
Copy link

codecov bot commented Oct 20, 2021 •
edited
Loading

Codecov Report

Merging #665 (526f1db) into master (425cd6d) will increase coverage by 3.38%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #665      +/-   ##
==========================================
+ Coverage   18.96%   22.34%   +3.38%     
==========================================
  Files          68       69       +1     
  Lines        8021     8172     +151     
==========================================
+ Hits         1521     1826     +305     
+ Misses       5996     5803     -193     
- Partials      504      543      +39
Flag Coverage Δ
integration_tests 19.04% <100.00%> (+0.08%) ⬆️
unit_tests 9.73% <ø> (?)

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

Impacted Files Coverage Δ
cmd/chain-maind/app/app.go 54.76% <100.00%> (+1.79%) ⬆️
app/app.go 12.82% <0.00%> (-77.18%) ⬇️
config/config.go 66.66% <0.00%> (ø)
x/nft/keeper/querier.go 8.67% <0.00%> (+1.15%) ⬆️
x/nft/keeper/invariants.go 3.92% <0.00%> (+3.92%) ⬆️
x/nft/types/validation.go 50.00% <0.00%> (+14.28%) ⬆️
x/nft/keeper/keeper.go 83.33% <0.00%> (+21.21%) ⬆️
x/nft/client/cli/tx.go 76.04% <0.00%> (+32.29%) ⬆️
x/nft/keeper/denom.go 84.61% <0.00%> (+33.33%) ⬆️
x/nft/client/cli/query.go 68.71% <0.00%> (+35.19%) ⬆️
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 425cd6d...526f1db. Read the comment docs.

tomtau
tomtau suggested changes Oct 21, 2021
View reviewed changes
Copy link
Contributor

@tomtau tomtau left a comment

Choose a reason for hiding this comment

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

congrats on your first PR! 🎆

I think this doesn't solve the problem yet -- when I run chain-maind init, it still has 0.0.0.0.
It'd probably need fixing here for the init command: https://github.com/crypto-org-chain/chain-main/blob/master/cmd/chain-maind/app/app.go#L92

@damoncro
Copy link
Contributor Author

Let me check~

@damoncro
Add customAppConfig support
ae00c5b 
Config the grpc.address and grpc-web.address to localhost.
@damoncro
Copy link
Contributor Author

damoncro commented Oct 22, 2021 •
edited
Loading

I guess it should now support custom configuration. Please check

https://github.com/cosmos/cosmos-sdk/blob/master/simapp/simd/cmd/root.go#L86

Test result:

--- #<buffer damonchen/app.toml>
+++ #<buffer config/app.toml>
@@ -158,7 +158,7 @@
 enable = true
 
 # Address defines the gRPC server address to bind to.
-address = "0.0.0.0:9090"
+address = "127.0.0.1:9090"
 
 ###############################################################################
 ###                        gRPC Web Configuration                           ###
@@ -171,7 +171,7 @@
 enable = true
 
 # Address defines the gRPC-web server address to bind to.
-address = "0.0.0.0:9091"
+address = "127.0.0.1:9091"
 
 # EnableUnsafeCORS defines if CORS should be enabled (unsafe - use it at your own risk).
 enable-unsafe-cors = false

yihuang
yihuang reviewed Oct 22, 2021
View reviewed changes
app/docs/swagger-ui/swagger.yaml Outdated
@@ -73,7 +73,7 @@ paths:
example: 'on'
rpc_address:
type: string
example: 'tcp://0.0.0.0:26657'
example: 'tcp://127.0.0.1:26657'
Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess no need to change the rpc listen address, maybe undo this.

yihuang
yihuang requested changes Oct 22, 2021
View reviewed changes
Copy link
Collaborator

@yihuang yihuang left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Tried locally, it works 👍
I think you should undo the RPC listen-address change.

And fix the golang lint error.

@damoncro
Copy link
Contributor Author

damoncro commented Oct 22, 2021 •
edited
Loading

I did not have the lint error, what error you mentioned?

Do you mean in CI bot?

  Error: File is not `gofmt`-ed with `-s` (gofmt)
  
  Error: issues found

Let me check.

@damoncro
Copy link
Contributor Author

Fixed @yihuang

yihuang
yihuang approved these changes Oct 22, 2021
View reviewed changes
Copy link
Collaborator

@yihuang yihuang left a comment

Choose a reason for hiding this comment

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

LGTM

@damoncro damoncro requested a review from tomtau October 25, 2021 02:09
tomtau
tomtau approved these changes Oct 25, 2021
View reviewed changes
Copy link
Contributor

@tomtau tomtau left a comment

Choose a reason for hiding this comment

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

looks all right -- may be good to add a changelog entry, but can fix that in a different PR

@tomtau tomtau merged commit 94d7567 into crypto-org-chain:master Oct 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yihuang yihuang yihuang approved these changes

@tomtau tomtau tomtau approved these changes

@calvinaco calvinaco Awaiting requested review from calvinaco

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Problem: default gRPC init configuration bound to 0.0.0.0
3 participants
@damoncro @yihuang @tomtau