extended CompilationFailed error(s) #2464
Conversation
crates/rpc/src/error.rs
Outdated
| @@ -202,7 +202,9 @@ impl ApplicationError { | |||
| ApplicationError::InsufficientResourcesForValidate => None, | |||
| ApplicationError::InsufficientAccountBalance => None, | |||
| ApplicationError::ValidationFailure => None, | |||
| ApplicationError::CompilationFailed => None, | |||
| ApplicationError::CompilationFailed { data } => Some(json!({ | |||
| "data": data, | |||
There was a problem hiding this comment.
I think this "data": is redundant here. The JSON value returned by the .data() method is already wrapped in "data": in the JSON-RPC result.
And I'm not actually sure this is compliant with the specification: COMPILATION_FAILED doesn't have its data specified in the OpenRPC specification.
There was a problem hiding this comment.
Ah, OK, so this will eventually be valid after this PR is merged into the 0.8 specification. Then we should maybe make this change only for the 0.8 JSON-RPC API so that we don't break JSON-RPC 0.7?
There was a problem hiding this comment.
I think this
"data":is redundant here. The JSON value returned by the.data()method is already wrapped in"data":in the JSON-RPC result.
Well yes, but this is implementing the .data() method... Anyway, I just went by analogy with the other cases having data (i.e. GatewayError and TransactionExecutionError)...
Also, json!({ data }) doesn't compile - "missing tokens in macro arguments"... Or do you mean serde_json::Value::String(data.to_string()) ?
There was a problem hiding this comment.
I think this
"data":is redundant here. The JSON value returned by the.data()method is already wrapped in"data":in the JSON-RPC result.Well yes, but this is implementing the
.data()method... Anyway, I just went by analogy with the other cases having data (i.e.GatewayErrorandTransactionExecutionError)...Also,
json!({ data })doesn't compile - "missing tokens in macro arguments"... Or do you meanserde_json::Value::String(data.to_string())?
Yes, I meant json!(data).
crates/rpc/src/error.rs
Outdated
| RpcVersion::V08 => Some(json!({ | ||
| "data": data, | ||
| })), | ||
| _ => None, |
There was a problem hiding this comment.
BTW: considering the introduction of the next JSON-RPC version (if ever) I think we should special-case the V07 variant instead and default to including data. That way we'd automatically get the data for the next version.
vbar
force-pushed
the
vbar/return-compilation-error
branch
from
23a43c2 to
26b2395
Compare
Returning Starknet compilation failure error message.
Fixes #2462 .