Test that API Receipts can Unmarshal to types.Receipts

[axelKingsley]

When creating Receipts to return over API, a one-off []map[string]interface{} structure is defined to contain the returnable fields. These structures are tested by marshaling individual receipts and comparing to an expected content from JSON test-files.

Separately, in core, types.Receipt structures are used to represent Transaction Receipts. These are tested by marshaling and unmarshalling sample structures.

However, there isn't testing which links these two structures. It is possible that a given version of geth might emit Receipts over its API which can't be unmarshalled by that same version of geth. If ever there is a change to either the API structure or the types.Receipt structure, this incompatibility may occur. When it does, there should exist testing which exposes this.

This change simply adds a round-trip marshal-unmarshal from the API structure to a types.Receipts. This confirms that the emitted API structure is compatible as a types.Receipts.

Testing

Tested this manually in the following ways:

• Added a new field to types.Receipt which is required by the codec. Observed that the API test previously would pass (because the API structure matches its own expected output), but with the new testing this now fails, highlighting the missing field.
• Changed the type of an existing field in the API. Existing checks against the expected-JSON file failed, but once that was updated, this new check is required to show that the type is not suitable for types.Receipt
• Added additional fields to the API structure. Existing checks against the expected-JSON file failed, but once that was updated, this new check is required to show that the type is not expected for types.Receipt

Reasoning

Geth's own API should be compatible with its ethereum spec definition of structures. Therefore, while these structures are separately managed, it makes sense to include testing which confirms their own internal compatibility.

OP would have benefitted from this testing when we added new L2 fields in this PR: 3653ceb . The fields were already defined in the types.Receipt structure, but they were still big.Int, meaning this API structure is not compatible with its own Receipt. (this was later fixed in subsequent PR 6b2bf0f)

[sebastianst]

I think it would be nicer to get access to the golden file content directly, and unmarshal the file content into the receipt struct. You can then marshal the receipt struct again into an indented JSON string, and assert that the marshaled receipt matches the golden file content.

For that, you could add an additional any parameter to testRPCResponseWithFile into which you unmarshal and at the calling site you instantiate it, like

Suggested change

	testRPCResponseWithFile(t, i, result, "eth_getBlockReceipts", tt.file)
	rec := new(types.Receipt)
	testRPCResponseWithFile(t, i, result, "eth_getBlockReceipts", tt.file, rec)

this would also work for the other callers of testRPCResponseWithFile.

[axelKingsley]

I've taken your advice and made the expected type part of the testRPCResponseWithFile parameters, so it can confirm unmarshalling.

Interestingly, however, when I added the check you mention here, that the type.Whatever marshals back to the same as the "golden file content", The TestRPCGetBlockOrHeader tests fail!

        	Messages:   	test 0: json does not match type struct, want:[...]
        	            	Diff:
        	            	--- Expected
        	            	+++ Actual
        	            	@@ -1,4 +1,6 @@
        	            	-(map[string]interface {}) (len=18) {
        	            	+(map[string]interface {}) (len=21) {
        	            	  (string) (len=13) "baseFeePerGas": (string) (len=9) "0xfdc7303",
        	            	+ (string) (len=11) "blobGasUsed": (interface {}) <nil>,
        	            	  (string) (len=10) "difficulty": (string) (len=7) "0x20000",
        	            	+ (string) (len=13) "excessBlobGas": (interface {}) <nil>,
        	            	  (string) (len=9) "extraData": (string) (len=2) "0x",
        	            	@@ -12,2 +14,3 @@
        	            	  (string) (len=6) "number": (string) (len=3) "0xa",
        	            	+ (string) (len=21) "parentBeaconBlockRoot": (interface {}) <nil>,
        	            	  (string) (len=10) "parentHash": (string) (len=66) "0xedb9ccf3a85f67c095ad48abfb0fa09d47179bb0f902078d289042d12428aca5",
        	            	@@ -17,4 +20,4 @@
        	            	  (string) (len=9) "timestamp": (string) (len=4) "0x64",
        	            	- (string) (len=15) "totalDifficulty": (string) (len=3) "0x1",
        	            	- (string) (len=16) "transactionsRoot": (string) (len=66) "0xb0893d21a4a44dc26a962a6e91abae66df87fb61ac9c60e936aee89c76331445"
        	            	+ (string) (len=16) "transactionsRoot": (string) (len=66) "0xb0893d21a4a44dc26a962a6e91abae66df87fb61ac9c60e936aee89c76331445",
        	            	+ (string) (len=15) "withdrawalsRoot": (interface {}) <nil>
        	            	 }

I've updated this PR to this version which has this issue. I'll have to look at it in the morning, but to me it indicates that not all type objects will look the same as their original API counterparts once unmarshalled+marshalled.

[sebastianst]

I see. Looking at the definition of types.Header, no struct field has omitempty set in their json tag, so missing fields are marshaled as nulls instead of omitted. It's probably an inconsistency that has just never been detected (or just ignored) because such a test didn't exist before.

totalDifficulty is a special case that's added by the API server that's not part of the Header struct, see BlockChainAPI.rpcMarshalHeader and rpcMarshalBlock.

I guess we could just skip headers and blocks for now by passing nil as unmarshalTo and track solving this, possibly by raising a PR upstream to add omitempty to those fields (and add special treatment for field totalDifficulty). We could actually also ask upstream why there even are these manual rpcMarshal... functions, if the same can be achieved with properly configured json Go field tags. The existing tests would guarantee that json marshaling produces the expected results.

[axelKingsley]

Agreed, I've added nil to the Header/Block test

[BlocksOnAChain]

@axelKingsley is this one good to be closed or it's still under reviews?