8000 fix(services/chain): detecting keyring by ilgooz · Pull Request #1449 · ignite/cli · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

fix(services/chain): detecting keyring #1449

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
Merged
merged 5 commits into from
Aug 12, 2021
Merged

fix(services/chain): detecting keyring #1449

merged 5 commits into from
Aug 12, 2021

Conversation

ilgooz
Copy link
Member
@ilgooz ilgooz commented Aug 10, 2021

when client.toml > keyring-backend used.

@ilgooz
fix(services/chain): detecting keyring
543a8cb 
when client.toml > keyring-backend used.
@ilgooz ilgooz linked an issue Aug 10, 2021 that may be closed by this pull request
starport c serve doesn't use init.client.keyring-backend #1446
Closed
@fadeev
Copy link
Contributor
fadeev commented Aug 10, 2021

Getting an error when trying to run serve twice with os backend.

Screenshot 2021-08-10 at 17 53 05

@lumtis
Copy link
Contributor
lumtis commented Aug 10, 2021

Getting an error when trying to run serve twice with os backend.

I may think it's a normal behavior from the os keyring that the command fails when we try to create a new account with an already existing name.
We never had this issue before because with test the file containing the key is erased at each new serve. It would require not recreating the account if the name already exist

lumtis
lumtis approved these changes Aug 10, 2021
View reviewed changes
Copy link
Contributor
@lumtis lumtis left a comment

Choose a reason for hiding this comment

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

Code looks legit to me. We just need to fix integration test.

We may also need to track #1449 (comment)
I may think the solution would be to refacto the serve workflow and not create the accounts when they already exist

fadeev
fadeev approved these changes Aug 10, 2021
View reviewed changes
Copy link
Contributor
@fadeev fadeev left a comment

Choose a reason for hiding this comment

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

👍

@lumtis
Copy link
Contributor
lumtis commented Aug 10, 2021

I just realized that os is the default keyring used if nothing is specified?

If it's actually the case, we should have

init:
  client:
    keyring-backend: "test"

by default in the generated config.yml

@fadeev
Copy link
Contributor
fadeev commented Aug 11, 2021

@lubtd I don't believe that's the case. Uses test by default.

Screenshot 2021-08-11 at 12 47 46

@fadeev fadeev self-requested a review August 11, 2021 11:14
@lumtis
Copy link
Contributor
lumtis commented Aug 11, 2021

@lubtd I don't believe that's the case. Uses test by default.

Yes it's fine. I think I used os without paying attention

@lumtis
Copy link
Contributor
lumtis commented Aug 11, 2021 •
edited
Loading

@fadeev I realized it happens with spn

Although nothing is specified in config.yml, I get the error account already exists with serve and even more surprising an empty client.toml is created

Can someone else try? https://github.com/tendermint/spn

If I set in config it works

init:
  client:
    keyring-backend: "test"

@fadeev
Copy link
Contributor
fadeev commented Aug 11, 2021

Trying it now, but I think it may be related to the version of spm.

@fadeev
Copy link
Contributor
fadeev commented Aug 11, 2021

Yeah, this change breaks compatibility with chains scaffolded with previous versions of Starport.

@fadeev
Copy link
Contributor
fadeev commented Aug 11, 2021

Updating spm in a chain scaffolded with v0.17.1 fixes it, though.

@fadeev fadeev self-requested a review August 12, 2021 06:35
@ilgooz
Copy link
Member Author
ilgooz commented Aug 12, 2021

We can force upgrade the version of spm in older chains to the latest. But I'm no sure what would be the side effects of this on user's chain. For some, it may break their chain because of possibly different sdk versions or may make their chain behave differently.

For now, I'll set the fallback keyring backend to test if no configuration specified by the user, let me know if this not OK.

fadeev
fadeev approved these changes Aug 12, 2021
View reviewed changes
Copy link
Contributor
@fadeev fadeev left a comment

Choose a reason for hiding this comment

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

Seems to be working!

@ilgooz ilgooz merged commit c78b8ef into hotfix/0.17.2 Aug 12, 2021
@ilgooz ilgooz deleted the fix/init-keyring branch August 12, 2021 11:28
ilgooz added a commit that referenced this pull request Aug 17, 2021
@ilgooz @fadeev
fix(hotfix): v0.17.2 (#1448)
d534217 
* feat(templates/cmd): upgrade spm to add 'config' cmd (#1417)

* feat(templates/cmd): upgrade spm to add 'config' cmd

to appd.

* also make $HOME/config/client.toml configurable through 'init.client'
section of chain's Starport config.yml.

* default to test

* fix

* fix

* updated changelog

* fix config application order

* fix

Co-authored-by: Denis Fadeev <denis@fadeev.org>

* fix(services/chain): detecting keyring (#1449)

* fix(services/chain): detecting keyring

when client.toml > keyring-backend used.

* fix

* fix

* default to test kb

* fix

* fix(services/chain): default home dir for app (#1457)

* fix(services/chain): support 0.43.x based chains (#1455)

* Apply suggestions from code review

* Apply suggestions from code review

* feat(pkg/chaincmd/runner): support YAML encoded CLI response (#1468)

* feat(pkg/chaincmd/runner): support YAML encoded CLI response

* Apply suggestions from code review

* fix

* fix

* fix

* fix

Co-authored-by: Denis Fadeev <denis@fadeev.org>
@snyk-bot snyk-bot mentioned this pull request Jan 17, 2022
Open
This was referenced Jan 17, 2022
Open
Open
Open
@vutting4221 vutting4221 mentioned this pull request May 4, 2022
Open
Jchicode pushed a commit to Jchicode/cli that referenced this pull request Aug 9, 2023
@ilgooz @fadeev
fix(hotfix): v0.17.2 (ignite#1448)
872f0bd 
* feat(templates/cmd): upgrade spm to add 'config' cmd (ignite#1417)

* feat(templates/cmd): upgrade spm to add 'config' cmd

to appd.

* also make $HOME/config/client.toml configurable through 'init.client'
section of chain's Starport config.yml.

* default to test

* fix

* fix

* updated changelog

* fix config application order

* fix

Co-authored-by: Denis Fadeev <denis@fadeev.org>

* fix(services/chain): detecting keyring (ignite#1449)

* fix(services/chain): detecting keyring

when client.toml > keyring-backend used.

* fix

* fix

* default to test kb

* fix

* fix(services/chain): default home dir for app (ignite#1457)

* fix(services/chain): support 0.43.x based chains (ignite#1455)

* Apply suggestions from code review

* Apply suggestions from code review

* feat(pkg/chaincmd/runner): support YAML encoded CLI response (ignite#1468)

* feat(pkg/chaincmd/runner): support YAML encoded CLI response

* Apply suggestions from code review

* fix

* fix

* fix

* fix

Co-authored-by: Denis Fadeev <denis@fadeev.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fadeev fadeev fadeev approved these changes

@Pantani Pantani Pantani approved these changes

@lumtis lumtis lumtis approved these changes

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

Successfully merging this pull request may close these issues.

starport c serve doesn't use init.client.keyring-backend
4 participants
@ilgooz @fadeev @lumtis @Pantani
0