search

celo-org / op-geth

GNU Lesser General Public License v3.0
0 stars 0 forks source link

Failing test: `TestSignTransaction` #117

Closed palango closed 1 month ago

palango commented 2 months ago

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 commented 2 months ago

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.

https://github.com/celo-org/op-geth/blob/b510efe282ea4e8bfa11cfc84917644f75d0bdeb/core/types/transaction_marshalling.go#L62

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 commented 1 month ago

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 commented 1 month ago

This has been merged in the celo5 branch.