feat!: adjust Candid-RPC methods for state machine tests #95
Conversation
…ic-eth-rpc into test-unexpected-response-fields
| #[ic_cdk_macros::update] | ||
| #[candid_method] | ||
| #[update(name = "eth_getLogs")] | ||
| #[candid_method(rename = "eth_getLogs")] |
There was a problem hiding this comment.
Question for anyone more familiar with the Rust CDK: is there a way to avoid all these duplicate attributes (aside from renaming the function itself)?
There was a problem hiding this comment.
I don't know the answer. What happens if you only use rename?
There was a problem hiding this comment.
Only using one or the other results in the generated Candid interface having a different name from the canister method.
rvanasa
changed the title
| @@ -199,11 +200,15 @@ impl BoundedStorable for PrincipalStorable { | |||
|
|
|||
| #[derive(Clone, Debug, Eq, PartialEq, CandidType, Deserialize)] | |||
| pub struct ProviderView { | |||
| #[serde(rename = "providerId")] | |||
There was a problem hiding this comment.
Candid unfortunately doesn't support #[serde(rename_all = ...)], so these individual attributes are necessary. Leaving out the attributes results in an encoding / decoding mismatch, so this is required for supporting both JSON and Candid (de)serialization from the same data structure.
| ) | ||
| cycles: u128, | ||
| ) -> RpcResult<HttpResponse> { | ||
| if !is_authorized(&ic_cdk::caller(), Auth::FreeRpc) { |
There was a problem hiding this comment.
The cycles payment logic was originally in the forked ckETH codebase; I moved it here to support the FreeRpc authorization for Candid-RPC methods. Everything else works the same as before.
src/candid_rpc.rs
Outdated
| } | ||
| .into()); | ||
| } | ||
| ic_cdk::api::call::msg_cycles_accept128(cycles); |
There was a problem hiding this comment.
I'm a bit confused about the logic here: Shouldn't the attached cycles match a predetermined amount required to make the outcall? What is the point of passing along a cycles parameter that may or may not be sufficient (nor necessarily the same as the number of attached cycles)?
Why is any amount greater than cycles allowed?
There was a problem hiding this comment.
cycles (now renamed to cycles_cost for clarity) is the cost calculated within the ckETH codebase based on the HTTPS outcall. msg_cycles_accept128() only consumes this cost, so you can send more cycles than necessary without being overcharged.
This will eventually be replaced with the get_request_cost() function in this repository, but it's quite a large refactor so I'm making incremental steps towards that.
| } | ||
|
|
||
| #[test] | ||
| fn eth_send_raw_transaction_should_succeed() { |
There was a problem hiding this comment.
Would it make sense to add a few tests for the unhappy path? For example, sending an invalid transaction should result in an error, right?
There was a problem hiding this comment.
I was planning to add these separately, but I'll include them here for efficiency given the review time zone offset.
Update: added several unhappy path / "fail case" tests.
This PR adds initial state machine tests for the Candid-RPC endpoints.
Progress:
EvmRpcSetupimplementation based on feedback in previous code reviewsCandidTypetrait implementations in ckETH codebaseNote that this PR also switches the canister to use the original JSON-RPC camelCase naming conventions, which ended up being necessary due to limitations with the Candid Rust implementation. This has the side benefit of being more consistent with the large amount of existing documentation on JSON-RPC methods across the Internet.
Resolves #91.
Future work: