refactor: require HTTP outcall base fee by lpahlavi · Pull Request #185 · dfinity/sol-rpc-canister

lpahlavi · 2025-07-09T12:25:37Z

(XC-416) Require all requests to endpoints making an HTTP outcall to have at least the base cycles fee of an HTTP outcall attached when not in demo mode.

gregorydemay

Thanks @lpahlavi for this PR! Main concern is related to the exposed error code, but otherwise LGTM!

canister/src/main.rs

integration_tests/tests/tests.rs

libs/client/src/request/mod.rs

integration_tests/tests/tests.rs

gregorydemay

Thanks for the improvements @lpahlavi! Some remarks regarding the error mapping.

gregorydemay · 2025-07-11T08:07:34Z

canister/src/main.rs

+    fn base_fee() -> u128 {
+        let num_nodes = read_state(|state| state.get_num_subnet_nodes());
+        3_000_000_u128
+            .saturating_add(60_000_u128.saturating_mul(num_nodes as u128))
+            .saturating_mul(num_nodes as u128)
+    }


nit: note that the base fee only changes with state.get_num_subnet_nodes(). In other words, we could have a new private field in State that contains that value so that it's computed only once.

Yes! Good idea! Done.

libs/client/src/request/mod.rs

…tionCode`

… cycles to perform an HTTP outcall

lpahlavi · 2025-07-11T14:48:19Z

Thanks a lot for the review and comments @gregorydemay! I moved the refactoring related to the new try_send method (and the whole RejectCode vs RejectionCode) to #187 so unfortunately I had to rebase this PR, but it's quite a bit smaller now so it should be hopefully easier to review!

gregorydemay · 2025-07-28T11:03:55Z

canister/src/memory/mod.rs

    log_filter: LogFilter,
    mode: Mode,
    num_subnet_nodes: u32,
+    base_http_outcall_fee: u128,


note that adding a mandatory field to a struct serialized in stable memory will fail to upgrade, as shown by the CI pipeline

Client error: failed to call getBalance: (SysFatal, "An error happened during the call: 5: IC0503: Error from Canister zaylz-mqaaa-aaaar-qaqzq-cai: Canister called ic0.trap with message: 'Panicked at 'failed to decode memory bytes a6686170695f6b657973a96e416c6368656d794d61696e6e65747820306e6d7372394e755a4c347a57744f62544f7747467035314e776638626969346d416c6368656d794465766e65747820306e6d7372394e755a4c347a57744f62544f7747467035314e776638626969346b416e6b724d61696e6e65747840663330663736633030386361626539343130373238363130383161373862326661313865643937383630616263366432373534656563373332646236383532316a416e6b724465766e657478406633306637366330303863616265393431303732383631303831613738623266613138656439373836306162633664323735346565633733326462363835323171436861696e737461636b4d61696e6e65747820323334303131643131326336303339666531663837363038663139306662636570436861696e737461636b4465766e6574782066323562396561656635626263653031313235333639643734643434663230626a447270634465766e6574782c416d5356303941636845703874447a4243743873796a55625530524f45377752384c30464e6a57436e30326a6d48656c6975734d61696e6e6574782438623464643638612d383031322d346533342d396361352d3337356664333732323938636c48656c6975734465766e6574782438623464643638612d383031322d346533342d396361352d333735666433373232393863726170695f6b65795f7072696e636970616c7381581dce7a4082861dd14808ea4b8ac53a9f2a59e35747757afd24c2d00a7302716f766572726964655f70726f7669646572a16c6f766572726964655f75726cf66a6c6f675f66696c7465726753686f77416c6c646d6f6465664e6f726d616c706e756d5f7375626e65745f6e6f6465731822: Semantic(None, "missing field base_http_outcall_fee")', canister/src/memory/mod.rs:88:29'.\nConsider gracefully handling failures from this canister or altering the canister to handle exceptions. See documentation: https://internetcomputer.org/docs/current/references/execution-errors#trapped-explicitly")

It would be a good idea to

add a test to ensure state compatibility, e.g. like this

Make this field optional and change the getter to something like pub fn compute_base_http_outcall_fee(&mut self) -> u128 that returns the previously computed value if there is one and otherwise set it and returns.

Good catch! I've modified State to have an optional base_http_outcall_fee field instead and added some tests.

gregorydemay

Thanks for adding the test! I think there is a minor problem with the lazy_ method but otherwise code LGTM!

gregorydemay · 2025-07-29T05:01:40Z

canister/src/memory/mod.rs

    }

-    pub fn get_base_http_outcall_fee(&self) -> u128 {
+    pub fn lazy_compute_base_http_outcall_fee(&self) -> u128 {


Note that this will constantly re-evaluate compute_base_http_outcall_fee(self.num_subnet_nodes) if the initial value is empty. To be lazy, it's unavoidable that this method must be able to mutate the underlying state (i.e. parameter &mut self instead of &self).

🙈 I think my brain was mush because of the jetlag yesterday, thanks for catching that!

canister/src/types/mod.rs

canister/src/memory/tests.rs

gregorydemay

Thanks @lpahlavi for the improvements!

## 🤖 New release * `sol_rpc_types`: 1.0.0 -> 2.0.0 (⚠ API breaking changes) * `sol_rpc_canister`: 1.0.0 -> 1.1.0 (✓ API compatible changes) * `sol_rpc_client`: 1.0.0 -> 2.0.0 (⚠ API breaking changes) ### ⚠ `sol_rpc_types` breaking changes ```text --- failure constructible_struct_adds_field: externally-constructible struct adds field --- Description: A pub struct constructible with a struct literal has a new pub field. Existing struct literals must be updated to include the new field. ref: https://doc.rust-lang.org/reference/expressions/struct-expr.html impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.41.0/src/lints/constructible_struct_adds_field.ron Failed in: field TransactionStatusMeta.cost_units in /tmp/.tmpuppblp/sol-rpc-canister/libs/types/src/solana/transaction/mod.rs:286 ``` <details><summary>Changelog</summary> ## `sol_rpc_types` <blockquote> ## [2.0.0] - 2025-07-31 ### Added - Add optional `cost_units` to `TransactionStatusMeta` ([#180](#180)) - Add build requirements to READMEs and rustdoc ([#169](#169)) - Add `Cargo.toml` linting to CI pipeline ([#155](#155)) ### Changed - Require HTTP outcall base fee ([#185](#185)) - Select supported providers based on successful responses ([#183](#183)) - Migrate dependencies to `solana-sdk` repository ([#55](#55)) - Improve docs for `InstallArgs` ([#172](#172)) [2.0.0]: https://github.com/dfinity/sol-rpc-canister/compare/1.0.0..2.0.0 </blockquote> ## `sol_rpc_canister` <blockquote> ## [1.1.0] - 2025-07-31 ### Added - Add optional `cost_units` to `TransactionStatusMeta` (#180) - Add more metrics (#144) ### Changed - Do not record metrics for requests with insufficient cycles (#184) - Require HTTP outcall base fee (#185) - Select supported providers based on successful responses (#183) ### Fixed - Change `nat16` to `nat32` in examples (#151) </blockquote> ## `sol_rpc_client` <blockquote> ## [2.0.0] - 2025-07-31 ### Added - Add `try_send` method to SOL RPC client ([#187](#187)) - Add build requirements to READMEs and rustdoc ([#169](#169)) - Add `Cargo.toml` linting to CI pipeline ([#155](#155)) ### Changed - Revert `sol_rpc_client` bump ([#178](#178)) - Migrate dependencies to `solana-sdk` repository ([#55](#55)) - Bump `sol_rpc_client` to `v1.0.1` ([#164](#164)) - Enable `ed25519` feature in docs ([#162](#162)) ### Fixed - Use correct fee for t-sig with local development key ([#160](#160)) [2.0.0]: https://github.com/dfinity/sol-rpc-canister/compare/1.0.0..2.0.0 </blockquote> </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>

lpahlavi force-pushed the pahlavi/require-http-outcall-base-fee branch 3 times, most recently from 5ac86a8 to ad65b99 Compare July 9, 2025 13:32

lpahlavi requested a review from gregorydemay July 9, 2025 13:58

lpahlavi marked this pull request as ready for review July 9, 2025 13:58

lpahlavi requested a review from a team as a code owner July 9, 2025 13:58

gregorydemay reviewed Jul 10, 2025

View reviewed changes

lpahlavi requested a review from gregorydemay July 10, 2025 09:07

gregorydemay reviewed Jul 11, 2025

View reviewed changes

lpahlavi added 7 commits July 11, 2025 15:44

Use ic_error_types::RejectCode instead of `ic_cdk::api::call::Rejec…

6ff5ac1

…tionCode`

Revert explicit update of canhttp

6602e40

XC-416: Require all requests to update endpoints have at least enough…

0d7bde1

… cycles to perform an HTTP outcall

XC-416: Clippy

9b1910c

XC-416: Fix test

f13b789

XC-416: Use ic_error_types::RejectCode and add demo mode test

6218e2d

XC-416: Update panic message

8ba52de

lpahlavi force-pushed the pahlavi/require-http-outcall-base-fee branch from 79122f2 to 8ba52de Compare July 11, 2025 14:35

lpahlavi changed the base branch from main to lpahlavi/add-try-send-method July 11, 2025 14:36

lpahlavi added 2 commits July 11, 2025 16:39

Fix rebase

a1df170

Cache base HTTP outcall fee in state

2c58539

lpahlavi requested a review from gregorydemay July 11, 2025 14:48

lpahlavi added 2 commits July 11, 2025 16:49

Fix rebase

a869199

Fix rebase

b7c79e6

gregorydemay reviewed Jul 28, 2025

View reviewed changes

Lazy compute base HTTP fee

e60e5f2

Base automatically changed from lpahlavi/add-try-send-method to main July 28, 2025 11:55

lpahlavi added 3 commits July 28, 2025 13:56

Merge branch 'main' into pahlavi/require-http-outcall-base-fee

ac7d054

Add proptest for state upgrade

945fcbc

Formatting

c13e41a

Clippy

1bdcfcd

lpahlavi requested a review from gregorydemay July 28, 2025 14:49

gregorydemay reviewed Jul 29, 2025

View reviewed changes

lpahlavi added 3 commits July 29, 2025 07:55

Add arb_state_v* methods

6d09f0f

Remove #[derive(Arbitrary)]

09f4c37

Update state after base HTTP fee initialization

5da0e9e

lpahlavi requested a review from gregorydemay July 29, 2025 06:04

lpahlavi added 5 commits July 29, 2025 08:16

Clippy

4fd1151

Use mutate_state instead of read_state

7916fa3

Clippy

507247a

Add arb_api_key() method

cc27162

Fix special characters handling in arb_api_key

6fcb20c

gregorydemay approved these changes Jul 29, 2025

View reviewed changes

lpahlavi merged commit 4d142a5 into main Jul 29, 2025
12 checks passed

lpahlavi deleted the pahlavi/require-http-outcall-base-fee branch July 29, 2025 09:36

This was referenced Jul 30, 2025

chore: release #189

Closed

chore: release #201

Merged

github-actions bot mentioned this pull request Aug 29, 2025

chore: release #219

Merged

Conversation

lpahlavi commented Jul 9, 2025

Uh oh!

gregorydemay left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gregorydemay left a comment

Choose a reason for hiding this comment

Uh oh!

gregorydemay Jul 11, 2025

Choose a reason for hiding this comment

Uh oh!

lpahlavi Jul 11, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lpahlavi commented Jul 11, 2025

Uh oh!

gregorydemay Jul 28, 2025

Choose a reason for hiding this comment

Uh oh!

lpahlavi Jul 28, 2025

Choose a reason for hiding this comment

Uh oh!

gregorydemay left a comment

Choose a reason for hiding this comment

Uh oh!

gregorydemay Jul 29, 2025

Choose a reason for hiding this comment

Uh oh!

lpahlavi Jul 29, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

gregorydemay left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants