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

feat: Add mm portfolio api - token search controller #5142

Open
wants to merge 21 commits into
base: main
Choose a base branch
from

Conversation

Bigshmow
Copy link

@Bigshmow Bigshmow commented Jan 13, 2025

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

  • ADDED: TokenSearchDiscoveryController which fetches token search results from the metamask portfolio api

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
  • I've prepared draft pull requests for clients and consumer packages to resolve any breaking changes

@Bigshmow Bigshmow changed the title Add mm portfolio api - token search controller feat: Add mm portfolio api - token search controller Jan 14, 2025
@Bigshmow Bigshmow marked this pull request as ready for review January 14, 2025 02:28
@Bigshmow Bigshmow requested a review from a team as a code owner January 14, 2025 02:28
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.

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.

@Bigshmow
Copy link
Author

Thanks for the review @mcmire , I really appreciate it! I'll work on these changes and additions asap.

@Bigshmow Bigshmow requested a review from mcmire January 15, 2025 17:20
@Bigshmow
Copy link
Author

@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?

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.

2 participants