-
Notifications
You must be signed in to change notification settings - Fork 16
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
test: mock HTTPS outcalls in state machine tests #89
Conversation
…ic-eth-rpc into mock-https-outcalls
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @rvanasa for the PR!
} | ||
} | ||
|
||
/// Shorthand for deriving an `EvmRpcSetup` with the caller as the canister controller. | ||
pub fn as_controller(&self) -> Self { | ||
let mut setup = self.clone(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rvanasa : understanding question, taking &self
as a parameter to call afterwards self.clone()
seems like an anti-pattern. Why not taking directly self
as parameter and avoid cloning altogether? (similar question for the fluent methods below)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the detailed feedback on this PR!
Here is a motivating example:
let setup = EvmRpcSetup::new();
setup.as_controller().authorize(...);
setup.register_provider(...);
setup.as_anonymous().request(...);
Another (more verbose) option would be for the caller to perform the cloning on their end:
let setup = EvmRpcSetup::new();
setup.clone().as_controller().authorize(...);
setup.register_provider(...);
setup.clone().as_anonymous().request(...);
I see the case for switching to the latter if you think that would be preferable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rvanasa : It's just that typically with the builder pattern you would do this in a single statement, e.g.,
let setupt = EvmRpcSetup::new()
.as_controller().authorize(...)
.register_provider(...)
.as_anonymous().request(...);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll do another pass on this while adding more tests. This would be the builder pattern equivalent:
EvmRpcSetup::new()
.as_controller()
.authorize(...)
.as_user()
.register_provider(...)
.as_anonymous()
.request(...);
The reason I'm using the current logic is that it's more explicit about which authorization to use. For instance, removing as_user()
or as_anonymous()
above could accidentally change the permissions for actions later in the chain. On the other hand, it's certainly more convenient, so I'll continue weighing both of these options.
@@ -211,3 +370,35 @@ fn test_provider_registry() { | |||
] | |||
) | |||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rvanasa : additional test ideas (also ok if you prefer to do that in another MR):
- ensure that the json is correctly sanitized. E.g, query two different providers with the same JSON-RPC request such that both providers return the same JSON content but the string value is different (e.g. fields are ordered differently). Ensure that results provided by the rpc canister are equal in terms of string equality.
- Ensure that unknown fields in the response are ignored. This ensures that things will keep working if there is a new version of the Ethereum JSON-RPC API
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
➕
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good! I added (1.) in this pull request and will include (2.) separately; tracking in #91.
@@ -8,8 +8,8 @@ pub use ethers_core as core; | |||
|
|||
pub use rpc::{call_contract, get_provider, request}; | |||
|
|||
#[ic_cdk_macros::query(name = "__transform_ic_evm_rpc")] | |||
pub fn transform_evm_rpc(args: TransformArgs) -> HttpResponse { | |||
#[ic_cdk_macros::query(name = "__ic_evm_transform_json_rpc")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dumb question: why have another name as the method's name (__ic_evm_transform_json_rpc
vs transform_json_rpc
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code above is from a separate Rust library (in lib/rust
) which was intended as an alternative to calling the EVM RPC canister. At one point, the canister used this library to perform JSON-RPC requests, but this has changed for the current MVP design.
__ic_evm_transform_json_rpc
is provided by the ic_evm
library whereas __transform_json_rpc
is specific to the canister. They're essentially doing the same thing, so I used arbitrarily different names to distinguish these from each other. I'd be happy to change the names if we can come up with something better.
Co-authored-by: Thomas Locher <thomas.locher@dfinity.org>
…puter-protocol/ic-eth-rpc into mock-https-outcalls
Adds a mechanism for mocking HTTP requests (based on this MR from ckETH).
Resolves #87.