feat!: reintroduce RPC provider agreement logic #100
Conversation
There was a problem hiding this comment.
Thanks @rvanasa for this nice MR! I have some minor comments and understanding questions but nothing major.
| type MultiRpcResult_1 = variant { | ||
| Consistent : Result_1; | ||
| Inconsistent : vec record { RpcService; Result_1 }; |
There was a problem hiding this comment.
I'm a bit puzzled by the naming MultiRpcResult_1/Result_1, MultiRpcResult_2/Result_2, which is not really indicative of what it represents. I'm guessing that candid doesn't really have a concept for generics and that's why such tricks are necessary but maybe we could have something a bot more user friendly, e.g. with fee history,
type MultiFeeHistoryResult = variant {
Consistent : FeeHistoryResult;
Inconsistent : vec record { RpcService; FeeHistoryResult };
};
type FeeHistoryResult = variant { Ok : opt FeeHistory; Err : RpcError };
...
eth_feeHistory : (CandidRpcSource, FeeHistoryArgs) -> (MultiFeeHistoryResult);
Does this make sense?
PS: I would also drop the Rpc in MultiRpcResult to have 2 types of results MultiResult/Result.
There was a problem hiding this comment.
Makes sense for sure. This Candid file is automatically generated, but we could manually overwrite these names. The missing piece is a way to statically verify that the Candid interface matches the Rust source code. @dfx-json mentioned that a solution is currently in progress for this (as of a month or two ago).
There was a problem hiding this comment.
The way it was done for ckETH and some other canisters is to have the 2 (candid and Rust code) not auto-generated but then there is a unit test that controls that both are equivalent. Ideally, the candid file should be the source of truth and it should be able to generate the boilerplate Rust code, but I think that's out-of-scope for now. Maybe one easy way to modify this would be to avoid Rust generics in the types involved in the Candid interface and use struct like MultiFeeHistoryResult/FeeHistoryResult so that the generated Candid file is fixed.
There was a problem hiding this comment.
Thanks! This unit test is exactly what I was looking for. Included in #105.
candid/evm_rpc.did
Outdated
| eth_feeHistory : (CandidRpcSource, FeeHistoryArgs) -> (Result); | ||
| eth_getBlockByNumber : (CandidRpcSource, BlockTag) -> (Result_1); | ||
| eth_getLogs : (CandidRpcSource, GetLogsArgs) -> (Result_2); | ||
| eth_feeHistory : (CandidRpcSource, FeeHistoryArgs) -> (MultiRpcResult); |
There was a problem hiding this comment.
nit: from an interface perspective the name CandidRpcSource seems weird to me because the name of the language specifying the interface (Candid) shouldn't IMO appear anywhere in the interface iteself. Maybe rename it to RpcSource?
There was a problem hiding this comment.
The original intent was to distinguish from what is currently called Source for JSON-RPC methods. I switched these around based on your suggestion (RpcSource for Candid and JsonRpcSource for JSON).
| match services { | ||
| Some(services) => { | ||
| if services.is_empty() { | ||
| Err(ProviderError::ProviderNotFound)?; |
There was a problem hiding this comment.
understanding question: why do we need to distinguish between no provider being specified and an empty vector? Why not in both cases, use the default providers?
If distinguishing both cases is really necessary, this seems a bit a misuse of ProviderError::ProviderNotFound, maybe a dedicate error would be better suited, something like ProviderError::MissingRequiredProvider?
There was a problem hiding this comment.
Why not in both cases, use the default providers?
I went back and forth on this and came to the conclusion that passing an empty vector would be unintentional in almost all situations. Updated the error based on the above suggestion.
| &self, | ||
| block: candid_types::BlockTag, | ||
| ) -> MultiRpcResult<Block> { | ||
| multi_result(self.client.eth_get_block_by_number(block.into()).await) |
There was a problem hiding this comment.
understanding question: interesting, it was not clear to me until now that this calls the corresponding function eth_get_block_by_number from your own branch of cketh. Do you plan on copying over the code into that repo so that ckETH can delete it when we will integeration with the EVM RPC canister or is that the part that's meant as a separate library?
There was a problem hiding this comment.
That's the plan, although this is going to be a huge lift due to the large number of types shared between the rest of ckETH and the Ethereum RPC logic. This corresponds to the "create shared library with ckETH" part of the roadmap, which was deferred until after completing the MVP.
It's safe to remove the RPC logic on the ckETH end prior to this refactor.
| pub fn consistent(self) -> Option<RpcResult<T>> { | ||
| match self { | ||
| MultiRpcResult::Consistent(result) => Some(result), | ||
| MultiRpcResult::Inconsistent(_) => None, | ||
| } | ||
| } | ||
|
|
||
| pub fn inconsistent(self) -> Option<Vec<(RpcService, RpcResult<T>)>> { | ||
| match self { | ||
| MultiRpcResult::Consistent(_) => None, | ||
| MultiRpcResult::Inconsistent(results) => Some(results), | ||
| } | ||
| } |
| } | ||
|
|
||
| impl<T> MultiRpcResult<T> { | ||
| pub fn map<R>(self, f: impl Fn(T) -> R) -> MultiRpcResult<R> { |
There was a problem hiding this comment.
nit: I would add unit tests to ensure the that function is executed on all ok variants.
| ), | ||
| }) | ||
| } | ||
|
|
||
| fn wrap_result<T>(result: Result<T, MultiCallError<T>>) -> RpcResult<T> { | ||
| fn multi_result<T>(result: Result<T, MultiCallError<T>>) -> MultiRpcResult<T> { |
There was a problem hiding this comment.
I would add some unit tests for the result mapping
|
approved. @rvanasa will address review comments in follow-up PRs |
The EVM RPC canister now calls several providers, returning either one consistent result or all individual results when calling relevant Candid-RPC methods. The agreement logic is the same as in the ckETH minter canister.
Progress:
It's important to note that because this canister returns all JSON-RPC fields, it's possible to receive "inconsistent" results in cases where the results would otherwise be consistent for ckETH. This may also cause developers to write their own consistency checks. In this case, any built-in logic would further increase the complexity of processing RPC results. It's unclear how much of an issue this would become in practice, so developer feedback will be crucial here.
Context: internal design document