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

refactor: Consolidate how system state modifications are extracted from the System API #3706

Merged

Conversation

dsarlis
Copy link
Member

@dsarlis dsarlis commented Jan 31, 2025

Currently, there are two similar functions in the SystemApiImpl to extract system state modifications. One of them take_system_state_modifications is used in the sandbox when preparing the state changes to be transmitted back to the replica. Then, the replica after deserializing the result that it receives from the sandbox calls into_system_state_changes on the reconstructed system_api to extract the changes again.

In a recent PR, into_system_state_modifications was changed (to make it more clear which changes are relevant per message type) but take_system_state_modifications wasn't. This works correctly because into_system_state_modifications is the last one that's called before applying the state changes back to the canister state. However, it's also a very clear example of how the code can easily diverge and go unnoticed, potentially with more severe implications in the future.

This PR proposes to use one function which seems to provide the usual benefits of consistent way of applying the changes and "having one way" of doing the same task. We keep take_system_state_modifications (this allows us to get rid of some clones but more importantly it's needed in the sandbox, see comment in the existing code) and change the call sites respectively.

@dsarlis dsarlis force-pushed the dimitris/consolidate-taking-system-state-modifications branch from 69f3f6b to f2e51b8 Compare January 31, 2025 12:23
@dsarlis dsarlis marked this pull request as ready for review January 31, 2025 12:24
@dsarlis dsarlis requested a review from a team as a code owner January 31, 2025 12:24
@dsarlis dsarlis added this pull request to the merge queue Jan 31, 2025
Merged via the queue into master with commit cde7071 Jan 31, 2025
32 checks passed
@dsarlis dsarlis deleted the dimitris/consolidate-taking-system-state-modifications branch January 31, 2025 13:19
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