AlphaWallet / alpha-wallet-ios

An advanced Ethereum/EVM mobile wallet
https://www.alphawallet.com
MIT License
598 stars 372 forks source link

Read tokens.json and put tokens into the correct group in Wallet tab #3581

Closed hboon closed 2 years ago

hboon commented 2 years ago

Part of #3546 Depends on: #3580

A token matches if the contract and chain ID matches an entry in each array.

This file: https://github.com/AlphaWallet/tokens/blob/main/tokens.json (check update: https://github.com/AlphaWallet/alpha-wallet-ios/issues/3581#issuecomment-1019835025)

"All" should contain every token that is not hidden (like it is before the PR).

If a token is not categorized, display it under "Assets".

Just ignore the tokens tagged as "Collectibles" (if there are any) in the JSON. We still show all and only ERC721 and ERC1155 in the hardcoded Collectibles tab. Otherwise it wouldn't work.

eviltofu commented 2 years ago

Does this tokens.json file need to be compressed?

hboon commented 2 years ago

Don't work on this one yet.

eviltofu commented 2 years ago

Don't work on this one yet.

@hboon Is this still on hold?

hboon commented 2 years ago

Yes, but should be moving soon.

hboon commented 2 years ago

@eviltofu ok, can you try this file instead (it's embedded in the comment) : https://github.com/AlphaWallet/tokens/issues/1#issuecomment-1014318436

It looks small enough after the filtering/verification. Let's just embed it in the app directly. You'll have to hunt down wallets (to watch) that contains those tokens (buzz me if you are not sure how).

hboon commented 2 years ago

@eviltofu I forgot: do remember that the "Collectibles" tab is handled differently. I think, just ignore the tokens tagged as "Collectibles" (if there are any) in the JSON. We still show all and only ERC721 and ERC1155 in the hardcoded Collectibles tab. Otherwise it wouldn't work.

(Updated the first comment to include this)

eviltofu commented 2 years ago

@hboon im still trying to figure out how this screen works; where the updates are coming from etc.

hboon commented 2 years ago

@eviltofu Sure. I think a good place to start is how TokensViewModel exposes the data to TokensViewController's func tableView(_ tableView: UITableView, numberOfRowsInSection section: Int) -> Int { etc.

Probably have to modify how this works:

enum WalletFilter {
    case all
    case type(Set<TokenType>)
    case currencyOnly
    case assetsOnly
    case collectiblesOnly
    case keyword(String)
}

Good luck :)

hboon commented 2 years ago

Higher prority for this. Been stuck for a long time and there's a few things hanging on this.

eviltofu commented 2 years ago

@hboon

The tokens.json file has "group": "Assets", "group": "DeFi", "group": "Governance" group types. So the newly created code needs to read the tokens.json file and differentiate a token into the following types:

Is this correct?

