-
Notifications
You must be signed in to change notification settings - Fork 330
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(nns): Deleted ListNeurons from NNS governance.proto. #3546
refactor(nns): Deleted ListNeurons from NNS governance.proto. #3546
Conversation
There was a problem hiding this 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:
-
Done.
-
No canister behavior changes.
This is a refactor.
4aba721
to
84ca592
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Author's diff reading guide.
rs/nns/governance/proto/ic_nns_governance/pb/v1/governance.proto
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok from the Rosetta side.
…to. We should be able to delete most if not all request and response types from governance.proto, because we do not store those in stable memory. This shows how we would delete other request and response types. This also shows that such deletions are fairly straightforward.
…ronsResponse). This is done by making it use the analogous API types. This also allows //rs/rosetta-api/icp to stop depending on //rs/nns/governance entirely. Yay! That library should only be used by //rs/nns/governance:governance-canister.
…Ideally, we would chop it up. Some of the pieces would have more visibility than others. In any case, this is a step in the right direction, made possible by previous commit.
…o public, even though that is what most people in the world would want.
13c8544
to
a8c3a76
Compare
) The idea here is very much the same as the [previous PR] in this thread/series. Therefore, please see the description of the aforementioned PR for an overview, and details. [previous PR]:#3546 One difference here is that we only deleted the response type, not the request type. In this case, deleting the request type is not possible, because it is used (indirectly) by `Governance` (via `Proposal`). This is an unusual situation; most request types do not have this property.
Recently, we deleted a bunch of request and response types from NNS governance.proto. Although the types themselves are not necessary, the comments contained valuable information, which could (and should) have been copied to governance.did. This PR corrects that oversight. # References Types deleted in recent PRs: * [ListNeurons] * [ManageNeuronResponse] * [NeuronInfo] [ListNeurons]: #3546 [ManageNeuronResponse]: #3573 [NeuronInfo]: #3639
Also ListNeuronsResponse.
Motivation/Inspiration
I started working on deleting
(deciding|potential)_voting_power
fields fromgovernance.proto
(NNS1-3500). Some of those fields appear inNeuronInfo
. I looked into that type, and it seems like we could delete it, because AFAICT, it is only used in a couple of response types, such asListNeuronsResponse
. Therefore, this indirectly helps with getting rid of*_voting_power
fields ingovernance.proto
.We should be able to delete most request and response types from
governance.proto
, because we do not store those in stable memory. This shows how we would delete other request and response types. This also shows that such deletions can be fairly straightforward.Bonus Changes
Switch rosetta from using
//rs/nns/governance
to//rs/nns/governance/api
. This made it possible to "lock down"//rs/nns/governance
, i.e. reduce its visibility. Because min visibility good.Make aliases generated by rust_canister inherit visibility. This was needed to lock down
//rs/nns/governance
without blocking legitimate dependencies from elsewhere in the bazel workspace.How to Delete Request & Response Types from governance.proto
Here is an overview of how this PR was constructed; the same steps can (very likely) be used to delete other request and response types:
governance.proto
(and run Prost Rust code generation). Example. However, do not simply delete the comments; instead, move them to governance.did. Example.rs/nns/governance/src/pb/conversions.rs
. Example.bazel build //rs/nns/governance
. This will reveal all the places that need to be updated in the main lib.canister.rs
. It might well be that the only thing you need to do here is remove some.into()
. Those are no longer necessary, because now, they just convert T into T. Ofc, this change is not strictly necessary, but clippy might yell at you, and in any case, it confuses humans when you do.into()
even though the object is already of the desired type.The only "insight" here is that it is fairly easy to replace Prost-generated request and response types with API types. Other than that, these steps are more or less corollaries of "delete request and response types".
The examples above are all part of this similar PR.
Why Min Visibility Good
When you want to change something, you won't need to change the whole world. I.e. encapsulation, modularity, loose coupling, non-entanglement, etc. Being able to make changes in the future is extrem imperative, because change is the only constant.
This is the exact same rationale that is used to justify private ALL the things.
References
Next PR 👉