Skip to content

refactor: use CanisterManagerResponse in CanisterManager#9652

Open
mraszyk wants to merge 9 commits intomasterfrom
mraszyk/canister-manager-response
Open

refactor: use CanisterManagerResponse in CanisterManager#9652
mraszyk wants to merge 9 commits intomasterfrom
mraszyk/canister-manager-response

Conversation

@mraszyk
Copy link
Copy Markdown
Contributor

@mraszyk mraszyk commented Mar 29, 2026

This PR makes CanisterManager return CanisterManagerResponse (instead of ad-hoc return types) for the following endpoints:

  • start_canister;
  • update_settings;
  • provisional_top_up_canister;
  • upload_chunk;
  • clear_chunk_store.

@mraszyk mraszyk marked this pull request as ready for review April 2, 2026 10:00
@mraszyk mraszyk requested a review from a team as a code owner April 2, 2026 10:00
Ok(CanisterManagerResponse {
canister_id: canister.canister_id(),
reply: Some(EmptyBlob.encode()),
heap_delta_increase: NumBytes::new(0),
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.

@maksymar Shouldn't there a heap delta be here from resized logs? I don't think this is a problem with this PR, but something that is missing from the log implementation.

If anything, this is a sign that this PR's refactor is very useful to make things like that obvious.

resource_saturation,
);

match result {
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 match statement seems to be exactly the same everywhere? Could this be made a function on CanisterManagerResponse?

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 could change

    fn process_canister_manager_response(
        &self,
        response: CanisterManagerResponse,
        ...
    ) -> ExecuteSubnetMessageResult {

to

    fn process_canister_manager_result(
        &self,
        result: Result<CanisterManagerResponse, CanisterManagerError>,
        ...
    ) -> ExecuteSubnetMessageResult {

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.

Done: d809ffd

Comment on lines +2589 to +2597
let canister = match canister_make_mut(args.get_canister_id(), state) {
Ok(canister) => canister,
Err(err) => {
return ExecuteSubnetMessageResult::Finished {
response: Err(err),
refund: msg.take_cycles(),
};
}
};
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.

This is also the same everywhere, but will be simplified in the final refactoring PR introducing a helper function passing a clone of the canister state (instead of the actual canister state).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants