fix: execution status revert reason field is optional

[fracek]

The Starknet JSON-RPC spec defines this field as optional. This commit fixes compatibility with nodes like Juno that don't include this field if the revert reason is empty.

[xJonathanLEI]

While the spec does say it's optional, I figured that it simply meant that it's null when the transaction succeeds? Is there a legitimate case where the revert reason is empty when the transaction reverts?

[xJonathanLEI]

But in any case I like this PR adds compatability without breaking changes on the API surface. However, one part I'd like to see changed is how the new implementation no longer rejects the combination of a successful execution with revert reason - I believe this apparently an invalid combination.

[fracek]

While the spec does say it's optional, I figured that it simply meant that it's null when the transaction succeeds? Is there a legitimate case where the revert reason is empty when the transaction reverts?

Juno (both in RPC 0.4 and 0.5) doesn't return the field if the reason is empty. You can see that making the following call on mainnet.

{
	"id": 0,
	"jsonrpc": "2.0",
	"method": "starknet_getTransactionReceipt",
	"params": ["0x0527505db95eca337e3ad224ff3881e8565403f817fcbef1a2364c3ed054d11c"]
}

RPC 0.4

{
	"jsonrpc": "2.0",
	"result": {
		"type": "INVOKE",
		"transaction_hash": "0x527505db95eca337e3ad224ff3881e8565403f817fcbef1a2364c3ed054d11c",
		"actual_fee": "0x24143b000377",
		"execution_status": "REVERTED",
		"finality_status": "ACCEPTED_ON_L1",
		"block_hash": "0x405453ad0c33bd9515cfb39e268a247b908cc16c996abc99f5955e9143204e2",
		"block_number": 367818,
		"messages_sent": [],
		"events": [
			{
				"from_address": "0x49d36570d4e46f48e99674bd3fcc84644ddd6b96f7c741b1562b82f9e004dc7",
				"keys": [
					"0x99cd8bde557814842a3121e8ddfd433a539b8c9f14bf31ebf108d12e6196e9"
				],
				"data": [
					"0x6f5ccf18f9a80aca492ea25c50f9916fdd1db207d09c7a1bd539b3c1303ce12",
					"0x1176a1bd84444c89232ec27754698e5d2e7e1a7f1539f12027f28b23ec9f3d8",
					"0x24143b000377",
					"0x0"
				]
			}
		]
	},
	"id": 0
}

RPC 0.5

{
	"jsonrpc": "2.0",
	"result": {
		"type": "INVOKE",
		"transaction_hash": "0x527505db95eca337e3ad224ff3881e8565403f817fcbef1a2364c3ed054d11c",
		"actual_fee": "0x24143b000377",
		"execution_status": "REVERTED",
		"finality_status": "ACCEPTED_ON_L1",
		"block_hash": "0x405453ad0c33bd9515cfb39e268a247b908cc16c996abc99f5955e9143204e2",
		"block_number": 367818,
		"messages_sent": [],
		"events": [
			{
				"from_address": "0x49d36570d4e46f48e99674bd3fcc84644ddd6b96f7c741b1562b82f9e004dc7",
				"keys": [
					"0x99cd8bde557814842a3121e8ddfd433a539b8c9f14bf31ebf108d12e6196e9"
				],
				"data": [
					"0x6f5ccf18f9a80aca492ea25c50f9916fdd1db207d09c7a1bd539b3c1303ce12",
					"0x1176a1bd84444c89232ec27754698e5d2e7e1a7f1539f12027f28b23ec9f3d8",
					"0x24143b000377",
					"0x0"
				]
			}
		],
		"execution_resources": {
			"steps": "0x0",
			"memory_holes": "0x0",
			"pedersen_builtin_applications": "0x0",
			"range_check_builtin_applications": "0x0",
			"bitwise_builtin_applications": "0x0",
			"ecdsa_builtin_applications": "0x0",
			"ec_op_builtin_applications": "0x0",
			"keccak_builtin_applications": "0x0",
			"poseidon_builtin_applications": "0x0"
		}
	},
	"id": 0
}

But in any case I like this PR adds compatability without breaking changes on the API surface. However, one part I'd like to see changed is how the new implementation no longer rejects the combination of a successful execution with revert reason - I believe this apparently an invalid combination.

True. I'm concerned that at some point in the future there will be a client that sends a successful status with an empty error message (because why not?) since it's technically following the spec. Happy to change the code if you think the risk is too low.

[xJonathanLEI]

Thanks for he update. I guess what I meant was that whether this is a legit case or simply a bug in Juno. I just checked pathfinder for the same transaction and it returns "" for revert_reason, so I guess there's indeed a legit case where reason is missing even when reverted.

I'm concerned that at some point in the future there will be a client that sends a successful status with an empty error message (because why not?) since it's technically following the spec.

On the other hand, I'm concerned that clients & SDKs that are too forgiving lead to wrong impls (see #490 for example), so I prefer enforcing more rules to start, and if things turn out to unfortunately be against us we can always change to loosen those (see #491).

[xJonathanLEI]

LGTM