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

Fix error in getLogs method #805

Conversation

danielattilasimon
Copy link

@danielattilasimon danielattilasimon commented Apr 28, 2020

When listening on an event filter with a null parameter followed by
a non-null parameter, it would keep throwing the error:

{ code: -32600, message: "data types must start with 0x" }

Turns out params.topics were being encoded as e.g.:

  "topics": [
    ["0x000000000000000000000000a94f5374fce5edbc8e2a8697c15331677e6ebf0b"],
    [null],
    [
      "0x000000000000000000000000a94f5374fce5edbc8e2a8697c15331677e6ebf0b",
      "0x0000000000000000000000000aff3454fce5edbc8cca8697c15331677e6ebccc"
    ]
  ]

instead of:

  "topics": [
    "0x000000000000000000000000a94f5374fce5edbc8e2a8697c15331677e6ebf0b",
    null,
    [
      "0x000000000000000000000000a94f5374fce5edbc8e2a8697c15331677e6ebf0b",
      "0x0000000000000000000000000aff3454fce5edbc8cca8697c15331677e6ebccc"
    ]
  ]

Fix this by flattening the array if there is only one topic in a
position.

When listening on an event filter with a null parameter followed by
a non-null parameter, it would keep throwing the error:

  { code: -32600, message: "data types must start with 0x" }`

Turns out params.topics were being encoded as e.g.:

  "topics": [
    ["0x000000000000000000000000a94f5374fce5edbc8e2a8697c15331677e6ebf0b"],
    [null],
    [
      "0x000000000000000000000000a94f5374fce5edbc8e2a8697c15331677e6ebf0b",
      "0x0000000000000000000000000aff3454fce5edbc8cca8697c15331677e6ebccc"
    ]
  ]

instead of:

  "topics": [
    "0x000000000000000000000000a94f5374fce5edbc8e2a8697c15331677e6ebf0b",
    null,
    [
      "0x000000000000000000000000a94f5374fce5edbc8e2a8697c15331677e6ebf0b",
      "0x0000000000000000000000000aff3454fce5edbc8cca8697c15331677e6ebccc"
    ]
  ]

Fix this by flattening the array if there is only one topic in a
position.
@ricmoo ricmoo added investigate Under investigation and may be a bug. on-deck This Enhancement or Bug is currently being worked on. labels Apr 28, 2020
@ricmoo
Copy link
Member

ricmoo commented Apr 28, 2020

Thanks. Looking into this now. I'd have thought null and [ null ] would both match the same filter, but I'll fix this right away. :)

@ricmoo
Copy link
Member

ricmoo commented Apr 28, 2020

Can you provide an example of how you generated that first filter?

@ricmoo
Copy link
Member

ricmoo commented Apr 28, 2020

When I try, I get:

const abi = [ "event Transfer(address indexed from, address indexed to, uint value)" ];
const provider = ethers.getDefaultProvider();

const contract = new ethers.Contract("dai.tokens.ethers.eth", abi, provider);

 const filter = contract.filters.Transfer(null, to);
console.log(filter);
// { address: 'dai.tokens.ethers.eth',
//  topics:
//   [ '0xddf252ad1be2c89b69c2b068fc378daa952ba7f163c4a11628f55a4df523b3ef',
//     null,
//     '0x000000000000000000000000a5407eae9ba41422680e2e00537571bcc53efbfd' ] }

@ricmoo
Copy link
Member

ricmoo commented Apr 28, 2020

(oh wiat... You are finding it happens during serialization... one sec)

@danielattilasimon
Copy link
Author

Exactly, if you call deserializeTopics("0x000000000000000000000000a94f5374fce5edbc8e2a8697c15331677e6ebf0b&null&0x000000000000000000000000a94f5374fce5edbc8e2a8697c15331677e6ebf0b|0x0000000000000000000000000aff3454fce5edbc8cca8697c15331677e6ebccc") you get my example.

@ricmoo ricmoo added bug Verified to be an issue. and removed investigate Under investigation and may be a bug. labels Apr 28, 2020
@ricmoo
Copy link
Member

ricmoo commented Apr 28, 2020

Perfect. Yes, I've reproduced it and verified it isn't a problem in a. few other places you had me worried about. ;)

@ricmoo
Copy link
Member

ricmoo commented Apr 28, 2020

I have merged the fix locally. I'm also pinging a few other people so I can convince myself why [ [ null ], A ] isn't the same as [ null, A ]. I need to make sure I haven't made other weird assumptions. :)

@danielattilasimon
Copy link
Author

Thanks so much for looking at this so quickly!

In my opinion the spec is too vague on the definition of topics. The best I can find is the note in https://github.com/ethereum/EIPs/blob/master/EIPS/eip-1474.md#parameters-34

Otherwise, topics is only defined as a Data[], when it should probably be something like (null | Data | Data[])[].

@ricmoo
Copy link
Member

ricmoo commented Apr 28, 2020

Yeah... I have the same page open now too. "Specification" might be too string a word. :)

I'm going to assume null can't be inside a data for now, until I hear back. A few outstanding questions I have are:

  1. Can [ [ A, B, null ], C ] be simplified to [ null, C ] (or is it invalid)?
  2. Can [ A, [ ], B ] be simplified to never-gonna-happen?

But I'll commit these changes for now until I hear back. :)

@ricmoo
Copy link
Member

ricmoo commented Apr 28, 2020

(also these are things I plan to address in the ethers docs, so after this research there is some place with the results ;))

ricmoo added a commit that referenced this pull request Apr 28, 2020
@danielattilasimon
Copy link
Author

Thanks again for such a quick turnaround! I see you've already published a new package with the fix included.

@ricmoo
Copy link
Member

ricmoo commented Apr 28, 2020

Yes, thanks! Let me know if it works for you. :)

Perfect PR, btw. You even named variables the exact same way I do. :)

@ricmoo ricmoo added fixed/complete This Bug is fixed or Enhancement is complete and published. and removed on-deck This Enhancement or Bug is currently being worked on. labels Apr 28, 2020
@danielattilasimon
Copy link
Author

Already integrated the new release, works perfectly!

@danielattilasimon
Copy link
Author

danielattilasimon commented Apr 29, 2020

After looking into the geth sources, I think the API weirdness comes down to how the data is represented by the backend, and the mechanics of JSON encoding in Go. I know very little about Go though, so I could be wrong.

Topics are typed as a [][]common.Hash, a slice of slices of hashes.
https://github.com/ethereum/go-ethereum/blob/5d7e5b00be3eb04a334366d78b6ab742772c8e0a/eth/filters/filter.go#L51-L62

In Go, nil functions as an empty slice:
https://tour.golang.org/moretypes/12

A nil slice is encoded as null when marshalling to JSON, and vice-versa:
https://golang.org/pkg/encoding/json/#Marshal
https://golang.org/pkg/encoding/json/#Unmarshal

The type [][]common.Hash represents an array of Hash-arrays. In this JSON representation:

  "topics": [
    ["0x000000000000000000000000a94f5374fce5edbc8e2a8697c15331677e6ebf0b"],
    [null],
    [
      "0x000000000000000000000000a94f5374fce5edbc8e2a8697c15331677e6ebf0b",
      "0x0000000000000000000000000aff3454fce5edbc8cca8697c15331677e6ebccc"
    ]
  ]

null (which will be translated to nil) stands in a position where a hash is expected. I haven't looked into the parsing of hashes, but common.Hash is a fixed-size array of bytes, so I'm assuming that a null/nil will be rejected.

On the other hand, this should work:

  "topics": [
    ["0x000000000000000000000000a94f5374fce5edbc8e2a8697c15331677e6ebf0b"],
    [],
    [
      "0x000000000000000000000000a94f5374fce5edbc8e2a8697c15331677e6ebf0b",
      "0x0000000000000000000000000aff3454fce5edbc8cca8697c15331677e6ebccc"
    ]
  ]

@danielattilasimon
Copy link
Author

Shall we close this now that it's fixed?

@ricmoo
Copy link
Member

ricmoo commented May 8, 2020

Oh yes! Thanks. :)

michaeltout pushed a commit to michaeltout/ethers.js that referenced this pull request Aug 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Verified to be an issue. fixed/complete This Bug is fixed or Enhancement is complete and published.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants