feat!: add RpcConfig arg to Candid-RPC methods
#146
Conversation
rvanasa
changed the title
RpcConfig arg to Candid-RPC methodsRpcConfig arg to Candid-RPC methods
| @@ -27,7 +27,7 @@ ic-stable-structures = { workspace = true } | |||
| ic-certified-map = { workspace = true } | |||
| ic-cdk = { workspace = true } | |||
| ic-cdk-macros = { workspace = true } | |||
| cketh-common = { git = "https://github.com/dfinity/ic", rev = "13af7fa681e3055adae4e83d172204bd3c8188b6", package = "ic-cketh-minter" } | |||
| cketh-common = { git = "https://github.com/rvanasa/ic", rev = "6a8e2eb86e4e48ad8d8a4b49d6477b7112aae12e", package = "ic-cketh-minter" } | |||
There was a problem hiding this comment.
Why did you revert back to the rvanasa repository?
There was a problem hiding this comment.
This was mentioned in this comment that I must have accidentally resolved at some point. Necessary for being able to work on this in parallel with the GitLab MR. I'll switch this back once merged.
| eth_getBlockByNumber : (RpcSource, BlockTag) -> (MultiGetBlockByNumberResult); | ||
| eth_getLogs : (RpcSource, GetLogsArgs) -> (MultiGetLogsResult); | ||
| eth_getTransactionCount : (RpcSource, GetTransactionCountArgs) -> ( | ||
| eth_feeHistory : (RpcSource, opt RpcConfig, FeeHistoryArgs) -> (MultiFeeHistoryResult); |
There was a problem hiding this comment.
Just wondering... why don't we follow the pattern of a single argument for these functions?
For example, why not have RpcSource and RpcConfig inside FeeHistoryArgs?
There was a problem hiding this comment.
RpcSource and RpcConfig are expected for all Candid-RPC methods, whereas everything in FeeHistoryArgs is specific to the eth_feeHistory JSON-RPC method. I suppose we could include source and the fields in config in a record arg, but this would lead to an awkward situation if the RPC method also includes something named source or config.
I might give this a try anyway in case there ends up being a relatively nice way to do this.
| @@ -1619,3 +1659,30 @@ fn should_restrict_rpc_access() { | |||
| } | |||
| ); | |||
| } | |||
|
|
|||
| #[test] | |||
| fn should_use_custom_response_size_estimate() { | |||
There was a problem hiding this comment.
What about the not-so-happy path?
There was a problem hiding this comment.
Right, so there's a limitation with ic-state-machine-tests where the max HTTP request size unfortunately isn't taken into account. There doesn't seem to be any way to simulate the expected IC error message (which really feels like it should be built into the testing library). For example, if I added a test case which limits the response to zero bytes, the request would succeed the first time. All we can really do is ensure that the custom size estimate is respected when sending the first HTTP outcall.
* Update 'cketh-common' * Fix compiler errors until merging #146 * Limit 'eth_getLogs' block count when specifying explicit numbers for 'fromBlock' and 'toBlock' * Fix linter warnings * Update Candid interface
…ister' feat(evmrpc): set up `evm-rpc-canister` branch Prerequisite changes for [#146](dfinity/evm-rpc-canister#146), [#147](dfinity/evm-rpc-canister#147), and [#148](dfinity/evm-rpc-canister#148) in the [EVM RPC canister](https://github.com/internet-computer-protocol/ic-eth-rpc) repository. Merges into the `evm-rpc-canister` branch. * Adds an `RpcConfig` struct with optional `response_size_estimate` field. * Changes the default response size estimate for `eth_getLogs` from 100 to 1024. * Refactors the ckETH max header size so that it is not added to the custom response size. * Updates CI based on the requirements for the `evm-rpc-canister` branch. * Removes the `Debug` trait from `RpcApi` (which may contain API keys). * Adjusts `ValidationError` enum variants. Note that these changes will not be merged into `master` at any point and are only used for the EVM RPC canister. See merge request dfinity-lab/public/ic!17319
Makes it possible to specify a custom response size estimate for any Candid-RPC method. This also gives us a way to add more configuration options (e.g. consensus/agreement policy) in a backwards-compatible manner.