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

Remove old disposable keys from the wallet #3350

Merged
merged 6 commits into from
Jun 6, 2024

Conversation

sug0
Copy link
Contributor

@sug0 sug0 commented Jun 3, 2024

Describe your changes

Closes #3239

Garbage collect disposable signing keys.

Indicate on which release or other PRs this topic is based on

v0.38.1

Checklist before merging to draft

  • I have added a changelog
  • Git history is in acceptable state

@sug0 sug0 added enhancement New feature or request client SDK labels Jun 3, 2024
@sug0 sug0 changed the title Tiago/diposable keys cleanup Remove old disposable keys from the wallet Jun 3, 2024
@sug0 sug0 added wallet and removed client labels Jun 3, 2024
@sug0 sug0 marked this pull request as ready for review June 3, 2024 10:50
@sug0 sug0 requested review from grarco and murisi June 3, 2024 10:50
Copy link

codecov bot commented Jun 3, 2024

Codecov Report

Attention: Patch coverage is 97.52066% with 3 lines in your changes missing coverage. Please review.

Project coverage is 54.20%. Comparing base (883bd0f) to head (2e52673).
Report is 2 commits behind head on main.

Files Patch % Lines
crates/sdk/src/wallet/mod.rs 97.45% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3350      +/-   ##
==========================================
+ Coverage   54.05%   54.20%   +0.14%     
==========================================
  Files         315      315              
  Lines      106296   106405     +109     
==========================================
+ Hits        57461    57679     +218     
+ Misses      48835    48726     -109     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@sug0 sug0 requested a review from tzemanovic June 3, 2024 16:36
@tzemanovic tzemanovic mentioned this pull request Jun 3, 2024
Copy link
Contributor

@grarco grarco left a comment

Choose a reason for hiding this comment

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

I still believe the strongest solution would be to wait for a confirmation from the client and one block on top (in case of a rollback) just to have absolute certainty that the transaction has been executed and we don't need the address anymore. Anyway, this solution is already a nice improvement, thanks!

@brentstone
Copy link
Contributor

should we implement @grarco's suggestion or merge as is? @sug0

@sug0
Copy link
Contributor Author

sug0 commented Jun 4, 2024

@brentstone I think we should increase the key lifetime a bit; let them live for, let's say 1 week, before deleting them

brentstone added a commit that referenced this pull request Jun 5, 2024
* tiago/diposable-keys-cleanup:
  Changelog for #3350
  Make `test_disposable_keys_are_garbage_collected` more robust
  Test that non-disposable keys are not garbage collected
  Automatically clear disposable keys from the wallet
  Convert a datetime to a unix timestamp
  Change naming scheme of disposable signing keys
brentstone added a commit that referenced this pull request Jun 5, 2024
* origin/tiago/diposable-keys-cleanup:
  Changelog for #3350
  Make `test_disposable_keys_are_garbage_collected` more robust
  Test that non-disposable keys are not garbage collected
  Automatically clear disposable keys from the wallet
  Convert a datetime to a unix timestamp
  Change naming scheme of disposable signing keys
@brentstone brentstone merged commit a150303 into main Jun 6, 2024
19 checks passed
@brentstone brentstone deleted the tiago/diposable-keys-cleanup branch June 6, 2024 07:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Garbage collect disposable keys from the wallet
4 participants