Cannot map ENUM to JSON... submitAndWatchExtrinsic error

[TarikGul]

rel: https://substrate.stackexchange.com/questions/1680/drr-createtypeextrinsicstatus-cannot-map-enum-json-unable-to-find-reason

Given the issue in stackexchange I put together a small reproduction repo to clarify and track the issue. The issue in stackexchange is reproducible when used against this branch (dp-jsonrpsee-integration-2) in substrate.

When we use the small script above against dp-jsonrpsee-integration-2, we are able to reproduce:

DRR: createType(ExtrinsicStatus):: Cannot map Enum JSON, unable to find 'reason' in future, ready, broadcast, inblock, retracted, finalitytimeout, finalized, usurped, dropped, invalid
Error: createType(ExtrinsicStatus):: Cannot map Enum JSON, unable to find 'reason' in future, ready, broadcast, inblock, retracted, finalitytimeout, finalized, usurped, dropped, invalid

In the reproduction above, in any normal circumstance we would expect the following error since we are passing in a empty tx:

RPC-CORE: submitAndWatchExtrinsic(extrinsic: Extrinsic): ExtrinsicStatus:: 1011: Unknown Transaction Validity: NoUnsignedValidator
2022-04-07 11:16:55             DRR: 1011: Unknown Transaction Validity: NoUnsignedValidator

cc: @niklasad1

[niklasad1]

To elaborate what is happening here is that jsonrpc waits on sending the subscriptionID after the actual bytes has been decoded as an valid extrinsic and jsonrpsee actually regards the subscription has valid because the bytes were decode-able as Bytes according to rpc definition

Then after decoding the bytes as extrinsic fails jsonrpsee sends its own custom message SubscriptionClosedReason as subscription notification (as the rpc call has already been accepted as a valid subscription) which is serialized to

{"reason": {"Server": "Runtime error: Execution failed: Error calling api function: Failed to convert parameter `tx` from node to runtime of validate_transaction"}}

It's rather a matter if we should send out TransactionStatus::Invalid or keep the SubscriptionClosedReason for p.js to decode

Thoughts?

[jacogr]

Despite the fact that this is JSON and anything goes, i.e. unstructured, we really should keep to actual responses as defined by the JSONRPC spec. There is no "error" field in here as per JSONRPC, which would indicate an error. Since it is "success" it expects to decode an actual response enum.

In the above case, I would suggest sending an actual JSONRPC error response back. In the simplest form -

{
  "id": 1,
  "error": {
    "message": "Runtime error: Execution failed: Error calling api function: Failed to convert parameter `tx` from node to runtime of validate_transaction",
    "code": -32603
  },
  "jsonrpc": "2.0"
}

[niklasad1]

I agree that the ideal solution would be to send back an actual JSON-RPC error object.

However, as I tried to explain it's not possible to do what you suggested in jsonrpsee with current API. Once the type in JSON-RPC API is decoded successfully the "JSON-RPC method call for the subscription" will be answered with a subscriptionID by the library and it's not possible to reject any call at that point.

To illustrate how this works (jsonrpc vs jsonrpsee):

jsonrpsee

