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.

Sign up for GitHub

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

Jump to bottom

feat: reservoir migration #4030

Merged
merged 16 commits into from
Mar 25, 2024
Merged

feat: reservoir migration #4030

merged 16 commits into from
Mar 25, 2024

Conversation

sahar-fehri
Copy link
Contributor

@sahar-fehri sahar-fehri commented Mar 7, 2024 •
edited
Loading

Explanation

Migrates the calls from OpenseaV2 to Reservoir.
Removes unused Opensea types and add Reservoir types.

Ideally the types are shared from NFT-API to other consuming clients rather than hardcoding them on core. We will address this in the future.

References

Changelog

@metamask/assets-controllers

  • REMOVED: Removed opensea map functions from assetsUtil.ts like mapOpenSeaNftV2ToV1, mapOpenSeaDetailedNftV2ToV1 and mapOpenSeaContractV2ToV1.
  • ADDED: Added NFT_API_BASE_URL in constants.ts
  • ADDED: Added reservoir types in NftDetectionController.ts
  • REMOVED: Call to openseaV2 to get user nfts in NftDetectionController.ts
  • Added: Call to reservoir to get user nfts in NftDetectionController.ts
  • REMOVED: Removed Opensea types from NftController.ts
  • REMOVED: Removed call Opensea to get NFT information and added call to reservoir in NftController.ts
  • REMOVED: Removed fct getNftContractInformationFromApi that calls OS
  • CHANGED: Changed fct getNftContractInformation to receive nftMetadataFromApi as arg (which contains contract information fetched already from reservoir and aggregate the result with getNftContractInformationFromContract
  • CHANGED: Changed the call to getNftInformation inside addNft function to be before addNftContract, because the nftInformation received from reservoir already includes collection metadata, no need to make another separate call to fetch it.

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

@sahar-fehri sahar-fehri requested a review from a team as a code owner March 7, 2024 15:05
@sahar-fehri sahar-fehri marked this pull request as draft March 7, 2024 15:05
@sahar-fehri sahar-fehri marked this pull request as ready for review March 18, 2024 12:56
@sahar-fehri sahar-fehri changed the title feat: initial reservoir migration commit feat: reservoir migration commit Mar 18, 2024
@sahar-fehri sahar-fehri changed the title feat: reservoir migration commit feat: reservoir migration Mar 18, 2024
mcmire
mcmire reviewed Mar 18, 2024
View reviewed changes
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.

This looks pretty good. Most of my suggestions below were about JSDocs/TypeDocs.

packages/assets-controllers/src/NftDetectionController.ts Outdated Show resolved Hide resolved
packages/assets-controllers/src/NftDetectionController.ts Outdated Show resolved Hide resolved
packages/assets-controllers/src/NftDetectionController.ts Outdated Show resolved Hide resolved
packages/assets-controllers/src/NftDetectionController.ts Outdated Show resolved Hide resolved
packages/assets-controllers/src/NftDetectionController.ts Outdated Show resolved Hide resolved
packages/assets-controllers/src/NftDetectionController.ts Outdated Show resolved Hide resolved
packages/assets-controllers/src/NftDetectionController.ts Outdated Show resolved Hide resolved
packages/assets-controllers/src/assetsUtil.ts Outdated Show resolved Hide resolved
@sahar-fehri sahar-fehri requested a review from mcmire March 19, 2024 14:13
@MajorLift MajorLift mentioned this pull request Mar 19, 2024
Merged
@bergeron
Copy link
Contributor

Generally looks good to me! Love that we can get more information in fewer requests

bergeron
bergeron previously approved these changes Mar 20, 2024
View reviewed changes
mcmire
mcmire previously approved these changes Mar 20, 2024
View reviewed changes
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.

This looks good to me! Just one comment, but is non-blocking.

packages/assets-controllers/src/NftController.ts Outdated Show resolved Hide resolved
@mcmire
Copy link
Contributor

mcmire commented Mar 20, 2024

This is also non-blocking, but it seems there are a few breaking changes in this PR. Can you note those in Changes? Perhaps we want this:

- **REMOVED**: **BREAKING:** Removed opensea map functions from assetsUtil.ts like `mapOpenSeaNftV2ToV1`,  `mapOpenSeaDetailedNftV2ToV1` and `mapOpenSeaContractV2ToV1`.
- **ADDED**: Added `NFT_API_BASE_URL` in constants.ts
- **ADDED**: Added reservoir types in NftDetectionController.ts
- **REMOVED**: **BREAKING:** Call to openseaV2 to get user nfts in NftDetectionController.ts
- **Added**: Call to reservoir to get user nfts in NftDetectionController.ts
- **REMOVED**: **BREAKING:** Removed Opensea types from NftController.ts
- **REMOVED**: **BREAKING:** Removed call Opensea to get NFT information and added call to reservoir in  NftController.ts
- **REMOVED**: **BREAKING:** Removed fct getNftContractInformationFromApi that calls OS
- **CHANGED**: Changed fct `getNftContractInformation` to receive `nftMetadataFromApi` as arg (which contains contract information fetched already from reservoir and aggregate the result with `getNftContractInformationFromContract`
- **CHANGED**: Changed the call to `getNftInformation` inside `addNft` function to be before  `addNftContract`, because the nftInformation received from reservoir already includes collection metadata, no need to make another separate call to fetch it.

@sahar-fehri sahar-fehri dismissed stale reviews from mcmire and bergeron via ff83052 March 21, 2024 09:07
@sahar-fehri sahar-fehri mentioned this pull request Mar 22, 2024
Closed
7 tasks
@sahar-fehri sahar-fehri mentioned this pull request Mar 25, 2024
Closed
13 tasks
@sahar-fehri sahar-fehri merged commit 80ff2fb into main Mar 25, 2024
139 checks passed
@sahar-fehri sahar-fehri deleted the feat/reservoir-migration branch March 25, 2024 23:49
@Gudahtt Gudahtt mentioned this pull request Apr 12, 2024
Merged
@montelaidev montelaidev mentioned this pull request Apr 17, 2024
Merged
3 tasks
montelaidev added a commit that referenced this pull request Apr 17, 2024
@montelaidev
Release/140.0.0 (#4177)
ad25a2a 
## Explanation

## References

## Changelog

## [13.0.0] Accounts Controller

### Changed

- Fix update setSelectedAccount to throw if the id is not found
([#4167](#4167))
- Fix normal account indexing naming with index gap
([#4089](#4089))
- **BREAKING** Bump peer dependency `@metamask/snaps-controllers` to
`^6.0.3` and dependencies `@metamask/snaps-sdk` to `^3.1.1`,
`@metamask/eth-snap-keyring` to
`^3.0.0`([#4090](#4090))

## [28.0.0] Assets Controller

### Added

- Add reservoir migration
([#4030](#4030))

### Changed

- Fix getting nft tokenURI
([#4136](#4136))
- **BREAKING** Bump peer dependency on `@metamask/keyring-controller`
([#4090](#4090))
- Fix token detection during account change
([#4133](#4133))
- Fix update nft metadata when toggles off
([#4096](#4096))
- Adds `tokenMethodIncreaseAllowance`
([#4069](#4069))
- Fix mantle token mispriced
([#4045](#4045))

## [15.0.0] Keyring Controller

### Changed

- **BREAKING** use getAccounts on HD Keyring when calling addNewAccount
([#4158](#4158))
- Pass CAIP-2 scope to execution context
([#4090](#4090))
- Allow gas limits to be changed during #addPaymasterData
([#3942](#3942))

## [10.0.0] Preferences Controller

### Changed

- **BREAKING** Bump peer dependency on `@metamask/keyring-controller` to
`^15.0.0` ([#4090](#4090))
- Restore previous behavior of toChecksumHexAddress
([#4046](#4046))

## [15.0.0] Signature Controller

### Changed

- **BREAKING** Bump peer dependency on `@metamask/keyring-controller` to
`^15.0.0` ([#4090](#4090))

## [8.0.0] User Operation Controller 

### Changed

- **BREAKING** Bump peer dependency on `@metamask/keyring-controller` to
`^15.0.0` and Pass CAIP-2 scope to execution context
([#4090](#4090))
- Allow gas limits to be changed during #addPaymasterData
([#3942](#3942))


## 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
@sahar-fehri sahar-fehri mentioned this pull request May 6, 2024
Merged
7 tasks
sahar-fehri added a commit to MetaMask/metamask-mobile that referenced this pull request May 8, 2024
@sahar-fehri
feat: OS to Reservoir migration (#9547)
0d20df1 
## **Description**

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

## **Related issues**


[MMASSETS-156.](https://consensyssoftware.atlassian.net/jira/software/projects/MMASSETS/boards/640/backlog?selectedIssue=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**


https://github.com/MetaMask/metamask-mobile/assets/10994169/27d6e580-adb3-47f3-8bfe-5d2b9238a3f1




https://github.com/MetaMask/metamask-mobile/assets/10994169/f3eee367-1dcf-4e78-b738-d6e29f1bef86


### **After**


https://github.com/MetaMask/metamask-mobile/assets/10994169/920747e7-ab27-41ab-961a-88004f087c9a




https://github.com/MetaMask/metamask-mobile/assets/10994169/17fa6860-a7b5-4897-a543-d58bb0cdbd5e



## **Pre-merge author checklist**

- [ ] I’ve followed [MetaMask Coding
Standards](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/CODING_GUIDELINES.md).
- [ ] 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](https://jsdoc.app/) format
if applicable
- [ ] I’ve applied the right labels on the PR (see [labeling
guidelines](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/LABELING_GUIDELINES.md)).
Not required for external contributors.

## **Pre-merge reviewer checklist**

- [x] I've manually tested the PR (e.g. pull and build branch, run the
app, test code being changed).
- [x] 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.
Andepande pushed a commit to MetaMask/metamask-mobile that referenced this pull request May 21, 2024
@sahar-fehri @Andepande
feat: OS to Reservoir migration (#9547)
913d0f0 
## **Description**

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

## **Related issues**


[MMASSETS-156.](https://consensyssoftware.atlassian.net/jira/software/projects/MMASSETS/boards/640/backlog?selectedIssue=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**


https://github.com/MetaMask/metamask-mobile/assets/10994169/27d6e580-adb3-47f3-8bfe-5d2b9238a3f1




https://github.com/MetaMask/metamask-mobile/assets/10994169/f3eee367-1dcf-4e78-b738-d6e29f1bef86


### **After**


https://github.com/MetaMask/metamask-mobile/assets/10994169/920747e7-ab27-41ab-961a-88004f087c9a




https://github.com/MetaMask/metamask-mobile/assets/10994169/17fa6860-a7b5-4897-a543-d58bb0cdbd5e



## **Pre-merge author checklist**

- [ ] I’ve followed [MetaMask Coding
Standards](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/CODING_GUIDELINES.md).
- [ ] 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](https://jsdoc.app/) format
if applicable
- [ ] I’ve applied the right labels on the PR (see [labeling
guidelines](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/LABELING_GUIDELINES.md)).
Not required for external contributors.

## **Pre-merge reviewer checklist**

- [x] I've manually tested the PR (e.g. pull and build branch, run the
app, test code being changed).
- [x] 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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mcmire mcmire mcmire approved these changes

@bergeron bergeron bergeron approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@sahar-fehri @bergeron @mcmire