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

Added logic to use an internal SpamToken image to TokenImageFetcherImpl instead of fetching it over the network #6657

Merged
merged 1 commit into from
Apr 11, 2023

Conversation

eviltofu
Copy link
Contributor

@eviltofu eviltofu commented Apr 5, 2023

Closes #6446

Simulator Screenshot - iPhone 14 Pro - 2023-04-11 at 09 06 17 copy

@eviltofu eviltofu requested a review from hboon April 5, 2023 05:19
@hboon hboon requested review from oa-s and removed request for hboon April 5, 2023 05:22
@hboon
Copy link
Member

hboon commented Apr 5, 2023

@eviltofu I just realized that we can't remove the spam token icon files from the iconassets repo rightaway since we need to give it some time after both the Android and iOS app has been live. Can you help create/move an issue into the iconassets repo to remove those files and assign to yourself, referring back to this comment so we remember to finish it up?

@eviltofu
Copy link
Contributor Author

eviltofu commented Apr 6, 2023

@hboon

So I schedule a task to remove the spam token icons files 1 month later?

Also, this process of adding to the adjustments.js file after checking iconassets needs to be streamlined. Maybe we can just have a spam.json file and then from that generate the adjustments.js file and remove the spam logo from iconassets? Generate a script file from spam.json that checks the iconassets repo. Also instead of removing the logo.png file, maybe just rename it to spam.png to hide it from the app? Otherwise if we make a mistake in classifying a token as spam and later discovering it was an error, we would need to go find and download the token logo icon again.

@hboon
Copy link
Member

hboon commented Apr 6, 2023

So I schedule a task to remove the spam token icons files 1 month later?

Yes, but can you do it this way: Set up a new test class that does this:

class Foo_Some_Name_Reminders: XCTestCase {
    func testRemoveSpamTokenIconFiles() {
        //Fails with a TODO to create an issue with "some title" on and after 20230601
    }
}

Thought I wrote about this approach in the Android repo for some other issue, but I forgot where

@hboon
Copy link
Member

hboon commented Apr 6, 2023

Also, this process of adding to the adjustments.js file after checking iconassets needs to be streamlined. Maybe we can just have a spam.json file and then from that generate the adjustments.js file and remove the spam logo from iconassets? Generate a script file from spam.json that checks the iconassets repo.

Improving the process is good. adjustments.json currently only holds contracts for the Spam group though. Maybe wait a bit to see what we do with the file before we modify the code/process? Seems unnecessary now.

Also instead of removing the logo.png file, maybe just rename it to spam.png to hide it from the app? Otherwise if we make a mistake in classifying a token as spam and later discovering it was an error, we would need to go find and download the token logo icon again.

It wouldn't be because it's the same icon file contents (and it's still in git any way)

@eviltofu
Copy link
Contributor Author

@oa-s still reviewing?

@eviltofu eviltofu requested review from hboon and removed request for oa-s April 10, 2023 09:15
@@ -130,6 +136,10 @@ public class TokenImageFetcherImpl: TokenImageFetcher {
blockChainNameColor: UIColor,
serverIconImage: UIImage?) -> TokenImage? {

if tokenGroupsIdentifier.isSpam(address: contractAddress.eip55String, chainID: server.chainID) {
return .init(image: .image(.loaded(image: spamImage)), isFinal: true, overlayServerIcon: nil)
Copy link
Member

Choose a reason for hiding this comment

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

Didn't test. But I think overlayServerIcon: still has to be either nil or not-nil using the switch-case a few lines later. That's how the current hardcoded spam (or any token image) loaded from the iconassets repo are displayed, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated. Please check @hboon

}
}

if tokenGroupsIdentifier.isSpam(address: contractAddress.eip55String, chainID: server.chainID) {
Copy link
Member

Choose a reason for hiding this comment

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

Seems like this will never be true because it was checked in the switch within the iconIamgeForContractAndhainID(image:address:chain:) call, so the if-block can be removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed.

@hboon hboon added this pull request to the merge queue Apr 11, 2023
Merged via the queue into AlphaWallet:master with commit 348a526 Apr 11, 2023
@hboon hboon deleted the SpamTokenImage branch April 11, 2023 03:17
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.

[tokens.json] Automatically apply the Spam token icon instead of the one from sources like iconassets repo
2 participants