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

feat: add flag for the private key selection #73

Merged
merged 2 commits into from
Feb 10, 2021
Merged

feat: add flag for the private key selection #73

merged 2 commits into from
Feb 10, 2021

Conversation

Kynea0b
Copy link
Contributor

@Kynea0b Kynea0b commented Feb 9, 2021

Closes: #XXX

Description

This is an omission of correction of this PR.
#43

Motivation and context

Same as this PR #43.

How has this been tested?

Same as this PR #43.

Ref:

Finschia/ostracon#125
line/link#1111
line/link#1133

Checklist:

  • I followed the contributing guidelines.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.

Copy link

@egonspace egonspace left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@wetcod wetcod left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -5,6 +5,7 @@ package server
import (
"fmt"

"github.com/cosmos/cosmos-sdk/client/flags"
Copy link
Contributor

Choose a reason for hiding this comment

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

nit but I think we need to move this line to the below other cosmos-sdk imports.

I think it's a kind of pain point that doesn't work w/ auto formatting but we need to follow up the convention.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, that's right. I think so, too. I will correct this! Thank you!

Copy link
Member

@tnasu tnasu left a comment

Choose a reason for hiding this comment

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

LGTM

But I think we should re-examine where we are using GenFilePV.
(I mean that it wasn't enough to just use GenFilePV through LoadOrGenFilePV and ResetAll.)

  • tendermint/privval/file.go:LoadOrGenFilePV
  • tendermint/cmd/tendermint/commands/reset_prev_validator.go:resetFilePV

I've found the others candidates the followings:

  • tendermint/cmd/tendermint/commands/gen_validator.go:genValidator
  • tendermint/cmd/tendermint/commands/init.go:initFilesWithConfig
  • tendermint/consensus/common_test.go:randConsensusNetWithPeers

@Kynea0b
Copy link
Contributor Author

Kynea0b commented Feb 10, 2021

Thank you. I understand.

@Kynea0b Kynea0b merged commit 5c778f6 into Finschia:v0.38.5-0.1.0-linking_link_with_line_consensus Feb 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants