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: OS to Reservoir migration #9547

Merged
merged 1 commit into from
May 8, 2024
Merged

feat: OS to Reservoir migration #9547

merged 1 commit into from
May 8, 2024

Conversation

sahar-fehri
Copy link
Contributor

@sahar-fehri sahar-fehri commented May 6, 2024

Description

This PR adds patches to mobile to enable Open Sea (OS) migration.

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-18-0-0...patch/mobile-assets-controllers-v-18-0-0-os-migration-v2?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

Screen.Recording.2024-05-08.at.00.15.48.mov
Screen.Recording.2024-05-08.at.01.38.49.mov

After

Screen.Recording.2024-05-07.at.23.44.39.mov
Screen.Recording.2024-05-08.at.00.08.08.mov

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 review from a team as code owners May 6, 2024 14:03
Copy link
Contributor

github-actions bot commented May 6, 2024

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 self-assigned this May 6, 2024
@sahar-fehri sahar-fehri added needs-assets-ux-review A shared label between the Assets and UX team to flag PRs ready for consolidated team review. team-assets team-mobile-ux DEPRECATED: please use "team-wallet-ux" label instead labels May 6, 2024
Copy link

sonarcloud bot commented May 6, 2024

@sahar-fehri sahar-fehri added the Run Smoke E2E Triggers smoke e2e on Bitrise label May 6, 2024
Copy link
Contributor

github-actions bot commented May 6, 2024

https://bitrise.io/ Bitrise

✅✅✅ pr_smoke_e2e_pipeline passed on Bitrise! ✅✅✅

Commit hash: 52d7aeb
Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/9bc2efd0-3271-4b9c-8cf2-0a359f56a128

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

@tommasini
Copy link
Contributor

Hey @sahar-fehri this PR looks good to me, but can we have a recording with the nft auto detect and custom import to have a history of how it was the functionality now? It's good as well to record playing around with the IPFS Gateway Enabled and Display Nft Media privacy toggles to make sure the privacy features

@sahar-fehri
Copy link
Contributor Author

Hey @sahar-fehri this PR looks good to me, but can we have a recording with the nft auto detect and custom import to have a history of how it was the functionality now? It's good as well to record playing around with the IPFS Gateway Enabled and Display Nft Media privacy toggles to make sure the privacy features

Thanks @tommasini 🙏 i added videos to the description!
You might notice that after the migration some collection logos might look different. This is because the new third party provider is returning sometimes the nft image and not the collection image. I think this is not a blocker to the rollout and we can check this with them.

Copy link
Contributor

@tommasini tommasini left a comment

Choose a reason for hiding this comment

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

LGTM!

@sahar-fehri sahar-fehri merged commit 0d20df1 into main May 8, 2024
50 of 51 checks passed
@sahar-fehri sahar-fehri deleted the feat/os-migration-v2 branch May 8, 2024 16:15
@github-actions github-actions bot locked and limited conversation to collaborators May 8, 2024
@metamaskbot metamaskbot added the release-7.23.0 Issue or pull request that will be included in release 7.23.0 label May 8, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
needs-assets-ux-review A shared label between the Assets and UX team to flag PRs ready for consolidated team review. release-7.23.0 Issue or pull request that will be included in release 7.23.0 Run Smoke E2E Triggers smoke e2e on Bitrise team-assets team-mobile-ux DEPRECATED: please use "team-wallet-ux" label instead
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

4 participants