Add check to avoid overriding existing keys

[BlaineHeffron]

What

stellar keys generate will no longer overwrite an identity if it already exists, unless a different seed is provided.

Why

Workflows that utilize generate might unintentionally overwrite during contract development.

Close #1380

Known limitations

N/A

[leighmcculloch]

We can't merge this change at the moment, not until closer to a v22 release, because it breaks existing behavior.

[janewang]

Hi @BlaineHeffron Did we agree on this feature? Could you show me the thread of the discussion please

[willemneal]

@leighmcculloch How about a flag to prevent the overwrite? Then we can deprecate it in the next breaking release?

[BlaineHeffron]

Hi @BlaineHeffron Did we agree on this feature? Could you show me the thread of the discussion please

There isn't a discussion thread that I'm aware of, it was something we had discussed with other AhaLabs team members.

[leighmcculloch]

@leighmcculloch How about a flag to prevent the overwrite? Then we can deprecate it in the next breaking release?

It's unclear how effective an option would be. I doubt the average CLI user is going to start using a --prevent-overwrite option when generating keys, it just seems a bit esoteric.

What use case / workflow does the option serve?

[BlaineHeffron]

What use case / workflow does the option serve?

The app Loam which we are developing has a configuration option to auto generate contract wasms and install / deploy them in addition to generating and funding accounts. Currently we have a workaround where we check if the account exists before running generate. The problem with overwriting the account is someone might have some contract state tied to a specific account which would get lost if the account is overwritten.

[chadoh]

Original discussion thread in #1380 and in Slack. @janewang, @fnando, @elizabethengelman, and @willemneal were all in strong agreement with "overwrite by default is a bug" back in June.

[leighmcculloch]

...were all in strong agreement with "overwrite by default is a bug" back in June.

Yup, I think the idea of avoiding overwriting a key, which is a destructive action, is good too, but it needs to be held until the next major version because it is a breaking change. And I was otherwise only pushing back on the suggestion to add a temporary --no-overwrite since a major release is planned in the near future.

@Ifropc has labelled the issue as a breaking change meaning we'll circle back to it to the period leading up to the major release when main is open for breaking changes.

[leighmcculloch]

Couple more things I noticed that I think we should change before merging this in the next major release.

[BlaineHeffron]

Comments have been addressed. This PR now also resolves #1407

• I added an --overwrite flag to force overwrites. Otherwise it never overwrites.
• If it finds that it would overwrite but --overwrite is not set, it throws an Error.
• It no longer funds the account afterwards. This also means there is no longer a --no-fund arg.

Now I need to fix all the integration tests so that they fund the account after generate

[BlaineHeffron]

Actually, does it make sense to just add a --fund so that you could do both at once like the old functionality? (Would also make it easier for me to fix the tests ) @leighmcculloch

[BlaineHeffron]

Tests are fixed, should be ready.

[fnando]

Closing in favor of #1709