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.

Sign up for GitHub

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

Jump to bottom

GNS tests fail if subgraphNFT's tokenURI has leading zeros #576

Open
tmigone opened this issue May 23, 2022 · 3 comments · May be fixed by #577
Open

GNS tests fail if subgraphNFT's tokenURI has leading zeros #576

tmigone opened this issue May 23, 2022 · 3 comments · May be fixed by #577
Assignees
@tmigone

Comments

@tmigone
Copy link
Contributor

tmigone commented May 23, 2022

Will work on reproducing the issue, in the meantime here are logs from a recent failed PR check:


  1) GNS
       NFT descriptor
         without token descriptor:

      AssertionError: expected '0x00fae00cac62e26a062ca75eb35392df730f5e6c84aa36aa7d199b4bd713f73a' to equal '0xfae00cac62e26a062ca75eb35392df730f5e6c84aa36aa7d199b4bd713f73a'
      + expected - actual

      -0x00fae00cac62e26a062ca75eb35392df730f5e6c84aa36aa7d199b4bd713f73a
      +0xfae00cac62e26a062ca75eb35392df730f5e6c84aa36aa7d199b4bd713f73a
      
      at Context.<anonymous> (test/gns.test.ts:1042:27)

  2) GNS
       NFT descriptor
         without token descriptor and baseURI:

      AssertionError: expected 'ipfs://0x00fae00cac62e26a062ca75eb35392df730f5e6c84aa36aa7d199b4bd713f73a' to equal 'ipfs://0xfae00cac62e26a062ca75eb35392df730f5e6c84aa36aa7d199b4bd713f73a'
      + expected - actual

      -ipfs://0x00fae00cac62e26a062ca75eb35392df730f5e6c84aa36aa7d199b4bd713f73a
      +ipfs://0xfae00cac62e26a062ca75eb35392df730f5e6c84aa36aa7d199b4bd713f73a
      
      at Context.<anonymous> (test/gns.test.ts:1055:39)
tmigone added a commit that referenced this issue May 24, 2022
@tmigone
fix: ensure subgraphNFT handles metadata with leading zeros correctly
5e1db55 
Closes: #576
Signed-off-by: Tomás Migone <tomas@edgeandnode.com>
@tmigone tmigone linked a pull request May 24, 2022 that will close this issue
Open
tmigone added a commit that referenced this issue May 24, 2022
@tmigone
fix: ensure subgraphNFT handles metadata with leading zeros correctly
ff7880e 
Closes: #576
Signed-off-by: Tomás Migone <tomas@edgeandnode.com>
@tmigone
Copy link
Contributor Author

tmigone commented May 24, 2022

Looks like the leading zeros are being removed by function toString(uint256 value) here. This in turn means that tokenURI returns a truncated IPFS hash (ipfs://0x00de4b0939985d32d8241c279800b133e4d75c76ee913d035b3e5f2baf922cb7 instead of ipfs://0xde4b0939985d32d8241c279800b133e4d75c76ee913d035b3e5f2baf922cb7).

This bug is not critical but could lead to users interacting with the contract to observe incorrect behavior, for example if converting the IPFS hash into an IPFS CID since the leading zeros do make a difference in the resulting value.

This is not necessarily a bug with the library as in ASCII representation of values we don't specify leading zeros (ie: 05 vs 5) so we should account for it when using it in subgraphNFT contract.

@tmigone tmigone self-assigned this May 27, 2022
@pcarranzav pcarranzav mentioned this issue Jul 14, 2022
Merged
@pcarranzav pcarranzav mentioned this issue Sep 29, 2022
Closed
@tmigone
Copy link
Contributor Author

tmigone commented Nov 28, 2022

Another instance of this happening: https://github.com/graphprotocol/contracts/actions/runs/3567393752/jobs/5995037836

@tmigone tmigone mentioned this issue Nov 28, 2022
Merged
@tmigone
Copy link
Contributor Author

tmigone commented Jan 25, 2023

Happened again: https://github.com/graphprotocol/contracts/actions/runs/4006618984/jobs/6878385117

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@tmigone tmigone

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix: ensure subgraphNFT handles metadata with leading zeros correctly graphprotocol/contracts
4 participants
@tmigone and others