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

[keyring-controller] Remove #getMemState returns #4199

Merged
merged 2 commits into from
Apr 24, 2024

Conversation

mikesposito
Copy link
Member

@mikesposito mikesposito commented Apr 22, 2024

Explanation

This PR is an intermediate refactor needed for #4192.

Since operations are eventually rolled back, the internal method will not be able to return the last controller state, risking returning a stale one. Moreover, function returns make more sense now.

This PR needs these changes to be merged first:

References

@metamask/keyring-controller

  • BREAKING: Change various KeyringController methods so they no longer return the controller state
    • Changed addNewAccount return type to Promise<string>
    • Changed addNewAccountWithoutUpdate return type to Promise<string>
    • Changed createNewVaultAndKeychain return type to Promise<void>
    • Changed createNewVaultAndRestore return type to Promise<void>
    • Changed importAccountWithStrategy return type to Promise<string>
    • Changed removeAccount return type to Promise<void>
    • Changed setLocked return type to Promise<void>
    • Changed submitEncryptionKey return type to Promise<void>
    • Changed submitPassword return type to Promise<void>

Checklist

  • I've updated the test suite for new or updated code as appropriate
  • I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate
  • I've highlighted breaking changes using the "BREAKING" category above as appropriate

@mikesposito mikesposito changed the title [keyring-controller]: Remove #getMemState returns [keyring-controller] Remove #getMemState returns Apr 22, 2024
@mikesposito mikesposito marked this pull request as ready for review April 22, 2024 09:40
@mikesposito mikesposito requested a review from a team as a code owner April 22, 2024 09:40
@mikesposito mikesposito force-pushed the feat/keyring-controller-lock-v2 branch from 9780406 to b6875dd Compare April 23, 2024 14:43
Copy link
Contributor

@mcmire mcmire left a comment

Choose a reason for hiding this comment

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

I suspect this PR may need to be rebased, but regardless, this PR makes sense.

@mikesposito mikesposito force-pushed the refactor/remove-get-mem-state branch from 97756af to 44de690 Compare April 24, 2024 11:41
@mikesposito mikesposito force-pushed the refactor/remove-get-mem-state branch from 55cc7c4 to dc3165d Compare April 24, 2024 12:19
@mikesposito mikesposito force-pushed the feat/keyring-controller-lock-v2 branch from 14ab92e to 952aa96 Compare April 24, 2024 14:12
Base automatically changed from feat/keyring-controller-lock-v2 to main April 24, 2024 14:24
mcmire
mcmire previously approved these changes Apr 24, 2024
Copy link
Contributor

@mcmire mcmire left a comment

Choose a reason for hiding this comment

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

One nit, but looks good regardless (we can always tweak the changelog later).

packages/keyring-controller/CHANGELOG.md Outdated Show resolved Hide resolved
Co-authored-by: Elliot Winkler <elliot.winkler@gmail.com>
@mikesposito mikesposito merged commit 59b13a5 into main Apr 24, 2024
139 checks passed
@mikesposito mikesposito deleted the refactor/remove-get-mem-state branch April 24, 2024 17:13
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.

None yet

2 participants