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

add token name symbol to pools #61

Merged
merged 11 commits into from
Jul 29, 2021
Merged

add token name symbol to pools #61

merged 11 commits into from
Jul 29, 2021

Conversation

alexcos20
Copy link
Member

@alexcos20 alexcos20 commented Mar 25, 2021

Closes #13

Changes proposed in this PR:

  • add token name to pool tokens
  • add token symbol to pool tokens

try it local with: query

Expected result:

{
        "balance": "100",
        "id": "0xa5342efad4ddd66dabb095ee2db4bd272d54445e-0x4d156a2ef69ffddc55838176c6712c90f60a2285",
        "poolId": {
          "tokens": [
            {
              "name": "Boorish Oyster Token",
              "symbol": "BOOOYS-26",
              "tokenAddress": "0x1d9a0ad91a5bd3d4091fd474c481a3079a846260",
              "tokenId": {
                "address": "0x1d9a0ad91a5bd3d4091fd474c481a3079a846260",
                "symbol": "BOOOYS-26"
              }
            },
            {
              "name": "OceanToken",
              "symbol": "OCEAN",
              "tokenAddress": "0x8967bcf84170c91b0d24d4302c2376283b0b3a07",
              "tokenId": null
            }
          ]
        },
        "userAddress": {
          "id": "0x4d156a2ef69ffddc55838176c6712c90f60a2285"
        }
      },

Questions: should we rename tokenId to datatokenId ?
If tokenId is null, then that token is not a datatoken

@kremalicious
Copy link
Contributor

It seems everything under tokenId is just redundant? name, symbol, address is all there already. I would ditch the token prefix for keys for everything under tokens[], cause also redundant. If we want to signal which one is a datatoken then something like isDatatoken would be way more clear.

And just double checking, the name for OCEAN will appear as Ocean Token (with space), right?

@alexcos20
Copy link
Member Author

sure, it makes sense to keep only address/symbol, as long as it is not breaking something (@idiom-bytes - is it ok from Dao/Snapsot side?)

@alexcos20
Copy link
Member Author

regarding symbol/names - it will fetch whatever is in the contract for symbol/names. Mainnet is with space, rinkeby without :)

@kremalicious
Copy link
Contributor

oh, interesting, so we never redeployed our token contract in the testnets as we did multiple times in mainnet. So the testnets do not reflect current token contract but guess this has no real implication https://github.com/oceanprotocol/token/blob/master/contracts/OceanToken.sol#L33

@alexcos20
Copy link
Member Author

oh, interesting, so we never redeployed our token contract in the testnets as we did multiple times in mainnet. So the testnets do not reflect current token contract but guess this has no real implication https://github.com/oceanprotocol/token/blob/master/contracts/OceanToken.sol#L33

indeed, we have only the old versions.

@alexcos20
Copy link
Member Author

new layout:

poolShares": [
      {
        "balance": "100",
        "id": "0xa5342efad4ddd66dabb095ee2db4bd272d54445e-0x4d156a2ef69ffddc55838176c6712c90f60a2285",
        "poolId": {
          "tokens": [
            {
              "address": "0x1d9a0ad91a5bd3d4091fd474c481a3079a846260",
              "isDatatoken": true,
              "name": "Boorish Oyster Token",
              "symbol": "BOOOYS-26"
            },
            {
              "address": "0x8967bcf84170c91b0d24d4302c2376283b0b3a07",
              "isDatatoken": false,
              "name": "OceanToken",
              "symbol": "OCEAN"
            }
          ]
        },
        "userAddress": {
          "id": "0x4d156a2ef69ffddc55838176c6712c90f60a2285"
        }
      },
      {
        "balance": "4.108596132489867437",
        "id": "0x18badbad531d05c8a8e7d8c3493ef9136d51430b-0x4d156a2ef69ffddc55838176c6712c90f60a2285",
        "poolId": {
          "tokens": [
            {
              "address": "0x8921f0f2bce1293772c4f091ebeda4d25eef024a",
              "isDatatoken": true,
              "name": "Sartorial Seagull Token",
              "symbol": "SARSEA-13"
            },
            {
              "address": "0x8967bcf84170c91b0d24d4302c2376283b0b3a07",
              "isDatatoken": false,
              "name": "OceanToken",
              "symbol": "OCEAN"
            }
          ]
        },
        "userAddress": {
          "id": "0x4d156a2ef69ffddc55838176c6712c90f60a2285"
        }
      }
    ]
