feat: add eth_sendRawTransaction client support#467
Conversation
eth_send RawTransaction client support
eth_send RawTransaction client supporteth_sendRawTransaction client support
| SendRawTransactionStatus::Ok(maybe_hash) => { | ||
| Ok(maybe_hash.map(alloy_primitives::B256::from)) | ||
| } | ||
| error => Err(RpcError::JsonRpcError(JsonRpcError { |
There was a problem hiding this comment.
@gregorydemay I noticed that we do some extra parsing for the result of eth_sendRawTransaction and return a non-API result (i.e. some JSON-RPC errors get converted to an Ok(SendRawTransactionStatus)). Obviously alloy doesn't have an equivalent type and normally one would just get the raw JSON RPC error.
IIUC the JSON-RPC processing is done to avoid getting inconsistent results due to differing error codes/messages between providers. However, I think it would be also OK to return the canonicalized errors still as a JSON-RPC error, just with a standardized error message and code.
In terms of error canonicalization, I think the exact message is not very important, but the code is less clear to me. Most providers return something along the lines of -32_000 (see here), and according to the JSON-RPC spec it's the beginning of the "Server Error" range, so IMO that could be a reasonable value.
Another direction, would be to add a new optional flag to the RPC config for eth_sendRawTransaction that disables the error processing for that call. The problem there is that we would then again get inconsistent results due to the different providers' differing error messages/codes...
Finally, we could either create a new SendRawTransactionStatus where the Ok variant contains a B256 or return a Result<Option<B256>,SendRawTransactionError> where SendRawTransactionError is an enum with variants for InsufficientFunds, NonceTooLow and NonceTooHigh. This would be probably the simplest option, and I'm not a fan of returning a JSON error wrapped in an Ok but this is closest to the Candid interface.
WDYT?
There was a problem hiding this comment.
- I think the OK return type should be
B256and notOption<B256>. The reason why the canister return an optional is because the RLP decoding may fail and we want to be lenient and still send the transaction. However, here with the client we will start by a transaction built with alloy so we should always be able to compute that transaction hash, no? - I think the error back in a JSON-RPC error is not a bad idea and matches what you would get when using alloy directly.
There was a problem hiding this comment.
I think the OK return type should be B256 and not Option. The reason why the canister return an optional is because the RLP decoding may fail and we want to be lenient and still send the transaction. However, here with the client we will start by a transaction built with alloy so we should always be able to compute that transaction hash, no?
Ah I see, that makes sense. I've changed the return type to be B256 and not an Option<B256>. I think I got confused because it seems some providers sometimes return a null transaction hash if there was an error, but it seems that's not really API-compliant and is also not consistent with alloy.
| /// .build(); | ||
| /// | ||
| /// let result = client | ||
| /// .send_raw_transaction(bytes!("0xf86c098504a817c800825208943535353535353535353535353535353535353535880de0b6b3a76400008025a028ef61340bd939bc2195fe537567866003e1a15d3c71ff63e1590620aa636276a067cbe9d8997f761aecb703304b3800ccf555c9f3dc64214b297fb1966a3b6d83")) |
There was a problem hiding this comment.
For a follow-up PR: here we could build an actual transaction with alloy and sign it with t-sig. The example then becomes rather long though, especially with signing. Some ideas:
- add a helper function (similar to the one for the SOL RPC client) for signing
alloytransactions - add support for
eth_sendTransactionwhere we wouldn't need to do the transaction serialization
19211ac to
97579c1
Compare
48e6f1e to
a17d9ff
Compare
a17d9ff to
0db5cc2
Compare
gregorydemay
left a comment
There was a problem hiding this comment.
Thanks @lpahlavi for this PR and sorry for the delay in reviewing it. Overall LGTM, only some minor comments
| /// .build(); | ||
| /// | ||
| /// let result = client | ||
| /// .send_raw_transaction(bytes!("0xf86c098504a817c800825208943535353535353535353535353535353535353535880de0b6b3a76400008025a028ef61340bd939bc2195fe537567866003e1a15d3c71ff63e1590620aa636276a067cbe9d8997f761aecb703304b3800ccf555c9f3dc64214b297fb1966a3b6d83")) |
| #[tokio::test] | ||
| async fn eth_send_raw_transaction_should_succeed() { | ||
| fn mock_response() -> JsonRpcResponse { | ||
| JsonRpcResponse::from(json!({ "id": 0, "jsonrpc": "2.0", "result": "Ok" })) |
There was a problem hiding this comment.
I'm confused by this response, the response in the happy case should actually contain the transaction hash (according to the spec), no?
There was a problem hiding this comment.
Yes... I actually got a bit confused here initially. It seems we compute the hash of the transaction in the request arguments, and return that hash, not the hash received in the JSON-RPC response. See here. I'm not actually sure I understand the rationale, but regardless the end result is that the transaction hash in the JSON-RPC result is ignored.
There was a problem hiding this comment.
I actually got a bit confused here initially. It seems we compute the hash of the transaction in the request arguments, and return that hash, not the hash received in the JSON-RPC response.
Yes, that's because of replicated calls: it's actually expected when you send a transaction that 1 out of 34 makes it and the other replicas receive an error along the line transaction already known so that the result after consensus will be transaction already known. For that reason we compute the transaction hash before hand.
Nonetheless I think the mocked response should match the Ethereum JSON-RPC spec. Could you modify it accordingly?
There was a problem hiding this comment.
Ah, the missing step of the puzzle was that i had missed the conversion from SendRawTransactionError::AlreadyKnown to SendRawTransactionResult::Ok here.
I was a bit quick and merged this PR but I added it in #467 in this commit.
| SendRawTransactionStatus::Ok(maybe_hash) => { | ||
| Ok(maybe_hash.map(alloy_primitives::B256::from)) | ||
| } | ||
| error => Err(RpcError::JsonRpcError(JsonRpcError { |
There was a problem hiding this comment.
- I think the OK return type should be
B256and notOption<B256>. The reason why the canister return an optional is because the RLP decoding may fail and we want to be lenient and still send the transaction. However, here with the client we will start by a transaction built with alloy so we should always be able to compute that transaction hash, no? - I think the error back in a JSON-RPC error is not a bad idea and matches what you would get when using alloy directly.
lpahlavi
left a comment
There was a problem hiding this comment.
Thanks a lot for the review @gregorydemay! Should be ready for another round 🚀
| SendRawTransactionStatus::Ok(maybe_hash) => { | ||
| Ok(maybe_hash.map(alloy_primitives::B256::from)) | ||
| } | ||
| error => Err(RpcError::JsonRpcError(JsonRpcError { |
There was a problem hiding this comment.
I think the OK return type should be B256 and not Option. The reason why the canister return an optional is because the RLP decoding may fail and we want to be lenient and still send the transaction. However, here with the client we will start by a transaction built with alloy so we should always be able to compute that transaction hash, no?
Ah I see, that makes sense. I've changed the return type to be B256 and not an Option<B256>. I think I got confused because it seems some providers sometimes return a null transaction hash if there was an error, but it seems that's not really API-compliant and is also not consistent with alloy.
| #[tokio::test] | ||
| async fn eth_send_raw_transaction_should_succeed() { | ||
| fn mock_response() -> JsonRpcResponse { | ||
| JsonRpcResponse::from(json!({ "id": 0, "jsonrpc": "2.0", "result": "Ok" })) |
There was a problem hiding this comment.
Yes... I actually got a bit confused here initially. It seems we compute the hash of the transaction in the request arguments, and return that hash, not the hash received in the JSON-RPC response. See here. I'm not actually sure I understand the rationale, but regardless the end result is that the transaction hash in the JSON-RPC result is ignored.
## 🤖 New release * `evm_rpc_types`: 2.1.0 -> 3.0.0 (✓ API compatible changes) * `evm_rpc_client`: 0.1.0 * `evm_rpc`: 2.5.0 -> 2.6.0 (✓ API compatible changes) <details><summary><i><b>Changelog</b></i></summary><p> ## `evm_rpc_types` <blockquote> ## [3.0.0] - 2025-10-20 ### Added - Add conversions between several types in this crate and the corresponding `alloy` types. These conversions are only available with the `alloy` feature ([#465](#465), [#466](#466), [#467](#467), [#476](#476)). - **Breaking:** Add `root` and `cumulativeGasUsed` fields to `TransactionsReceipt` type ([#474](#474)) [3.0.0]: https://github.com/dfinity/evm-rpc-canister/compare/evm_rpc_types-v2.1.0..evm_rpc_types-v3.0.0 </blockquote> ## `evm_rpc_client` <blockquote> ## [0.1.0] - 2025-10-20 ### Added - Add methods to modify RPC config to `RequestBuilder` ([#494](#494)) - Add `alloy` feature flag to `evm_rpc_client` ([#484](#484)) - Add new `json_request` endpoint ([#477](#477)) - Add client support for `eth_getTransactionReceipt` ([#476](#476)) - Add `eth_sendRawTransaction` client support ([#467](#467)) - Add client support for `eth_call` ([#466](#466)) - Add client support for `eth_getTransactionCount` ([#465](#465)) - Add support for `eth_feeHistory` to client ([#460](#460)) - Add support for `eth_getBlockByNumber` to client ([#459](#459)) - Add EVM RPC canister client ([#447](#447)) [0.1.0]: https://github.com/dfinity/evm-rpc-canister/releases/tag/evm_rpc_client-v0.1.0 </blockquote> ## `evm_rpc` <blockquote> ## [2.6.0] - 2025-10-20 ### Added - Add support for `root` and `cumulativeGasUsed` fields in `eth_getTransactionReceipt` response ([#474](#474)) - Add new `json_request` endpoint and deprecate existing `request` endpoint ([#477](#477)) ### Changed - Update `ic-cdk` to `v0.18.7` ([#489](#489)) - Update `dfx` to `v0.29.0` ([#490](#490)) - Cleanup unused dependencies ([#491](#491)) ### Removed - **Breaking**: Remove `getMetrics` endpoint, this is acceptable since it was a debugging endpoint ([#479](#479)) ### Fixed - Add `err_max_response_size_exceeded` to Prometheus metrics ([#487](#487)) [2.6.0]: v2.5.0...v2.6.0 </blockquote> </p></details> --- This PR was generated with [release-plz](https://github.com/release-plz/release-plz/). --------- Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Louis Pahlavi <louis.pahlavi@dfinity.org>
## 🤖 New release * `evm_rpc_types`: 2.1.0 -> 3.0.0 (✓ API compatible changes) * `evm_rpc_client`: 0.1.0 * `evm_rpc`: 2.5.0 -> 2.6.0 (✓ API compatible changes) <details><summary><i><b>Changelog</b></i></summary><p> ## `evm_rpc_types` <blockquote> ## [3.0.0] - 2025-10-20 ### Added - Add conversions between several types in this crate and the corresponding `alloy` types. These conversions are only available with the `alloy` feature ([#465](dfinity/evm-rpc-canister#465), [#466](dfinity/evm-rpc-canister#466), [#467](dfinity/evm-rpc-canister#467), [#476](dfinity/evm-rpc-canister#476)). - **Breaking:** Add `root` and `cumulativeGasUsed` fields to `TransactionsReceipt` type ([#474](dfinity/evm-rpc-canister#474)) [3.0.0]: https://github.com/dfinity/evm-rpc-canister/compare/evm_rpc_types-v2.1.0..evm_rpc_types-v3.0.0 </blockquote> ## `evm_rpc_client` <blockquote> ## [0.1.0] - 2025-10-20 ### Added - Add methods to modify RPC config to `RequestBuilder` ([#494](dfinity/evm-rpc-canister#494)) - Add `alloy` feature flag to `evm_rpc_client` ([#484](dfinity/evm-rpc-canister#484)) - Add new `json_request` endpoint ([#477](dfinity/evm-rpc-canister#477)) - Add client support for `eth_getTransactionReceipt` ([#476](dfinity/evm-rpc-canister#476)) - Add `eth_sendRawTransaction` client support ([#467](dfinity/evm-rpc-canister#467)) - Add client support for `eth_call` ([#466](dfinity/evm-rpc-canister#466)) - Add client support for `eth_getTransactionCount` ([#465](dfinity/evm-rpc-canister#465)) - Add support for `eth_feeHistory` to client ([#460](dfinity/evm-rpc-canister#460)) - Add support for `eth_getBlockByNumber` to client ([#459](dfinity/evm-rpc-canister#459)) - Add EVM RPC canister client ([#447](dfinity/evm-rpc-canister#447)) [0.1.0]: https://github.com/dfinity/evm-rpc-canister/releases/tag/evm_rpc_client-v0.1.0 </blockquote> ## `evm_rpc` <blockquote> ## [2.6.0] - 2025-10-20 ### Added - Add support for `root` and `cumulativeGasUsed` fields in `eth_getTransactionReceipt` response ([#474](dfinity/evm-rpc-canister#474)) - Add new `json_request` endpoint and deprecate existing `request` endpoint ([#477](dfinity/evm-rpc-canister#477)) ### Changed - Update `ic-cdk` to `v0.18.7` ([#489](dfinity/evm-rpc-canister#489)) - Update `dfx` to `v0.29.0` ([#490](dfinity/evm-rpc-canister#490)) - Cleanup unused dependencies ([#491](dfinity/evm-rpc-canister#491)) ### Removed - **Breaking**: Remove `getMetrics` endpoint, this is acceptable since it was a debugging endpoint ([#479](dfinity/evm-rpc-canister#479)) ### Fixed - Add `err_max_response_size_exceeded` to Prometheus metrics ([#487](dfinity/evm-rpc-canister#487)) [2.6.0]: dfinity/evm-rpc-canister@v2.5.0...v2.6.0 </blockquote> </p></details> --- This PR was generated with [release-plz](https://github.com/release-plz/release-plz/). --------- Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Louis Pahlavi <louis.pahlavi@dfinity.org>
(XC-412) Add support to the EVM RPC client for the
eth_sendRawTransactionendpoint.