Skip to content

refactor: remove ProxyRuntime and add proxy feature to PocketIcRuntime#92

Merged
lpahlavi merged 6 commits intomainfrom
lpahlavi/move-proxy-to-pocket-ic-runtime
Mar 10, 2026
Merged

refactor: remove ProxyRuntime and add proxy feature to PocketIcRuntime#92
lpahlavi merged 6 commits intomainfrom
lpahlavi/move-proxy-to-pocket-ic-runtime

Conversation

@lpahlavi
Copy link
Copy Markdown
Contributor

This PR removes the ProxyRuntime added in #84 and instead adds a proxy feature to the PocketIcRuntime to optionally route update calls through a proxy canister.

@lpahlavi lpahlavi force-pushed the lpahlavi/move-proxy-to-pocket-ic-runtime branch from 4b4f063 to c59235b Compare March 10, 2026 13:38
@lpahlavi lpahlavi requested a review from gregorydemay March 10, 2026 13:43
@lpahlavi lpahlavi marked this pull request as ready for review March 10, 2026 13:43
@lpahlavi lpahlavi requested a review from a team as a code owner March 10, 2026 13:43
Copy link
Copy Markdown
Contributor

@gregorydemay gregorydemay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this PR @lpahlavi ! Only some minor comments

Comment on lines +82 to +83
curl -L "https://github.com/dfinity/proxy-canister/releases/download/${PROXY_CANISTER_VERSION}/proxy.wasm" -o $HOME/wasm/proxy.wasm
echo "PROXY_CANISTER_WASM_PATH=$HOME/wasm/proxy.wasm" >> $GITHUB_ENV
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a couple of nits (could be for another PR)

  1. We could re-use target to store wasms, e.g. target/test-artifacts/<name>.wasm
  2. This logic is not the same as the one running locally. Apparently (according to chatgpt) a standard solution there would be to create an xtask, e.g. cargo xtask fetch-proxy-wasm which can be run locally and on CI. See here. Like this also the version needs to be defined only once.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could re-use target to store wasms, e.g. target/test-artifacts/.wasm

Good idea, done!

This logic is not the same as the one running locally. Apparently (according to chatgpt) a standard solution there would be to create an xtask, e.g. cargo xtask fetch-proxy-wasm which can be run locally and on CI. See here. Like this also the version needs to be defined only once.

Cool! I didn't know about xtask, I'll have a look and do this in a separate PR 💪

return wasm;
}

let bytes = reqwest::get(DOWNLOAD_URL)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: reqwest is quite heavy in terms of dependency, why not just call curl? E.g.

let path = "target/test-artifacts/module.wasm";

    if !std::path::Path::new(path).exists() {
        std::process::Command::new("curl")
            .args([
                "-L",
                "-o",
                path,
                "https://example.com/module.wasm",
            ])
            .status()
            .unwrap();
    }

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point! Done.

Comment on lines +69 to +70
C: CyclesChargingPolicy + Clone,
<C as CyclesChargingPolicy>::Error: std::error::Error + Send + Sync + 'static,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks scary, can be simplified with

Suggested change
C: CyclesChargingPolicy + Clone,
<C as CyclesChargingPolicy>::Error: std::error::Error + Send + Sync + 'static,
C: CyclesChargingPolicy<Error: Into<BoxError>> + Clone,

(because BoxError is just an alias for Box<dyn Error + Send + Sync>)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah I forgot I could use Into<BoxError>... Done!

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also didn't know I could use this shorthand syntax to combine the two bounds in a single line!

}
}

fn panic_when_encode_fails(err: candid::error::Error) -> Vec<u8> {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: It's a bit weird that this method is declared to return Vec<u8> when it's actually the never type !. Since it's only used in one place, it would be simpler to inline it, no?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. Done.

@lpahlavi lpahlavi merged commit c3156af into main Mar 10, 2026
10 checks passed
@lpahlavi lpahlavi deleted the lpahlavi/move-proxy-to-pocket-ic-runtime branch March 10, 2026 17:34
@github-actions github-actions bot mentioned this pull request Mar 10, 2026
lpahlavi added a commit that referenced this pull request Mar 12, 2026
## 🤖 New release

* `canhttp`: 0.5.1 -> 0.5.2 (✓ API compatible changes)
* `ic-canister-runtime`: 0.2.0 -> 0.2.1 (✓ API compatible changes)
* `ic-pocket-canister-runtime`: 0.4.0 -> 0.4.1 (✓ API compatible
changes)
* `ic-agent-canister-runtime`: 0.2.0 -> 0.2.1 (✓ API compatible changes)

<details><summary><i><b>Changelog</b></i></summary><p>

## `canhttp`

<blockquote>

## [0.5.2] - 2026-03-10

### Changed

- Update dependencies
([#89](#89))

[0.5.2]:
https://github.com/dfinity/canhttp/compare/canhttp-v0.5.1..canhttp-v0.5.2
</blockquote>

## `ic-canister-runtime`

<blockquote>

## [0.2.1] - 2026-03-10

### Changed

- Update dependencies
([#89](#89))
- Implement `Runtime` for `Runtime` references
([#85](#85))

[0.2.1]:
https://github.com/dfinity/canhttp/compare/ic-canister-runtime-v0.2.0..ic-canister-runtime-v0.2.1
</blockquote>

## `ic-pocket-canister-runtime`

<blockquote>

## [0.4.1] - 2026-03-10

### Added

- Add proxy feature to `PocketIcRuntime`
([#92](#92))

### Changed

- Update dependencies
([#89](#89))

[0.4.1]:
https://github.com/dfinity/canhttp/compare/ic-pocket-canister-runtime-v0.4.0..ic-pocket-canister-runtime-v0.4.1
</blockquote>

## `ic-agent-canister-runtime`

<blockquote>

## [0.2.1] - 2026-03-10

### Changed

- Update dependencies
([#89](#89))

[0.2.1]:
https://github.com/dfinity/canhttp/compare/ic-agent-canister-runtime-v0.2.0..ic-agent-canister-runtime-v0.2.1
</blockquote>


</p></details>

---
This PR was generated with
[release-plz](https://github.com/release-plz/release-plz/).

---------

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: Louis Pahlavi <louis.pahlavi@dfinity.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants