Skip to content

Conversation

@sahar-fehri
Copy link
Contributor

@sahar-fehri sahar-fehri commented Mar 22, 2024

Description

This PR adds patches to mobile to enable Open Sea (OS) migration.
It also fixes a small bug where this component app/components/UI/CollectibleContracts/index.js when on mainnet keeps making calls to OS while the NFTs are already detected.

Related issues

MMASSETS-156. (Restricted access Jira issue)

Main core PR: MetaMask/core#4030
Core Compare link used to create the assets patch: https://github.com/MetaMask/core/compare/patch/mobile-assets-controllers-v-12-0-0...patch/mobile-assets-controllers-v-12-0-0-res-migration?expand=1

Context

Problem: We want to roll out Reservoir to Extension and Mobile similar to how we have done in Portfolio.
Requirements:
Define a way to compare OpenSea’s spam filtering and Reservoir’s spam filtering. Ideally the spam filtering is similar.
Create Core PR to migrate calls from OS to Reservoir.
Create PR on mobile to migrate from OS to Reservoir.
Create Extension PR to migrate from OS to Reservoir
Define roll out plan for Extension and Mobile
Roll out Reservoir to Extension and Mobile

Today, to get the NFTs core is calling opensea V2 api to fetch user nfts and to add new nfts (get NFT metadata)
In this ticket we wanted to call Reservoir API instead of calling openseaV2.
We are not changing any of the functional behaviors of the app, we are just swapping third party providers.
So the app should behave the exact same regarding any NFT related functionality:
Like showing NFT details when clicking on the NFT
And being able to remove/add the NFT
Mark it as favorite
Sending NFts

Manual testing steps

  1. Go to the NFT tab
  2. You should see your NFTs
  3. Adding/removing NFTs should still work as expected
  4. clicking on the NFT and seeing the NFT details should work as expected.

Screenshots/Recordings

The NFT tab should behave the same. You should be able to see the same NFTs you had.

Only thing noticed while testing is for some collections you might see either a different image or the mobile default image (if the new third party provider was not able to return the image)

Before

After

Pre-merge author checklist

  • I’ve followed MetaMask Coding Standards.
  • I've completed the PR template to the best of my ability
  • I’ve included tests if applicable
  • I’ve documented my code using JSDoc format if applicable
  • I’ve applied the right labels on the PR (see labeling guidelines). Not required for external contributors.

Pre-merge reviewer checklist

  • I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed).
  • I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots.

@sahar-fehri sahar-fehri requested a review from a team March 22, 2024 12:53
@sahar-fehri sahar-fehri requested a review from a team as a code owner March 22, 2024 12:53
@github-actions
Copy link
Contributor

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

@sahar-fehri sahar-fehri changed the title Feat/os migration feat: os migration Mar 22, 2024
@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 45.59%. Comparing base (f031748) to head (98c94f5).
Report is 9 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #9032      +/-   ##
==========================================
+ Coverage   45.50%   45.59%   +0.08%     
==========================================
  Files        1275     1276       +1     
  Lines       31202    31311     +109     
  Branches     3184     3203      +19     
==========================================
+ Hits        14199    14276      +77     
- Misses      16163    16189      +26     
- Partials      840      846       +6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@github-actions
Copy link
Contributor

github-actions bot commented Mar 22, 2024

https://bitrise.io/ Bitrise

✅✅✅ pr_smoke_e2e_pipeline passed on Bitrise! ✅✅✅

Commit hash: 98c94f5
Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/f977f228-05b1-4bb1-b2eb-517aa78a0ef2

Note

  • You can kick off another pr_smoke_e2e_pipeline on Bitrise by removing and re-applying the Run Smoke E2E label on the pull request

@sonarqubecloud
Copy link

@sahar-fehri sahar-fehri marked this pull request as draft March 22, 2024 13:29
@sahar-fehri sahar-fehri marked this pull request as ready for review March 22, 2024 14:38
@NicolasMassart NicolasMassart changed the title feat: os migration feat: Open Sea migration Mar 28, 2024
@NicolasMassart NicolasMassart changed the title feat: Open Sea migration feat: Open Sea (OS) migration Mar 28, 2024
@NicolasMassart NicolasMassart changed the title feat: Open Sea (OS) migration feat: Open Sea (OS) to Reservoir migration Mar 28, 2024
Copy link
Contributor

@NicolasMassart NicolasMassart left a comment

Choose a reason for hiding this comment

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

Looks good to me.
I reviewed the patch outside of github as it's too big to display.
I was able to confirm the change to new service and no conneciton to opensea

# HTTP request for nft.api.cx.metamask.io/tokens (ID: c2fbdaaa-3de8-4376-8bad-34b718d11279)
  ## Request
  HTTP GET https://nft.api.cx.metamask.io/tokens?chainIds=1&tokens=...&includeTopBid=true&includeAttributes=true&includeLastSale=true
  Accept-Encoding: gzip
Connection: Keep-Alive
Cookie: ...
Host: nft.api.cx.metamask.io
User-Agent: okhttp/4.9.2
version: 1

  ## Response
  HTTP 200 undefined

@NicolasMassart NicolasMassart added No QA Needed Apply this label when your PR does not need any QA effort. needs-smoke-e2e needs-qa Any New Features that needs a full manual QA prior to being added to a release. and removed No QA Needed Apply this label when your PR does not need any QA effort. labels Mar 28, 2024
@legobeat legobeat changed the title feat: Open Sea (OS) to Reservoir migration feat: Open Sea to Reservoir migration May 2, 2024
-exports.OPENSEA_PROXY_URL = 'https://proxy.metafi.codefi.network/opensea/v1/api/v1';
-exports.OPENSEA_API_URL = 'https://api.opensea.io/api/v1';
-exports.OPENSEA_TEST_API_URL = 'https://testnets-api.opensea.io/api/v1';
+exports.OPENSEA_PROXY_URL = 'https://proxy.metafi.codefi.network/opensea/v1/api/v2';
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps these could be made configurable, instead of harcoded constants, in the upstream package? Would simplify this, remove the need for a patch, and these could be treated like any build config.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey @legobeat !
Are there examples of how we can make the url configurable? also do you think it can be done on a separate PR?
I will close this PR and create a new one because the assets-controller version on mobile has been upgraded and i will be using a different core branch to create the patch 🙏

@sahar-fehri
Copy link
Contributor Author

Closing this one in favor of this #9547

@sahar-fehri sahar-fehri closed this May 6, 2024
@github-actions github-actions bot locked and limited conversation to collaborators May 6, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

needs-qa Any New Features that needs a full manual QA prior to being added to a release. team-mobile-ux Mobile UX team

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

5 participants