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

fix: fix redundant priv-validator-key generation when running with kms #1086

Merged
merged 5 commits into from
Aug 24, 2023

Conversation

jaeseung-bae
Copy link
Contributor

@jaeseung-bae jaeseung-bae commented Aug 18, 2023

Description

  • Fix for redundant key generation. With running kms, generating priv-key is unnecessary.

Motivation and context

How has this been tested?

Screenshots (if appropriate):

Checklist:

  • I followed the contributing guidelines and code of conduct.
  • I have added a relevant changelog to CHANGELOG.md
  • I have added tests to cover my changes.
  • I have updated the documentation accordingly.
  • I have updated API documentation client/docs/swagger-ui/swagger.yaml

@jaeseung-bae jaeseung-bae self-assigned this Aug 18, 2023
@codecov
Copy link

codecov bot commented Aug 18, 2023

Codecov Report

Merging #1086 (7a004cc) into rc/v0.48.0-rcx (0538b80) will not change coverage.
The diff coverage is n/a.

Additional details and impacted files

Impacted file tree graph

@@               Coverage Diff               @@
##           rc/v0.48.0-rcx    #1086   +/-   ##
===============================================
  Coverage           69.70%   69.70%           
===============================================
  Files                 642      642           
  Lines               67285    67285           
===============================================
  Hits                46900    46900           
  Misses              18203    18203           
  Partials             2182     2182           

@jaeseung-bae jaeseung-bae marked this pull request as ready for review August 21, 2023 02:34
@zemyblue zemyblue added this to the v0.48.0 milestone Aug 23, 2023
@zemyblue zemyblue added the A: bug Something isn't working label Aug 23, 2023
@jaeseung-bae jaeseung-bae changed the base branch from main to rc/v0.48.0-rcx August 23, 2023 04:33
Copy link
Member

@zemyblue zemyblue left a comment

Choose a reason for hiding this comment

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

I think it's nice to add some explanation about doesn't generate a private key if some condition.

@jaeseung-bae
Copy link
Contributor Author

I think it's nice to add some explanation about doesn't generate a private key if some condition.

I named the function genPvFileOnlyWhenKmsAddressEmpty to express conditional generation of the key.
The name doesn't seem to be enough.
Do you have any idea?

@zemyblue
Copy link
Member

I think it's nice to add some explanation about doesn't generate a private key if some condition.

I named the function genPvFileOnlyWhenKmsAddressEmpty to express conditional generation of the key. The name doesn't seem to be enough. Do you have any idea?

I think the condition of which does not generate a private key seems that the config.yml set priv_validator_laddr is empty string and there is no private key. Is it right?

If then, how about adding this explanation in config.yml. Oh, it is Ostracon's area. right?

@jaeseung-bae
Copy link
Contributor Author

jaeseung-bae commented Aug 23, 2023

I think it's nice to add some explanation about doesn't generate a private key if some condition.

I named the function genPvFileOnlyWhenKmsAddressEmpty to express conditional generation of the key. The name doesn't seem to be enough. Do you have any idea?

I think the condition of which does not generate a private key seems that the config.yml set priv_validator_laddr is empty string and there is no private key. Is it right?

If then, how about adding this explanation in config.yml. Oh, it is Ostracon's area. right?

If priv_validator_laddr is empty string, key will be generated. It doesn't check the existence of the key file.
It's nice to put an explanation to config.yml.
Yes, It's ostracon's area. I'll put it in ostracon's area.

zemyblue
zemyblue previously approved these changes Aug 24, 2023
server/start_test.go Outdated Show resolved Hide resolved
server/start_test.go Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A: bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants