ethereum,ethclient,eth/filters: marshal/unmarshal ethereum.FilterQuery struct to/from JSON correctly

[zenovich]

• Marshal ethereum.FilterQuery struct to JSON correctly,
• Treat -1 as "latest" and -2 as "pending" (use rules from the rpc package),
• Unmarshal ethereum.FilterQuery from JSON applying defaults for missing optional fields,
• Use this implementation of marshalling in ethclient and eth/filters

[fjl]

We cannot make this change because it is an API change. The FilterQuery struct cannot be modified.

If we want to add JSON marshaling for this type, we need to add MarshalJSON/UnmarshalJSON methods instead. The code for these methods already exists in other places in go-ethereum. For example, package ethclient contains a function that marshals FilterQuery for use with JSON-RPC.

[zenovich]

No problem, @fjl. I'll rework that.

By the way, there is a question. The Ethereum Wiki (https://eth.wiki/json-rpc/API#parameters-45) says that if 'fromBlock' is missing it is treated as 'latest' while ethclient.toFilterArg() treats FromBlock==nil as 0x0 (i.e. "earliest") (see https://github.com/ethereum/go-ethereum/blob/master/ethclient/ethclient.go#L406). Which path should I take?

(If you don't mind, I'd suggest not to reconstruct missing optional fields on marshalling, but apply default values in UnmarshalJSON() instead. This would probably make things easier.)

Also, there is an inconsistency between ethclient.toBlockNumArg() (https://github.com/ethereum/go-ethereum/blob/master/ethclient/ethclient.go#L527) and the 'rpc' package constants (https://github.com/ethereum/go-ethereum/blob/master/rpc/types.go#L60): ethclient.toBlockNumArg() treats -1 as "pending" and -2 as -2 while the rpc package treats -1 as "latest" and -2 as "pending". Not sure which one is correct.

[fjl]

By the way, there is a question. The Ethereum Wiki (https://eth.wiki/json-rpc/API#parameters-45) says that if 'fromBlock' is missing it is treated as 'latest' while ethclient.toFilterArg() treats FromBlock==nil as 0x0 (i.e. "earliest") (see https://github.com/ethereum/go-ethereum/blob/master/ethclient/ethclient.go#L406). Which path should I take?

It's documented in package ethereum that FromBlock == nil means genesis block, so we shouldn't change it.

Also, there is an inconsistency between ethclient.toBlockNumArg() and the 'rpc' package constants : ethclient.toBlockNumArg() treats -1 as "pending" and -2 as -2 while the rpc package treats -1 as "latest" and -2 as "pending". Not sure which one is correct.

This is unfortunate, and it's my fault. In the PR that introduced it (#21177), I suggested using -1 instead of allowing all negative numbers to mean 'pending'. I did not cross-check with package rpc at that time.

I'll think about how to fix this.

[zenovich]

By the way, there is a question. The Ethereum Wiki (https://eth.wiki/json-rpc/API#parameters-45) says that if 'fromBlock' is missing it is treated as 'latest' while ethclient.toFilterArg() treats FromBlock==nil as 0x0 (i.e. "earliest") (see https://github.com/ethereum/go-ethereum/blob/master/ethclient/ethclient.go#L406). Which path should I take?

It's documented in package ethereum that FromBlock == nil means genesis block, so we shouldn't change it.

Also, there is an inconsistency between ethclient.toBlockNumArg() and the 'rpc' package constants : ethclient.toBlockNumArg() treats -1 as "pending" and -2 as -2 while the rpc package treats -1 as "latest" and -2 as "pending". Not sure which one is correct.

This is unfortunate, and it's my fault. In the PR that introduced it (#21177), I suggested using -1 instead of allowing all negative numbers to mean 'pending'. I did not cross-check with package rpc at that time.

I'll think about how to fix this.

Fixed

[zenovich]

@fjl, can we merge please?

[fjl]

Sorry this is taking so long. I have been reviewing this on and off between other tasks, and will change some things before we can get this in.