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

keys generate: don't overwrite existing account #1380

Closed
chadoh opened this issue Jun 17, 2024 · 6 comments · Fixed by #1709
Closed

keys generate: don't overwrite existing account #1380

chadoh opened this issue Jun 17, 2024 · 6 comments · Fixed by #1709
Assignees

Comments

@chadoh
Copy link
Contributor

chadoh commented Jun 17, 2024

What problem does your feature solve?

Right now if you have an existing account named alice and you run keys generate again, it will generate a new account, overwrite the old alice.toml file with the new seed phrase, and fund the new account.

$ cat .soroban/identity/alice.toml
───────┬────────────────────────────────────
       │ File: .soroban/identity/alice.toml
───────┼────────────────────────────────────
   1   │ seed_phrase = "zoo solid..."
───────┴────────────────────────────────────
$ soroban keys generate alice --network local
$ cat .soroban/identity/alice.toml
───────┬────────────────────────────────────
       │ File: .soroban/identity/alice.toml
───────┼────────────────────────────────────
   1   │ seed_phrase = "camp endorse..."
───────┴────────────────────────────────────

What would you like to see?

By default

Here's an example of what this could look like:

$ soroban keys generate alice --network local
alice already exists!
    seed phrase found at: /path/to/.soroban/identity/alice.toml
    current XLM balance: 231
Balance >100 XLM, nothing to do. 
To generate new keys for alice, you can `keys rm alice` first.

Or, if the account doesn't have sufficient balance:

$ soroban keys generate alice --network local
alice already exists!
    seed phrase found at: /path/to/.soroban/identity/alice.toml
    account XLM balance: 9
Topping off... Balance is now 1009 XLM.
To generate new keys for alice, you can `keys rm alice` first.

What alternatives are there?

With #1389, we're considering adding a --minimum-balance option. I think for the sake of clarity & simplicity, we do NOT add that to keys generate. But should we mention the existence of keys fund in the output here?

@leighmcculloch
Copy link
Member

Friendbot doesn't support topping off. Friendbot only creates new accounts and doesn't currently have the ability to send funds to existing accounts.

Instead of adding force, we can error and require the explicit removal instead of adding new options. So the way to do a force is just a remove and generate. Wdyt?

@willemneal
Copy link
Member

You can top off with friendbot. Generate a temporary account then transfer all funds to existing account. But after some reflection, since we have a keys rm it's not that hard for users to delete without needing to add another option.

@chadoh chadoh changed the title keys generate: don't overwrite existing account without --force keys generate: don't overwrite existing account Jun 18, 2024
@chadoh
Copy link
Contributor Author

chadoh commented Jun 18, 2024

I like it, @leighmcculloch @willemneal. I've added this:

And I updated the description here to mention it and to get rid of the --force idea.

@chadoh
Copy link
Contributor Author

chadoh commented Jun 18, 2024

But there's a new question: With #1389, we're considering adding a --minimum-balance option. I think for the sake of clarity & simplicity, we do NOT add that to keys generate. But should we mention the existence of keys fund --minimum-balance in the output of keys generate?

@fnando
Copy link
Member

fnando commented Jul 2, 2024

the way i usually handle topoffs is by creating a new account + funding with friendbot, then merging the account. That means the existing account will be refilled with 10K XLM every time, which is good enough. I personally wouldn't add a --minimum-balance, and instead would refill the account every time if keys fund is called on a existing account.

@leighmcculloch
Copy link
Member

But should we mention the existence of keys fund --minimum-balance in the output of keys generate?

Wouldn't hurt the UX to output to stderr a message saying that the account has been funded, and then how to refund it using #1389.

Today the command and its output looks like this:

$ stellar keys generate me --network testnet

We could change it to:

$ stellar keys generate me --network testnet
✅ Generated new key for 'me'
ℹ️ Public key: GDVLD7NRK4FSZ2QXPR4Z5QOM4U2KLBUP5J5RB7DDNBSHR6RASKR6APGE
ℹ️ Secret key: hidden (use 'stellar keys show me' to view)
🌎 Funding account with public key as address on testnet...
✅ Funded (use 'stellar keys fund me' to fund again)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
5 participants