Failing test: TestSignTransaction

[palango]

After a rebase (celo5) the new test TestSignTransaction fails with:

--- FAIL: TestSignTransaction (0.00s)
   ./op-geth/internal/ethapi/api_test.go:1176: result mismatch. Have:
        {"type":"0x2","chainId":"0x1","nonce":"0x0","to":"0x703c4b2bd70c169f5717101caee543299fc946c7","gas":"0x5208","gasPrice":null,"maxPriorityFeePerGas":"0x0","maxFeePerGas":"0x684ee180","value":"0x1","input":"0x","accessList":[],"v":"0x0","r":"0x8fabeb142d585dd9247f459f7e6fe77e2520c88d50ba5d220da1533cea8b34e1","s":"0x582dd68b21aef36ba23f34e49607329c20d981d30404daf749077f5606785ce7","yParity":"0x0","hash":"0x93927839207cfbec395da84b8a2bc38b7b65d2cb2819e9fef1f091f5b1d4cc8f","feeCurrency":null}
        Want:
        {"type":"0x2","chainId":"0x1","nonce":"0x0","to":"0x703c4b2bd70c169f5717101caee543299fc946c7","gas":"0x5208","gasPrice":null,"maxPriorityFeePerGas":"0x0","maxFeePerGas":"0x684ee180","value":"0x1","input":"0x","accessList":[],"v":"0x0","r":"0x8fabeb142d585dd9247f459f7e6fe77e2520c88d50ba5d220da1533cea8b34e1","s":"0x582dd68b21aef36ba23f34e49607329c20d981d30404daf749077f5606785ce7","yParity":"0x0","hash":"0x93927839207cfbec395da84b8a2bc38b7b65d2cb2819e9fef1f091f5b1d4cc8f"}

The problem is that "feeCurrency":null gets added to the json-ified version of the transaction, even though it shouldn't.

See also #109

[ezdac]

So I looked into this, and this doesn't have to do anything with the transaction-args issue (#109) but with the JSON schema definition.

op-geth/core/types/transaction_marshalling.go

Line 62 in b510efe

FeeCurrency *common.Address `json:"feeCurrency"` // nil means native currency

The FeeCurrency field does not have a omitempty JSON tag, so the key will always be explicitly included in the schema, even if it's nil - this is the same for all transaction types, not only 0x7b.
This is the same behavior as in the celo-blockchain implementation.

So I think for all transaction types except for 0x7b I would consider this unexpected behavior and the schema should not include celo-related fields.
The question is how should this behave for 0x7b type transactions? Should the field be omitted or not when the value is nil?

[ezdac]

The question is how should this behave for 0x7b type transactions? Should the field be omitted or not when the value is nil?

@palango and I decided to just include a ,omitempty tag for all transaction types for now, while we agree that the field should not be omitted when nil for the Celo transaction types. For now, we accept the intermediary state of omitting it, since implementing this feature would require significantly more code-changes to accommodate for the different encoding schemas.

[ezdac]

This has been merged in the celo5 branch.