Skip to content

[Hotfix] /4.0.1 NFT improvement#3657

Merged
sethkfman merged 43 commits into
release/4.0.1-RC2from
hotfx/4.0.1_nft_improvement
Jan 28, 2022
Merged

[Hotfix] /4.0.1 NFT improvement#3657
sethkfman merged 43 commits into
release/4.0.1-RC2from
hotfx/4.0.1_nft_improvement

Conversation

@sethkfman

Copy link
Copy Markdown
Contributor

Description

Write a short description of the changes included in this pull request.

Checklist

  • There is a related GitHub issue
  • Tests are included if applicable
  • Any added code is fully documented

Issue

Resolves #???

sethkfman and others added 30 commits January 20, 2022 15:14
* point build badges to main

* point video_source_uri to main

* point architecture.svg to main

* point translation subtitle uris to main
* Refactor RPC Methods

* Migrate to typescript

* Improve selectedAddress code

* Implement Swappable proxy for provider

* Fix url on rpc methods

* Fix app version code

* Temporarily fix Typescript issues on RPCMethodMiddleware

* Remove Beta

Co-authored-by: Cal Leung <cleun007@gmail.com>
@sethkfman sethkfman requested a review from a team as a code owner January 28, 2022 00:56
@github-actions

Copy link
Copy Markdown
Contributor

CLA Signature Action:

Thank you for your submission, we really appreciate it. We ask that you all read and sign our Contributor License Agreement before we can accept your contribution. You can sign the CLA by just by adding a comment to this pull request with this exact sentence:

I have read the CLA Document and I hereby sign the CLA

By commenting with the above message you are agreeing to the terms of the CLA. Your account will be recorded as agreeing to our CLA so you don't need to sign it again for future contributions to this repository.

1 out of 2 committers have signed the CLA.
@sethkfman
❌ @github-actions

GitHub can't find an account for github-actions.
You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.

@sethkfman sethkfman mentioned this pull request Jan 28, 2022
1 task
@sethkfman sethkfman added needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) needs-qa Any New Features that needs a full manual QA prior to being added to a release. labels Jan 28, 2022

@gantunesr gantunesr left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM. I'll give it another look tomorrow morning just in case I missed something

@cortisiko cortisiko added QA'd - Issues Found QA has been complete, however issues have been discovered that need to be addressed and removed needs-qa Any New Features that needs a full manual QA prior to being added to a release. labels Jan 28, 2022

@cortisiko cortisiko left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Issue 1.
With NFT auto detection turned off when i try to manually import my NFT, nothing happens. http://recordit.co/yLvEmxe7pP

to reproduce:
log in to your wallet
tap on the NFT tab
do not turn on NFT token detection but toggle on “enable OpenSea API”.
Import an NFT in your wallet
notice the NFT does not import.
I killed the app and relaunch and the NFT does not appear.

second set of reproduction steps if the above does not work:
log in to your wallet
tap on the NFT tab
toggle on NFT token detection
remove one of your NFT’s.
toggle token detection off.
import the NFT you removed.
notice the NFT does not import.

Issue 2.
The NFT auto-detection message appears on the import tokens view: https://www.screencast.com/t/2Fzobx9l4gql

Issue 3. (being nit picky here.)
going to the NFT detection toggle on the privacy & settings view is not as fluid compared to going to the enhance detection toggle on the experimental view.

NFT detection toggle: http://recordit.co/5InmXTVJmS
Token detection toggle: http://recordit.co/Q3qj6Fhzrv

Potential Issue 4.
This happened to me a couple times when i first started testing. Now I can no longer reproduce. Can you look into it and ensure it is not an issue:

when NFT auto detection is switched on, it only imports your first (oldest) NFT. The other NFT's do not import.

to reproduce:
log in to your wallet
tap on the NFT tab
turn on NFT token detection. Notice your first NFT imports
notice the other NFT’s does not import.

unfortunately I cannot provide a recording. But it did happen.

Comment thread app/components/Views/AddAsset/index.js Outdated
Comment on lines +107 to +111
{isMainNet(chainId) && !dismissNftInfo && !useCollectibleDetection && (
<View style={styles.infoWrapper}>
<CollectibleDetectionModal onDismiss={this.dismissNftInfo} navigation={navigation} />
</View>
)}

@gantunesr gantunesr Jan 28, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is one of the bugs reported. It's necessary to have a conditional as the one below.

{assetType === 'token' ? (
  //...
) : (
  //...
)}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

fixed

@gantunesr gantunesr left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Left a small question

Comment thread app/components/Views/Settings/SecuritySettings/index.js Outdated
@sethkfman sethkfman requested a review from gantunesr January 28, 2022 18:25
@cortisiko cortisiko added QA Passed QA testing has been completed and passed and removed QA'd - Issues Found QA has been complete, however issues have been discovered that need to be addressed labels Jan 28, 2022

@cortisiko cortisiko left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

🌮 🌮 🌮

@gantunesr gantunesr left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

@sethkfman sethkfman removed the needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) label Jan 28, 2022
@sethkfman sethkfman merged commit 7354c43 into release/4.0.1-RC2 Jan 28, 2022
@sethkfman sethkfman deleted the hotfx/4.0.1_nft_improvement branch January 28, 2022 18:56
@github-actions github-actions Bot locked and limited conversation to collaborators Jan 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

QA Passed QA testing has been completed and passed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants