Conversation
gregorydemay
left a comment
gregorydemay
left a comment
There was a problem hiding this comment.
Thanks for adding some int test! I have the impression we could simplify a few things.
| use tower::BoxError; | ||
|
|
||
| #[derive(Debug, CandidType, Deserialize)] | ||
| pub struct InsufficientCyclesError { |
There was a problem hiding this comment.
understanding question: why this new error type, why not keep BoxError? The examples should be easy to read and my impression here is that this complexifies many examples. I think it's fine to have an example where we show the charging with the proxy, but I would a priori not touch existing examples.
There was a problem hiding this comment.
The point of this new error type is that is implements CandidType so it can be returned by the canister in case insufficient cycles are passed. BoxError cannot implement CandidType.
As for the examples, I agree it makes sense to keep them as simple as possible, but I also feel like creating a whole new example just to show a different charging policy seems a little overkill... Potentially, inlining the make_http_post_request method could make the code easier to read.
test_fixtures/src/lib.rs
Outdated
| let Setup { | ||
| env, | ||
| canister_id, | ||
| proxy_canister_id: _, |
There was a problem hiding this comment.
What if there is already a proxy canister id set?
There was a problem hiding this comment.
This is test infrastructure code, so worst case we would install the proxy canister twice, but I added an assert to be safe.
| } | ||
| } | ||
|
|
||
| pub fn canister(&self) -> Canister<MaybeProxyRuntime<'_>> { |
There was a problem hiding this comment.
My impression is that we don't need another runtime implementation. We could have a dedicated proxy_canister method that returns a Canister<ProxyRuntime<PocketIcRuntime<'_>>> and call it in the specific test where we need it. WDYT?
There was a problem hiding this comment.
Indeed it's not the only way to go, but I also don't really like the idea of having to explicitly specifying whether to use the proxy or not when creating a Canister instance. IMO if the proxy canister is installed, it should be used by default.
Ideally, we would be able to box the runtime, but Runtime is not dyn-compatible due to the generics in its method signatures IIUC.
2 participants
Add integration tests for the
ProxyRuntimeadded in #84.