Skip to content
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

feat: add cost_* System API and methods that calculate cycles cost under the hood #570

Open
wants to merge 10 commits into
base: next
Choose a base branch
from

Conversation

lwshang
Copy link
Contributor

@lwshang lwshang commented Mar 5, 2025

SDK-1963

Description

  • Updated ic0 according to Add system API endpoints for cycle cost calculation portal#4110.
  • Added bindings for cost_* System API in api.rs.
  • Removed the _with_cycles suffix from some Management canister methods
    • They no longer takes cycles as an argument.
    • The cycles cost is calculated inside by invoking the new cost_* API.

Note: No binding for cost_vetkd_derive_encrypted_key because the corresponding Management canister methods hasn't been included.

How Has This Been Tested?

Updated related e2e tests.

Checklist:

  • The title of this PR complies with Conventional Commits.
  • I have edited the CHANGELOG accordingly.
  • I have made corresponding changes to the documentation.

@lwshang lwshang changed the base branch from main to next March 5, 2025 19:18
@lwshang lwshang marked this pull request as ready for review March 5, 2025 19:31
@lwshang lwshang requested a review from a team as a code owner March 5, 2025 19:31
@lwshang lwshang requested a review from michael-weigelt March 5, 2025 19:31
Comment on lines 520 to 526
/// Gets the cost of a canister http outcall via [`http_request`](crate::management_canister::http_request).
pub fn cost_http_request(request_size: u64, max_res_bytes: u64) -> u128 {
let mut dst = 0u128;
// SAFETY: `dst` is a mutable reference to a 16-byte buffer, which is the expected size for `ic0.cost_http_request`.
unsafe { ic0::cost_http_request(request_size, max_res_bytes, &mut dst as *mut u128 as usize) }
dst
}
Copy link

@michael-weigelt michael-weigelt Mar 5, 2025

Choose a reason for hiding this comment

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

This direct binding to the system API is already useful, but there is an explicit wish from users to have a slightly higher-level API as well, which accepts a &HttpRequestArgs, which is what the caller already constructs before making the actual http outcall. The necessary information to go from HttpRequestArgs to request_size and max_res_bytes are in the spec, so I find it reasonable to perform this conversion in the CDK. Could you please also add such a convenience function? (section in design doc)

The same is true for cost_sign_with_ecdsa, cost_sign_with_schnorr and the future vetkd cost endpoint.

Choose a reason for hiding this comment

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

Less importantly, more for consistency, the cost_call could also have an equivalent where the users passes the method name plus a reference to the actual payload.

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