An issue (#4141) has been created for this.

Can the file be named TokenGroups.json instead?

hboon commented 2 years ago

Is this correct?

Yes

Can the file be named TokenGroups.json instead?

No. And it's not specifically for grouping tokens. There will be other functionality tied to it.

eviltofu commented 2 years ago

@hboon Part of token.json is

"contracts": [
            {"address": "0x03ab458634910aad20ef5f1c8ee96f1d6ac54919", "chainId": 1},
            {"address": "0x00e5646f60AC6Fb446f621d146B6E1886f002905", "chainId": 137},
            {
                "address": "0xaeF5bbcbFa438519a5ea80B4c7181B4E78d419f2",
                "chainId": 42161
            },
            {"address": "0xa71353bb71dda105d383b02fc2dd172c4d39ef8b", "chainId": 250}
        ],

What does the address field map to in TokenObject? The contract field?

class TokenObject: Object {
    static func generatePrimaryKey(fromContract contract: AlphaWallet.Address, server: RPCServer) -> String {
        return "\(contract.eip55String)-\(server.chainID)"
    }

    @objc dynamic var primaryKey: String = ""
    @objc dynamic var chainId: Int = 0
    @objc dynamic var contract: String = Constants.nullAddress.eip55String
    @objc dynamic var name: String = ""
    @objc dynamic var symbol: String = ""
    @objc dynamic var decimals: Int = 0
    @objc dynamic var value: String = ""
    @objc dynamic var isDisabled: Bool = false
    @objc dynamic var rawType: String = TokenType.erc20.rawValue
    @objc dynamic var shouldDisplay: Bool = true
hboon commented 2 years ago

Yes. And the chainId to RPCServer

eviltofu commented 2 years ago

@hboon are the addresses case sensitive? I see mixed cases and some instances where both cases are used in the same address. 0x0AaB8D...

"contracts": [
            {"address": "0x595832f8fc6bf59c85c527fec3740a1b7a361269", "chainId": 1},
            {"address": "0x0AaB8DC887D34f00D50E19aee48371a941390d14", "chainId": 137},
            {
                "address": "0x4e91F2AF1ee0F84B529478f19794F5AFD423e4A6",
                "chainId": 42161
            }
        ],
        "group": "Assets"
eviltofu commented 2 years ago

@hboon some entries have no group property.

"contracts": [
      {"address": "0x4AaC461C86aBfA71e9d00d9a2cde8d74E4E1aeEa", "chainId": 1},
      {
        "address": "0x14B1f37c46ECf29C9657585DF0Dd7CEe1eC7C569",
        "chainId": 43114
      }
    ]

Where does a token matching this go? Assets?

hboon commented 2 years ago

@hboon are the addresses case sensitive? I see mixed cases and some instances where both cases are used in the same address. 0x0AaB8D...

Check this out: https://eips.ethereum.org/EIPS/eip-55

The 1 specific mix of uppercase and lowercase for Ethereum addresses are a checksum for the address itself.

They are the same address, but when we print them out in the app, we always use the EIP55 version (look for eip55String)

hboon commented 2 years ago

@hboon some entries have no group property.

"contracts": [ {"address": "0x4AaC461C86aBfA71e9d00d9a2cde8d74E4E1aeEa", "chainId": 1}, { "address": "0x14B1f37c46ECf29C9657585DF0Dd7CEe1eC7C569", "chainId": 43114 } ] Where does a token matching this go? Assets?

Yes, refer to https://github.com/AlphaWallet/alpha-wallet-ios/issues/3581#issue-1084438163

eviltofu commented 2 years ago

@hboon for the following entries, both first contracts 0x03ab458634910AaD20eF5f1C8ee96F1D6ac54919 have the same address and chainId. How should this be resolved? The file is taken from here. Is this an Asset or a Defi? Or is it both?

 {
        "contracts": [
            {"address": "0x03ab458634910AaD20eF5f1C8ee96F1D6ac54919", "chainId": 1},
            {"address": "0x7FB688CCf682d58f86D7e38e03f9D22e7705448B", "chainId": 10}
        ]
    },

and

    {
        "contracts": [
            {"address": "0x03ab458634910aad20ef5f1c8ee96f1d6ac54919", "chainId": 1},
            {"address": "0x00e5646f60AC6Fb446f621d146B6E1886f002905", "chainId": 137},
            {
                "address": "0xaeF5bbcbFa438519a5ea80B4c7181B4E78d419f2",
                "chainId": 42161
            },
            {"address": "0xa71353bb71dda105d383b02fc2dd172c4d39ef8b", "chainId": 250}
        ],
        "group": "DeFi"
    },
hboon commented 2 years ago

@eviltofu looks like a bug somewhere, so you can drop the group in such cases. Can you help in to https://github.com/AlphaWallet/tokens/issues/1 and @ foxgem to describe this?

eviltofu commented 2 years ago

@eviltofu looks like a bug somewhere, so you can drop the group in such cases. Can you help in to AlphaWallet/tokens#1 and @ foxgem to describe this?

I will only use the first occurrence and drop any future occurrences? Or will group-less entries have lower priority over entries with groups?

hboon commented 2 years ago

Ignore entire groups that contains tokens that have already appeared in previous groups. Pretending they will be fixed at the source.

eviltofu commented 2 years ago

@hboon for Collectibles, it is detected via TokenType?

   var type: TokenType {
        get {
            return TokenType(rawValue: rawType)!
        }
        set {
            rawType = newValue.rawValue
        }
    }
enum TokenType: String {
    case nativeCryptocurrency = "ether"
    case erc20 = "ERC20"
    case erc875 = "ERC875"
    case erc721 = "ERC721"
    case erc721ForTickets = "ERC721ForTickets"
    case erc1155 = "ERC1155"

    init(tokenInterfaceType: TokenInterfaceType) {
        switch tokenInterfaceType {
        case .erc20:
            self = .erc20
        case .erc721:
            self = .erc721
        case .erc875:
            self = .erc875
        case .erc1155:
            self = .erc1155
        }
    }
hboon commented 2 years ago

Yes

eviltofu commented 2 years ago

The plan now is Refactor the code so that instead of having one UITableViewDataSource and UITableViewDelegate that handles displaying all the tabs, we have one object per tab that handles the duties of data source and delegate. So if we want to add more tabs, we just create more classes to handle the tab instead of just extending the one data source/delegate.

hboon commented 2 years ago

I think you might need to have just have 2 of these UITableViewDataSource/UITableViewDelegate pairs because All, Assets, Defi, Governance look and work the same way whereas Collectibles is different instead of 1 pair for each tab.