-
-
Notifications
You must be signed in to change notification settings - Fork 169
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
feat: add ChainController #4227
Conversation
👍 Dependency issues cleared. Learn more about Socket for GitHub ↗︎ This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored. |
8fd62cf
to
395a3c3
Compare
ea51726
to
6165cd2
Compare
…ontroller instance
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello! I am not sure whether more work was planned for this PR, but I made some suggestions based on how we've written controllers and tests for controllers in other places. One question I have is whether ChainController needs to be a controller? I didn't notice any state being set, but perhaps that was just left out for now.
This matches better our coding style here: https://github.com/MetaMask/contributor-docs/blob/main/docs/unit-testing.md#-highlight-the-exercise-phase Co-authored-by: Elliot Winkler <elliot.winkler@gmail.com>
I've added the |
Co-authored-by: Gustavo Antunes <17601467+gantunesr@users.noreply.github.com>
New dependencies detected. Learn more about Socket for GitHub ↗︎
|
Removing the |
@metamaskbot publish-preview |
Preview builds have been published. See these instructions for more information about preview builds. Expand for full list of packages and versions.
|
2958fdb
to
0b1caeb
Compare
0b1caeb
to
61ff417
Compare
@ccharly Thank you for the changes! I agree it looks a lot better. Nothing jumps out at me, so feel free to get approval from your team and continue on with this PR :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Just one minor comment
Explanation
This new controller will be responsible of exposing Snaps that implement the newly defined Chain API.
This controller also implements the chain API itself.
Reference: https://github.com/MetaMask/chain-api/
Changelog
@metamask/chain-controller
ChainController
: exposes/interacts with chain API providersChecklist