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

Changed type on Pairs entities property volumeToken[x] #62

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from

Conversation

GabrielCamba
Copy link
Contributor

@GabrielCamba GabrielCamba commented Aug 25, 2022

Close #61

On entities Pair, PairHourly and PairDaily the properties volumeToken0 and volumeToken1 was expressed in BigInt with each volume calculated in atoms. That type made difficult to compare volumes and have a better idea of the volume traded.

volumeToken0 and volumeToken1 is now expressed in BigDecimal making the calculation of the decimals using each token decimals property.

Example query:

{
  pairHourly(id:"0x4ecaba5870353805a9f068101a40e0f32ed605c6-0x6a023ccd1ff6f2045c3309768ead9e68f978f6e1-1628812800"){
    token0{
      symbol
    }
    volumeToken0
  }
}

Previous version result:

{
  "data": {
    "pairHourly": {
      "token0": {
        "symbol": "USDT"
      },
      "volumeToken0": "109547611"
    }
  }
}

New version result:

{
  "data": {
    "pairHourly": {
      "token0": {
        "symbol": "USDT"
      },
      "volumeToken0": "109.547611"
    }
  }
}
  • Model was fixed to show this change.

To test:

For testing this is deployed both on Ethereum mainnet and Gnosis chain

Here is an example query that should work for Pairs, PairsDailies and PairsHourlies entities:

{
  pairDailies(first: 5){
    token0{
      address
      symbol
    }
    token1 {
      address
      symbol
    }
    volumeToken0
    volumeToken1
  }
}

The expected result is to have volumes of the tokens expressed with decimals, based on each token definition

@GabrielCamba GabrielCamba added Subgraph Subgraph related task Protofire labels Aug 25, 2022
@GabrielCamba GabrielCamba requested a review from ramirotw August 25, 2022 12:43
@GabrielCamba GabrielCamba self-assigned this Aug 25, 2022
@GabrielCamba GabrielCamba changed the title Changed property type Changed type on Pairs entities property volumeToken[x] Aug 25, 2022
@GabrielCamba GabrielCamba marked this pull request as ready for review August 25, 2022 14:38
Copy link
Contributor

@alfetopito alfetopito left a comment

Choose a reason for hiding this comment

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

Very nice Gabi, great work in the PR description 👍

Is this deployed anywhere yet that can be tested?

If it is, please add it in the description under a testing section or similar

For reference, here's one of my recent PRs cowprotocol/cowswap#905

Think as if you are someone looking at this PR without much context and need to verify it.
List steps - if necessary - to validate the changes done here

Copy link
Contributor

@alfetopito alfetopito left a comment

Choose a reason for hiding this comment

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

Awesome!

@alfetopito
Copy link
Contributor

The PR is approved and the change is working as expected.

I have one question that is related but likely not affected by the changes here.
Should ETH token address and symbol (0xeee...) still be displayed like that?

Screen Shot 2022-08-26 at 15 39 31

@GabrielCamba
Copy link
Contributor Author

The PR is approved and the change is working as expected.

I have one question that is related but likely not affected by the changes here. Should ETH token address and symbol (0xeee...) still be displayed like that?

Sorry I missed this answer. Regarding this, we are only replacing the price for 0xee... token. If you are ok with this, I can add a ticket for addressing this change.

Base automatically changed from upgradeGraphVersionFix to develop September 27, 2022 12:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Protofire Subgraph Subgraph related task
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants