Skip to content
This repository has been archived by the owner on Oct 7, 2024. It is now read-only.

feat: remove KeyringSnapControllerClient to fix dependency problems #241

Merged
merged 1 commit into from
Jan 19, 2024

Conversation

danroc
Copy link
Contributor

@danroc danroc commented Jan 18, 2024

Description

Summary: This PR addresses the circular dependency issues caused by the KeyringSnapControllerClient class.

The KeyringSnapControllerClient class currently relies on SnapController. This relationship introduces an extensive range of dependencies into the keyring-api. Consequently, this has led to the emergence of circular dependencies within other packages that also depend on keyring-api.

To mitigate this issue, we propose the removal of the KeyringSnapControllerClient class from the keyring-api. It's important to note that this class is exclusively utilized by the eth-snap-keyring package, an internal component of MetaMask.

@danroc danroc requested a review from a team as a code owner January 18, 2024 23:09
The `KeyringSnapControllerClient` class depends on the `SnapController`
which adds a lot of dependencies to the `keyring-api`, causing circular
dependency problems in other packages that depend on the `keyring-api`.

The `KeyringSnapControllerClient` is currently used only by the
`eth-snap-keyring` package, which is internal to MetaMask, so this
change shouldn't break any other dependent code.
Copy link

Removed dependencies detected. Learn more about Socket for GitHub ↗︎

🚮 Removed packages: @metamask/[email protected]

danroc added a commit to MetaMask/eth-snap-keyring that referenced this pull request Jan 19, 2024
This commit add the `KeyringSnapControllerClient` to this package since
it's being removed from the `keyring-api`.

See: <MetaMask/keyring-api#241>
danroc added a commit to MetaMask/eth-snap-keyring that referenced this pull request Jan 19, 2024
This commit add the `KeyringSnapControllerClient` to this package since
it's being removed from the `keyring-api`.

See: <MetaMask/keyring-api#241>
Copy link
Member

@mikesposito mikesposito left a comment

Choose a reason for hiding this comment

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

LGTM!

@AlexJupiter AlexJupiter added the team-accounts This should be handled by the Accounts Team label Jan 19, 2024
github-merge-queue bot pushed a commit to MetaMask/eth-snap-keyring that referenced this pull request Jan 19, 2024
This commit add the `KeyringSnapControllerClient` to this package since
it's being removed from the `keyring-api`.

See: <MetaMask/keyring-api#241>
@danroc danroc self-assigned this Jan 19, 2024
@danroc danroc added the type-enhancement New feature or request label Jan 19, 2024
@danroc danroc added this pull request to the merge queue Jan 19, 2024
Merged via the queue into main with commit 660e25f Jan 19, 2024
17 checks passed
@danroc danroc deleted the feature/remove-snap-client branch January 19, 2024 14:04
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
team-accounts This should be handled by the Accounts Team type-enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants