contracts icon indicating copy to clipboard operation
contracts copied to clipboard

GNS tests fail if subgraphNFT's tokenURI has leading zeros

Open tmigone opened this issue 2 years ago • 3 comments

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 avatar May 23 '22 20:05 tmigone

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 avatar May 24 '22 17:05 tmigone

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

tmigone avatar Nov 28 '22 18:11 tmigone

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

tmigone avatar Jan 25 '23 16:01 tmigone