-
-
Notifications
You must be signed in to change notification settings - Fork 201
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 mm portfolio api - token search controller #5142
base: main
Are you sure you want to change the base?
Conversation
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 see that you may have used create-package
to create this controller — great! I wanted to let you know that when it comes to controllers in particular, there is a standard way that we have been trying to build them. We have some example controllers that you can take a look at to understand the general patterns: https://github.com/MetaMask/core/tree/main/examples/example-controllers. We also have some guidelines here as well: https://github.com/MetaMask/core/blob/main/docs/writing-controllers.md.
The "GasPricesController" in the examples might be a good controller to copy. In addition I think it would be good if we could encapsulate the network call in a service class. This is a pattern that we don't have enough documentation on, unfortunately, but you can see a good example in remote-feature-flag-controller
here: https://github.com/MetaMask/core/tree/main/packages/remote-feature-flag-controller/src/client-config-api-service. Essentially, this pattern allows us to retry failed requests easily and gracefully handle when the API goes down so we don't hammer the API when we don't need to.
packages/token-search-discovery-controller/src/token-search-discovery-controller.ts
Outdated
Show resolved
Hide resolved
packages/token-search-discovery-controller/src/token-search-discovery-controller.ts
Outdated
Show resolved
Hide resolved
packages/token-search-discovery-controller/src/token-search-discovery-controller.ts
Outdated
Show resolved
Hide resolved
packages/token-search-discovery-controller/src/token-search-discovery-controller.ts
Outdated
Show resolved
Hide resolved
packages/token-search-discovery-controller/src/token-search-discovery-controller.ts
Outdated
Show resolved
Hide resolved
packages/token-search-discovery-controller/src/token-search-discovery-controller.ts
Outdated
Show resolved
Hide resolved
packages/token-search-discovery-controller/src/token-search-discovery-controller.test.ts
Show resolved
Hide resolved
packages/token-search-discovery-controller/src/token-search-discovery-controller.test.ts
Outdated
Show resolved
Hide resolved
Thanks for the review @mcmire , I really appreciate it! I'll work on these changes and additions asap. |
…i-service/index.ts remove nested index
add service exports
@mcmire I think we're good on refactors per your comments, and test coverage is at 100%, but the linter is angry at new warnings being introduced (not from anything we worked on here though). Do we need to address those linter warnings separately, before this PR can be merged? |
Explanation
This is the introduction of metamask portfolio api's token search in the core monorepo.
This will return values from Moralis intended to provide search functionality coming soon to the metamask mobile app.
References
MMPD-1504
MMPD-1526
Changelog
@metamask/token-search-discovery-controller
Checklist