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: key import,export not working correctly (fix #26) #27

Merged
merged 1 commit into from
Sep 2, 2021
Merged

Problem: key import,export not working correctly (fix #26) #27

merged 1 commit into from
Sep 2, 2021

Conversation

leejw51crypto
Copy link
Contributor

@leejw51crypto leejw51crypto commented Aug 27, 2021 •
edited
Loading

Solution: make default keyring-backend as os
for consistency

without this pr, adding default is test, and others are os,
so user cannot find the key correctly without specifying keyring-backend

after applying this pr, when keyring-backend is not specified, default will be os , consistent

cronosd keys add mykey1 --algo "eth_secp256k1" --index 0 
cronosd keys list 
cronosd keys  show mykey1  
cronosd keys export mykey1   --unarmored-hex --unsafe
cronosd keys unsafe-import-eth-key my5   ....................  --home ./test
cronosd keys  list --home ./test
cronosd keys  show my5 --home ./test

@codecov
Copy link

codecov bot commented Aug 27, 2021

Codecov Report

❗ No coverage uploaded for pull request base (main@1b05e10). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff           @@
##             main     #27   +/-   ##
======================================
  Coverage        ?   3.75%           
======================================
  Files           ?      12           
  Lines           ?    1145           
  Branches        ?       0           
======================================
  Hits            ?      43           
  Misses          ?    1101           
  Partials        ?       1

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 1b05e10...48ccea4. Read the comment docs.

thomas-nguy
thomas-nguy reviewed Aug 27, 2021
View reviewed changes
cmd/cronosd/cmd/root.go
@@ -95,7 +95,7 @@ func NewRootCmd() (*cobra.Command, params.EncodingConfig) {
initRootCmd(rootCmd, encodingConfig)
overwriteFlagDefaults(rootCmd, map[string]string{
flags.FlagChainID: ChainID,
flags.FlagKeyringBackend: "test",
flags.FlagKeyringBackend: "os",
Copy link
Collaborator

Choose a reason for hiding this comment

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

is it the same for all operating system?
Do we need to overwrite this flag?

Copy link
Contributor Author

@leejw51crypto leejw51crypto Aug 27, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

i'll check again.
without this code, file is used for default

Copy link
Collaborator

Choose a reason for hiding this comment

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

we might just remove this line.

Copy link
Collaborator

@yihuang yihuang Aug 30, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

i'll check again.
without this code, file is used for default

I think the default value is os?

EDIT: it's default to os in cosmos-sdk, but default to file in ethermint commands. So change to os here seems indeed keep the code from different places consistent.

Copy link
Collaborator

Choose a reason for hiding this comment

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

evmos/ethermint#511
similar issue in ethermint side.

@yihuang
Copy link
Collaborator

yihuang commented Aug 30, 2021 •
edited
Loading

Solution: make default keyring-backend as os
for consistency

without this pr, adding default is test, and others are os,
so user cannot find the key correctly without specifying keyring-backend

The reason of setting test as default is because cronosd start command only works with test backend currently (personal rpc apis). evmos/ethermint#269

yihuang
yihuang reviewed Aug 30, 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.

I think the root cause of this issue is, in the default generated client.toml, the keyring-backend is set to os, and some commands will read the default value in the command line flag, but some commands will read the client.toml when no command line flag is passed.

cmd/cronosd/cmd/root.go
@@ -95,7 +95,7 @@ func NewRootCmd() (*cobra.Command, params.EncodingConfig) {
initRootCmd(rootCmd, encodingConfig)
overwriteFlagDefaults(rootCmd, map[string]string{
flags.FlagChainID: ChainID,
flags.FlagKeyringBackend: "test",
flags.FlagKeyringBackend: "os",
Copy link
Collaborator

Choose a reason for hiding this comment

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

we might just remove this line.

@yihuang yihuang merged commit 96d47fe into crypto-org-chain:main Sep 2, 2021
leejw51crypto added a commit to leejw51crypto/cronos that referenced this pull request Sep 8, 2021
@leejw51crypto
Problem: key import,export not working correctly (fix crypto-org-chai…
16696ff 
…n#26) (crypto-org-chain#27)

add scripts

pear

makefile

add my.nix

working

works

OK

works

work

chainmain works

tidy up

1 node
leejw51crypto added a commit to leejw51crypto/cronos that referenced this pull request Oct 21, 2021
@leejw51crypto
Problem: key import,export not working correctly (fix crypto-org-chai…
347e7b3 
…n#26) (crypto-org-chain#27)

add scripts

pear

makefile

add my.nix

working

works

OK

works

work

chainmain works

tidy up

1 node
leejw51crypto added a commit to leejw51crypto/cronos that referenced this pull request Sep 18, 2023
@leejw51crypto
Problem: key import,export not working correctly (fix crypto-org-chai…
cdefc6d 
…n#26) (crypto-org-chain#27)

add scripts

pear

makefile

add my.nix

working

works

OK

works

work

chainmain works

tidy up

1 node
leejw51crypto added a commit to leejw51crypto/cronos that referenced this pull request Sep 18, 2023
@leejw51crypto
Problem: key import,export not working correctly (fix crypto-org-chai…
f1cfc3a 
…n#26) (crypto-org-chain#27)

add scripts

pear

makefile

add my.nix

working

works

OK

works

work

chainmain works

tidy up

1 node
leejw51crypto added a commit to leejw51crypto/cronos that referenced this pull request Sep 19, 2023
@leejw51crypto
windows msys2 working
1ba12ed 
add c source

fix

ok

Problem: key import,export not working correctly (fix crypto-org-chain#26) (crypto-org-chain#27)

add scripts

pear

makefile

add my.nix

working

works

OK

works

work

chainmain works

tidy up

1 node

mac

ok

go2go2

test
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

@thomas-nguy thomas-nguy thomas-nguy approved these changes

@calvinaco calvinaco Awaiting requested review from calvinaco

@devashishdxt devashishdxt Awaiting requested review from devashishdxt

@JayT106 JayT106 Awaiting requested review from JayT106

@tomtau tomtau Awaiting requested review from tomtau

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

Successfully merging this pull request may close these issues.
3 participants
@leejw51crypto @yihuang @thomas-nguy