refactor: move serve_journal into upgrade_journal.rs
#3393
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
anchpop
force-pushed
the
@anchpop/move-serve-journal
branch
from
49eb06f to
4b9b18f
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
If this pull request affects the behavior of any canister owned by
the Governance team, remember to update the corresponding
unreleased_changes.md file(s).
To acknowldge this reminder (and unblock the PR), dismiss this
code review by going to the bottom of the pull request page, and
supply one of the following reasons:
-
Done.
-
No canister behavior changes.
anchpop
dismissed
github-actions[bot]’s stale review
No canister behavior changes.
aterga
reviewed
Jan 10, 2025
aterga
reviewed
Jan 10, 2025
aterga
approved these changes
Jan 10, 2025
github-merge-queue bot
pushed a commit
that referenced
this pull request
Jan 10, 2025
… Governance (#3391) I think this was a bug in the code previously. [here](https://github.com/dfinity/ic/blob/4b9b18fcada06a30f9d2286eddcacff6521ace47/rs/sns/governance/canister/canister.rs#L258-L270) is where we serialize Governance: ```rust #[pre_upgrade] fn canister_pre_upgrade() { log!(INFO, "Executing pre upgrade"); let mut writer = BufferedStableMemWriter::new(STABLE_MEM_BUFFER_SIZE); governance() .proto // This gets the `Governance` that's from `ic-sns-governance` .encode(&mut writer) .expect("Error. Couldn't serialize canister pre-upgrade."); writer.flush(); // or `drop(writer)` log!(INFO, "Completed pre upgrade"); } ``` And this is where we were deserializing it: ```rust #[post_upgrade] fn canister_post_upgrade() { log!(INFO, "Executing post upgrade"); let reader = BufferedStableMemReader::new(STABLE_MEM_BUFFER_SIZE); match GovernanceProto::decode(reader) { // this is deserializing to the Governance that's in `ic-sns-governance-api` Err(err) => { // [...] } Ok(mut governance_proto) => { // Post-process GovernanceProto // TODO: Delete this once it's been released. populate_finalize_disbursement_timestamp_seconds(&mut governance_proto); canister_init_(governance_proto); // then in here we pass to canister_init_ Ok(()) } } .expect("Couldn't upgrade canister."); // [...] } ``` `canister_init_`: ```rust #[init] fn canister_init_(init_payload: GovernanceProto) { let init_payload = sns_gov_pb::Governance::from(init_payload); // we convert the `ic-sns-governance-api` Governance to the `ic-sns-governance` Governance // [...] } ``` This seems not quite right to me because we serialize one Governance (`ic-sns-governance`), but deserialize to the other Governance (`ic-sns-governance-api`) and then convert. Why not just deserialize to the `ic-sns-governance` Governance directly? This only works because the `ic-sns-governance-api` types still set `prost` derives, but I don't think we want to be protobuffing the `ic-sns-governance-api` types anyway. (In fact, since we don't want to be protobuffing them, I actually remove their prost implementations in the next PR) --- However, we should still initialize the canister using the API types. Doing this ensures that no proto types are part of our public candid interface. So the `canister_init` method has been restored and annotated with `#[init]`, and takes the API type like `canister_init_` did previously. Now `canister_init_` takes the proto type, so it can be used by `canister_post_upgrade` and by `canister_init` after `canister_init` does the conversion. --- [← Previous PR](#3393) | [Next PR →](#3392)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What
serve_journalis moved intoupgrade_journal.rswithin SNS Governance. Previously it was located inic-nervous-system-commonWhy
serve_journalwas only used by SNS Governance, so it didn't really need to be in ic-nervous-system-common. I think it was there because that's also whereserve_logswas. If there was another reason, please let me know.serve_journalis a somewhat dangerous function because it is generic. It takes anything that implements Serialize. I think this was done because no function inic-nervous-system-commoncan takeUpgradeJournalbecause that would require depending onic-sns-governancewhich would introduce a circular dependency.But the function being generic is dangerous. We really only want to serve the journal with this function, not any other type. This would normally not be a problem, because what else are you going to pass to
serve_journalbesides the journal? But now we have two types named journal: the one inic-sns-governanceandic-sns-governance-api. And you really want to serialize theic-sns-governance-apione, because that's the one that has the manualSerializeimplementation on it that leads to the journal having a user-friendly output. By moving the function intoic-sns-governance, we're able to make it not-generic, and depend on only one type. Then there's no chance that it will be called incorrectly.For convenience, I made the function take a
ic-sns-governanceUpgradeJournaland do the conversion to theic-sns-governance-apiUpgradeJournalitself. This simplifies its usage at the call siteIn the next PR, I'm moving some things around, and this change is very useful to make sure we're passing right thing to serve_journal.
Next PR →