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

feat(cli): generate account key if not specified #3587

Closed
wants to merge 4 commits into from
Closed

feat(cli): generate account key if not specified #3587

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
842a465
WIP
1garo Nov 16, 2023
ff83e27
Merge branch 'development' into feat/generate-account-if-not-specified
1garo Jan 13, 2024
f983d77
Merge branch 'development' into feat/generate-account-if-not-specified
1garo Jan 16, 2024
41b76cf
Merge branch 'development' into feat/generate-account-if-not-specified
dimartiro Mar 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 9 additions & 8 deletions cmd/gossamer/commands/utils.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,6 +153,7 @@
return fmt.Errorf("error loading babe keystore: %w", err)
}

logger.Info(fmt.Sprintf("%s", ks.Gran.Type()))

Check failure on line 156 in cmd/gossamer/commands/utils.go

View workflow job for this annotation

GitHub Actions / linting 

S1025: the argument is already a string, there's no need to use fmt.Sprintf (gosimple)
err = keystore.LoadKeystore(accountKey, ks.Gran, ed25519keyRing)
if err != nil {
return fmt.Errorf("error loading grandpa keystore: %w", err)
Expand Down Expand Up @@ -322,14 +323,14 @@
func parseAccount() {
// if key is not set, check if alice, bob, or charlie are set
// return error if none are set
if key == "" {
if alice {
key = "alice"
} else if bob {
key = "bob"
} else if charlie {
key = "charlie"
}
if alice {
key = "alice"
} else if bob {
key = "bob"
} else if charlie {
key = "charlie"
} else if key == "" {
key = "default"

Check failure on line 333 in cmd/gossamer/commands/utils.go

View workflow job for this annotation

GitHub Actions / linting 

string `default` has 3 occurrences, make it a constant (goconst)
}

// if key is available, set it in the config
Expand Down
2 changes: 1 addition & 1 deletion config/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -355,7 +355,7 @@ func DefaultConfig() *Config {
Wasmer: DefaultLogLevel,
},
Account: &AccountConfig{
Key: defaultAccount,
Key: "",
Unlock: "",
},
Core: &CoreConfig{
Expand Down
16 changes: 16 additions & 0 deletions lib/keystore/helpers.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,6 +148,22 @@ func LoadKeystore(key string, keyStore TyperInserter, keyRing KeyRing) (err erro
return keyStore.Insert(keyRing.Heather())
case "ian":
return keyStore.Insert(keyRing.Ian())
case "default":
if keyStore.Type() == "ed25519" {
kp, err := ed25519.GenerateKeypair()
if err != nil {
return err
}

return keyStore.Insert(kp)
} else {
kp, err := sr25519.GenerateKeypair()
if err != nil {
return err
}

return keyStore.Insert(kp)
}
Comment on lines +151 to +166
Copy link
Contributor

@kanishkatn kanishkatn Nov 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

We currently only have the test key flow in the CLI. Check https://github.com/1garo/gossamer/blob/feat/generate-account-if-not-specified/cmd/gossamer/commands/root.go#L577

I feel we should separate out the flow for the keystore setup in the cmd package instead of this change. The rust implementation has --dev flag; maybe we could add the same!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you mean something like subkey?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe account generate already do that, I don't know if I really understood what you meant

Copy link
Contributor

Choose a reason for hiding this comment

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

I spent some more time going through the code. Thank you for your patience and time!

I believe we don't have the need to generate an account if not specified since we're still in development and only need development accounts.

If we want to support the use of auto-generated or external accounts, we'll have to

  • Add a new flag dev to enable the clear separation for the development runs
  • Clear separation for the development config
  • Store the auto generated account in disk
  • Ability to load the account from disk

I think this isn't a priority for us at the moment. I'll update the issue.

default:
return fmt.Errorf("invalid test key provided")
}
Expand Down
Loading