{"jsonrpc":"2.0","id":0,"method":"author_submitAndWatchExtrinsic","params":["0x0"]}
{"jsonrpc": "2.0", "result": "xaxoFVNpNQ6gjJtV", "id": 0}
{"jsonrpc": "2.0", "method": "author_extrinsicUpdate", "params": {"subscription": "xaxoFVNpNQ6gjJtV", "result": {"reason":{"Server": "Runtime error: Execution failed: Error calling api function: Failed to convert parameter `tx` from node to runtime of validate_transaction"}}}

jsonrpc

{"jsonrpc":"2.0","id":0,"method":"author_submitAndWatchExtrinsic","params":["0x0"]}
{"jsonrpc":"2.0","error":{"code":1002,"message":"Verification Error: Runtime error: Execution failed: Error calling api function: Failed to convert parameter `tx` from node to runtime of validate_transaction","data":"Runtime error: Execution failed: Error calling api function: Failed to convert parameter `tx` from node to runtime of validate_transaction"},"id":0}

So, my question is really if would be okay for us to send back TransactionStatus::Invalid (that would probably work with p.js but as user you would not known why it was rejected....) or send over our own message with the reason field (but that needs p.js fix)?

We thought this approach would be a bit nicer to use but not as flexible obviously.

[jacogr]

This is not a Polkadot-js fix, this is something that needs to be adjusted in the: JS libs, Python libs, Java libs, Go libs, C++ libs, Ruby libs, mobile libs (it is early and pre-coffee, I forgot the language for Fearless/Nova), Rust libs ... all SDKs.

If the question is if the subscription result can return correctly-formatted JSONRPC error, then yes. I'm not sure all SDKs handle this correctly, but it is certainly something that should be valid as I understand the JSONRPC structure and should be handled out of the box (at least in JS), aka

{
  "jsonrpc": "2.0",
  "method": "author_extrinsicUpdate",
  "params": {
     "subscription": "xaxoFVNpNQ6gjJtV",
     "error": {
        "message": "Runtime error: Execution failed: Error calling api function: Failed to convert parameter `tx` from node to runtime of validate_transaction",
        "code": 1002
    }
  }
}

The reason why I'm saying that SDKs may not handle this correctly is that at least in my realm the handling is there, but not 100% sure I've seen this out there in the wild. (If not handled correctly, it is indeed a fix where it is not)

Anyway, the Invalid status is something that is in the enum for those responses, so I can see why it is send in the current case. You can also update that enum to contain a new status for this formatting, not sure "reason" is descriptive enough, so to adjust the expected outcome from submitAndWatch -

enum RpcFailure {
	Server: { message: String }
}

pub enum TransactionStatus<Hash, BlockHash> {
	/// Transaction is part of the future queue.
	Future,
	/// Transaction is part of the ready queue.
	Ready,
	....
	/// Transaction is no longer valid in the current state.
	Invalid,
	/// Transaction failed on RPC
	Failed(RpcFailure)
}

SDKs should adjust their enums if something like the above is added, but it won't be the first time they need to align types. I'm just worried that the same would appear on other calls as well, so just the extrinsic status enum adjustment is probably not enough and the wrong place to do this since it only deals with extrinsics. Hence the error case is probably the generic solution for all as per the standard.

[niklasad1]

This is not a Polkadot-js fix, this is something that needs to be adjusted in the: JS libs, Python libs, Java libs, Go libs, C++ libs, Ruby libs, mobile libs (it is early and pre-coffee, I forgot the language for Fearless/Nova), Rust libs ... all SDKs.

That's a good point Jaco, I didn't really think about that.

If the question is if the subscription result can return correctly-formatted JSONRPC error, then yes. I'm not sure all SDKs handle this correctly, but it is certainly something that should be valid as I understand the JSONRPC structure and should be handled out of the box (at least in JS), aka

Cool, TIL that looks quite neat.
We should adopt that then, then we don't need the extra pattern Failed in JS/TS context at least?

SDKs should adjust their enums if something like the above is added, but it won't be the first time they need to align types. I'm just worried that the same would appear on other calls as well, so just the extrinsic status enum adjustment is probably not enough and the wrong place to do this since it only deals with extrinsics. Hence the error case is probably the generic solution for all as per the standard.

I'm not completely sure but the extrinsic is an "edge-base" because it's the scale decoding that failed not really that the JSON-RPC params that were invalid but I fear that somebody could pass in a "non existing storage key" in the storage subscriptions to trigger similar behavior. So if you are okay with the generic error notification that you suggested I think we should go with that.

[jacogr]

Sorry about the close. Finger slip on the phone.

[jacogr]

The params -> error indeed looks like the JSONRPC standard, so makes sense.

[niklasad1]

This is not relevant anymore we have changed to API to be able to reject subscriptions similar what the jsonrpc crate does but we have kept the error notifications/close notifications that you suggested Jaco thanks.

However, those not used in substrate anyway so should not be relevant anyway.

@TarikGul can you close this? 🙏

[polkadot-js-bot]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue if you think you have a related problem or query.