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

After importing a previously deleted key, be able to delete it again. #4653

Merged
merged 2 commits into from
Mar 15, 2021

Conversation

jennijuju
Copy link
Member

fixes #4621

Copy link
Contributor

@arajasek arajasek left a comment

Choose a reason for hiding this comment

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

It's probably "correcter", but a bit riskier, to instead catch the problem described in #4621 at the "import" stage, by deleting any trash-prefixed files for the newly imported private key.

It's always scary to be deleting private key files, though, so maybe not?

node/repo/fsrepo.go Outdated Show resolved Hide resolved
@jennijuju
Copy link
Member Author

It's probably "correcter", but a bit riskier, to instead catch the problem described in #4621 at the "import" stage, by deleting any trash-prefixed files for the newly imported private key.

It's always scary to be deleting private key files, though, so maybe not?

I think there is no harm to keep the trash-key there?

Copy link
Contributor

@arajasek arajasek left a comment

Choose a reason for hiding this comment

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

Approved, but someone else should weigh in on:

It's probably "correcter", but a bit riskier, to instead catch the problem described in #4621 at the "import" stage, by deleting any trash-prefixed files for the newly imported private key.
It's always scary to be deleting private key files, though, so maybe not?

node/repo/fsrepo.go Show resolved Hide resolved
@arajasek
Copy link
Contributor

arajasek commented Nov 3, 2020

Might also fix #4306?

@strahe
Copy link
Contributor

strahe commented Nov 9, 2020

When the key is imported again, perhaps the trash-key should be deleted, which does no harm, but it is also unnecessary.when trash-key and wallet-key exist at the same time, how do we understand this situation?

Copy link
Contributor

@magik6k magik6k left a comment

Choose a reason for hiding this comment

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

Ideally we wouldn't overwrite the old key, even if it's basically guaranteed to be the same data

I'd:

  • Change Put(..) to put(name, info, retries)
  • If retries > 0, append -%d to the name we use for checking
  • If it's an existing trash key, call put with retries+1

@jennijuju jennijuju requested a review from raulk as a code owner March 9, 2021 15:53
@magik6k magik6k changed the base branch from master to next March 9, 2021 15:53
Copy link
Contributor

@arajasek arajasek left a comment

Choose a reason for hiding this comment

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

LGTM

Base automatically changed from next to master March 10, 2021 16:21
@arajasek arajasek merged commit a54c6bf into master Mar 15, 2021
@arajasek arajasek deleted the 4621-delete-wallet branch March 15, 2021 00:36
@magik6k magik6k changed the title Aftering importing a previously deleted key, be able to delete it again. After importing a previously deleted key, be able to delete it again. Apr 13, 2021
@magik6k magik6k mentioned this pull request Apr 13, 2021
69 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug: Wallet doesn't let you delete, import, then delete again
4 participants