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.

Sign up for GitHub

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

Jump to bottom

refactor: clean up the SNS Governance API type definitions #3392

Merged
merged 1 commit into from
Jan 13, 2025
Merged

refactor: clean up the SNS Governance API type definitions #3392

merged 1 commit into from
Jan 13, 2025

Conversation

anchpop
Copy link
Contributor

@anchpop anchpop commented Jan 10, 2025 •
edited
Loading

The SNS Governance API types are for manual editing, but still have a bunch of stuff left over from when they were autogenerated.

  1. Replaced ::core::option::Option with Option and similar
  2. Removed prost derives. API types aren't ever serialized to proto. Why not get rid of the prost derives? They make things really confusing and hard to edit, and lead to us accidentally serializing the wrong types (see the previous PR). I also removed other derives like comparable::Comparable which I don't think we normally see on our types.
  3. Added additional derives for things like Debug and Default (which previously prost was giving us).
  4. Remove the // This file is @generated by prost-build. comment

These changes were pretty much done all mechanically. Sorry for not splitting them up into separate commits

Now we should be able to start evolving our API types and our internal types independently!

← Previous PR

@anchpop anchpop force-pushed the @anchpop/canister-should-deserialize-to-proto-not-api branch from f07c8c1 to 9357480 Compare January 10, 2025 03:32
@anchpop anchpop force-pushed the @anchpop/clean-up-api branch 2 times, most recently from 61d5eff to 35c049e Compare January 10, 2025 03:42
@anchpop anchpop force-pushed the @anchpop/canister-should-deserialize-to-proto-not-api branch 2 times, most recently from e1112de to ec8228c Compare January 10, 2025 03:52
@anchpop anchpop force-pushed the @anchpop/clean-up-api branch 2 times, most recently from 6c5bb85 to 8ed9ba7 Compare January 10, 2025 04:02
@anchpop anchpop mentioned this pull request Jan 10, 2025
Merged
@anchpop anchpop changed the title refactor: clean up the SNS governance API type definitions refactor: clean up the SNS Governance API type definitions Jan 10, 2025
@anchpop anchpop marked this pull request as ready for review January 10, 2025 04:33
@anchpop anchpop requested a review from a team as a code owner January 10, 2025 04:33
github-actions[bot]
github-actions bot previously requested changes Jan 10, 2025
View reviewed changes
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

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:

  1. Done.

  2. No canister behavior changes.

@anchpop anchpop dismissed github-actions[bot]’s stale review January 10, 2025 04:51

No canister behavior changes.

@anchpop anchpop force-pushed the @anchpop/canister-should-deserialize-to-proto-not-api branch from ec8228c to f589972 Compare January 10, 2025 16:09
@anchpop anchpop force-pushed the @anchpop/clean-up-api branch from 8ed9ba7 to 25d8f70 Compare January 10, 2025 16:09
@anchpop anchpop force-pushed the @anchpop/canister-should-deserialize-to-proto-not-api branch from f589972 to 5b45cac Compare January 10, 2025 16:37
@anchpop anchpop force-pushed the @anchpop/clean-up-api branch from 25d8f70 to 45da517 Compare January 10, 2025 16:37
github-merge-queue bot pushed a commit that referenced this pull request Jan 10, 2025
@anchpop
fix: SNS Gov canister should deserialize to proto Governance, not API…
df7d443 
… 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)
Base automatically changed from @anchpop/canister-should-deserialize-to-proto-not-api to master January 10, 2025 21:35
@anchpop anchpop force-pushed the @anchpop/clean-up-api branch from 45da517 to fd398fe Compare January 11, 2025 00:02
@anchpop anchpop enabled auto-merge January 11, 2025 00:13
@anchpop anchpop added this pull request to the merge queue Jan 13, 2025
Merged via the queue into master with commit a358756 Jan 13, 2025
31 checks passed
@anchpop anchpop deleted the @anchpop/clean-up-api branch January 13, 2025 17:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@max-dfinity max-dfinity max-dfinity approved these changes

@github-actions github-actions[bot] github-actions[bot] left review comments

Assignees
No one assigned
Labels
refactor
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@anchpop @max-dfinity