Dispose or destroy keyrings on reference drop#233
Conversation
Gudahtt
left a comment
There was a problem hiding this comment.
Looks great! Left a few comments/suggestions
src/KeyringController.ts
Outdated
| } | ||
|
|
||
| if ('destroy' in keyring && typeof keyring.destroy === 'function') { | ||
| keyring.destroy(); |
There was a problem hiding this comment.
Between the two names, I'm slightly more inclined to use destroy because we already use this method name for some controllers and middleware.
Perhaps we could align on this name? Not a blocker, but renaming the method on Trezor might be fairly easy (that package had a recent release)
There was a problem hiding this comment.
Are you suggesting removing the dispose call, then changing the method name on Trezor Keyring after this PR has been merged, or should we do it before this PR?
There was a problem hiding this comment.
Either before or after; as long as that change gets included in core and in the clients prior to this one.
But this was a non-blocking suggestion, so whatever you'd prefer to do here is fine
There was a problem hiding this comment.
@Gudahtt I created this issue to track this work
Description
This PR calls the optional methods
destroyanddisposeon keyrings that support them (Trezor and Ledger) whenever the reference to the keyring is dropped: when locked and when the last account from a keyring is removed.To avoid introducing one of the two (Trezor or Ledger) keyrings just to be used on tests, I extended the HDKeyring prototype to also present a
destroyand adisposemethod, in order to be spied on.Changes
Keyring.disposeorKeyring.destroymethods are called on any keyring reference dropReferences
Checklist