```

Maybe we should add decimals as well?

@kremalicious
Copy link
Contributor

decimals is good idea, useful if we want to have more add token to metamask buttons

@alexcos20
Copy link
Member Author

done

{
        "balance": "100",
        "id": "0xa5342efad4ddd66dabb095ee2db4bd272d54445e-0x4d156a2ef69ffddc55838176c6712c90f60a2285",
        "poolId": {
          "tokens": [
            {
              "address": "0x1d9a0ad91a5bd3d4091fd474c481a3079a846260",
              "decimals": 18,
              "isDatatoken": true,
              "name": "Boorish Oyster Token",
              "symbol": "BOOOYS-26"
            },
            {
              "address": "0x8967bcf84170c91b0d24d4302c2376283b0b3a07",
              "decimals": 18,
              "isDatatoken": false,
              "name": "OceanToken",
              "symbol": "OCEAN"
            }
          ]
        },
        "userAddress": {
          "id": "0x4d156a2ef69ffddc55838176c6712c90f60a2285"
        }
      }

@kremalicious
Copy link
Contributor

as far as I can see this is a breaking change for market, but only on the History page:
https://github.com/oceanprotocol/market/blob/main/src/components/pages/History/PoolShares.tsx#L29

So if we want to deploy it it we need to prepare a small market update first

@alexcos20
Copy link
Member Author

ok, let's keep it on hold.
also @idiom-bytes , please check on your side as well

Copy link
Contributor

@kremalicious kremalicious left a comment

Choose a reason for hiding this comment

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

awesome, now much easier to solve oceanprotocol/market#456

@idiom-bytes
Copy link
Member

Hey guys,

As far as I can see, the only thing added was the token[{name, symbol}], in addition to the redundant bits that were removed.

Our pools query does not look into any of this data, so I don't think this will have an impact on the strategy (which isn't quite working yet).

For reference, this is the GQL query into DT pools.

I'm approving this PR.

{
pools (first: 1000, orderBy: oceanReserve, orderDirection: desc) {
  id,
  holderCount,
  oceanReserve,
  active,
  totalShares,
  shares (first: 1000) {
    id,
    userAddress {
      id,
      tokensOwned {
        id
      }
    },
    balance
  },
  tokens {
    balance,
    denormWeight,
    tokenId {
      id
    }
  }
}
}

@alexcos20
Copy link
Member Author

 tokens {
    balance,
    denormWeight,
    tokenId {
      id
    }
  }

tokenId was removed. you need to replace it probably with "address"

@idiom-bytes
Copy link
Member

idiom-bytes commented Mar 25, 2021

I see the latest schema.
We'll update the strategy.

@kremalicious
Copy link
Contributor

kremalicious commented Mar 25, 2021

this is the actual change: https://github.com/oceanprotocol/ocean-subgraph/pull/61/files#diff-efc1675187620595b0844197a25c35eac9b6df752f9182e5cffddee6f27a8489

So instead of:

tokens {
    balance,
    denormWeight,
    tokenId {
      id
    }
  }

after this change is deployed it should be like this:

tokens {
    balance,
    denormWeight,
    address
  }

As for market, was quick change oceanprotocol/market#457

Think we get our auto-generated typings from the Rinkeby subgraph so that PR build might succeed when it's deployed on Rinkeby. Otherwise we kinda have to deploy to Mainnet to make PR build succeed

@idiom-bytes
Copy link
Member

idiom-bytes commented Mar 25, 2021

As I mentioned to Alex, the strategy isn't quite working so this won't break anything.

I'll have a new GQL ready to go, and will test once the latest Rinkeby/Mainnet subgraph is available.

@alexcos20
Copy link
Member Author

alexcos20 commented Apr 7, 2021

@kremalicious @mihaisc - 2nd graph deployed and it's live.
check query
we can open a PR on market and do the updates without down time

@mihaisc mihaisc merged commit 9b3c466 into main Jul 29, 2021
@mihaisc mihaisc deleted the feature/tokens_symbols_names branch July 29, 2021 08:55
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.

poolId.tokens.tokenId.symbol is always null for OCEAN
4 participants