EthereumTransactionReceiptObject cannot always be decoded

[pixelmatrix]

I'm doing a transaction and then calling web3.eth.getTransactionReceipt(hash) and I found a couple of issues.

1. It seems that most of the time the EthereumTransactionReceiptObject can't be decoded because it's assuming EthereumLogObject always has the removed property. I'm not sure why, but i'm not seeing that value coming back in the logs. Changing that property to an optional value causes it to work for me.

2. The Promise extension for this method uses seal.resolve(rpc.result, rpc.error). When the rpc response includes a null result, it ends up calling 1seal.resolve(nil, nil)`, which PromiseKit views as an error since it can't infer if it was fulfilled or rejected. As a workaround for now, i'm just allowing that to return the PMKError, and catching on that, but it's not actually an error. Not a huge issue, but just thought i'd highlight that strange behavior.

[koraykoska]

Ok thats really weird. The RPC documentation states the following about the removed tag:

removed: TAG - true when the log was removed, due to a chain reorganization. false if its a valid log.

I actually thought it has to be either true or false. How are people supposed to handle the nil case? Do we assume it to be false if it is nil or is it a special case?

About 2: You are right that's an issue if decoding doesn't work as intended. Maybe we should fail in that case and send an appropriate error to the promise?

[koraykoska]

Btw, logIndex in EthereumLogObject should also be an optional. The documentation states the following:

logIndex: QUANTITY - integer of the log index position in the block. null when its pending log.

[pixelmatrix]

Yeah, it seems the spec is not quite clear about optional keys. I think it's safe to say if it's excluded it should be false, but I do wish there was a more precise spec there. I came across some discussion about that key here: https://ethereum.stackexchange.com/questions/7905/ethereum-event-log-removed-field

Another possibility is that there is a difference in how Parity and geth nodes report the logs. Our infrastructure uses both to support all the test nets.

If you're alright with making those fields optional, i'd be happy to do a PR.

[koraykoska]

Yes we definitely want to support both parity and geth so just do it. It's still a pity that so many fields are optional though...

[pixelmatrix]

Do you prefer to keep removed as not optional and have a default value of false, or make it optional to match the json representation?

[koraykoska]

I would make it optional until we know for sure that the nil case actually means false. Until then people should decide themselves how to deal with that case.

[pixelmatrix]

Closing now that #21 is merged