feat(registry): make input fields in SubnetFeatures of type opt bool in registry canister#10492
feat(registry): make input fields in SubnetFeatures of type opt bool in registry canister#10492mraszyk wants to merge 5 commits into
Conversation
mraszyk
force-pushed
the
mraszyk/input-subnet-features-in-registry-canister
branch
from
f716c56 to
71b2c69
Compare
There was a problem hiding this comment.
Pull request overview
This PR updates the registry canister’s public Candid API for subnet feature flags so callers can provide feature fields as opt bool (i.e., optional booleans) rather than always specifying concrete booleans, while keeping the internal registry representation unchanged.
Changes:
- Introduce
SubnetFeaturesInput(all fieldsOption<bool>) and conversions into the internalSubnetFeatures. - Switch the
create_subnetandupdate_subnetcanister entrypoints to accept new argument structs (CreateSubnetArg,UpdateSubnetArg) that useSubnetFeaturesInput. - Update
registry.did/registry_test.didto exposeSubnetFeaturesInputand use it forfeaturesin create/update subnet payloads.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| rs/registry/subnet_features/src/lib.rs | Adds SubnetFeaturesInput and conversion to internal SubnetFeatures. |
| rs/registry/canister/src/mutations/do_update_subnet.rs | Adds UpdateSubnetArg and conversion into the existing UpdateSubnetPayload. |
| rs/registry/canister/src/mutations/do_create_subnet.rs | Adds CreateSubnetArg and conversion into the existing CreateSubnetPayload. |
| rs/registry/canister/canister/registry.did | Introduces SubnetFeaturesInput and changes features types for create/update subnet payloads. |
| rs/registry/canister/canister/registry_test.did | Mirrors the same Candid changes as registry.did for test builds. |
| rs/registry/canister/canister/canister.rs | Updates canister methods to accept the new CreateSubnetArg / UpdateSubnetArg and convert internally. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
This pull request changes code owned by the Governance team. Therefore, make sure that
you have considered the following (for Governance-owned code):
-
Update
unreleased_changelog.md(if there are behavior changes, even if they are
non-breaking). -
Are there BREAKING changes?
-
Is a data migration needed?
-
Security review?
How to Satisfy This Automatic Review
-
Go to the bottom of the pull request page.
-
Look for where it says this bot is requesting changes.
-
Click the three dots to the right.
-
Select "Dismiss review".
-
In the text entry box, respond to each of the numbered items in the previous
section, declare one of the following:
-
Done.
-
$REASON_WHY_NO_NEED. E.g. for
unreleased_changelog.md, "No
canister behavior changes.", or for item 2, "Existing APIs
behave as before.".
Brief Guide to "Externally Visible" Changes
"Externally visible behavior change" is very often due to some NEW canister API.
Changes to EXISTING APIs are more likely to be "breaking".
If these changes are breaking, make sure that clients know how to migrate, how to
maintain their continuity of operations.
If your changes are behind a feature flag, then, do NOT add entrie(s) to
unreleased_changelog.md in this PR! But rather, add entrie(s) later, in the PR
that enables these changes in production.
Reference(s)
For a more comprehensive checklist, see here.
GOVERNANCE_CHECKLIST_REMINDER_DEDUP
|
✅ No security or compliance issues detected. Reviewed everything up to e688a39. Security Overview
Detected Code Changes
|
daniel-wong-dfinity-org-twin
left a comment
daniel-wong-dfinity-org-twin
left a comment
There was a problem hiding this comment.
Aside: I did not know you can do this in Candid. Maybe, I used to know, but to avoid trauma, actively forgot it. IIUC, if someone sends true, but you want an opt bool, then decoding treats the true just like opt true (and likewise for false)... And yet, you cannot go the other way; if someone send you opt true, but all you want is a bool, you will NOT get a true, but rather, you will just fail to decode (and ditto for opt false)... This is why I hate Candid. I mean, I can understand why there is no "natural" value when you receive a null, but you want some other (non-opt) type. Well, Protocol Buffers has no problem with this, because every type has a "default" value, and that's what you end up with if you receive nothing when you want something... Anyway, this wacky inconsistency is tangential to your PR (hence "aside")...
| maximum_state_delta : opt nat64; | ||
| }; | ||
|
|
||
| type SubnetFeaturesInput = record { |
There was a problem hiding this comment.
A reader is not going to readily understand what "Input" is supposed to indicate. OTOH, I don't have any better ideas.
There was a problem hiding this comment.
I could use SubnetFeaturesV2 if that sounds better.
There was a problem hiding this comment.
V2 is wrong, because it does not supersede SubnetFeatures; SubnetFeatures is still current.
Idea: RequestedSubnetFeatures. When requesting features, it is natural that you need not explicitly mention ALL features (only the ones you care about).
Differences between my suggestion and "Input":
- SubnetFeatures is the base idea, so it should remain at the end, not be suffixed.
- At least in my experience, "request" is used more specifically in the context of RPCs. Whereas, "input" could be a command line flag or a function argument.
Ideally, we would also rename the existing SubnetFeatures to something like InEffectSubnetFeatures, but that is tangential, and disruptive. Similarly, it is natural that regardless of past requests, it must be that ALL features are either enabled or disabled.
| /// Input type for `create_subnet` canister method. | ||
| /// Same as `CreateSubnetPayload` but with `SubnetFeaturesInput` (all `Option<bool>`) for `features`. | ||
| #[derive(Clone, Eq, PartialEq, Debug, Default, CandidType, Deserialize, Serialize)] | ||
| pub struct CreateSubnetArg { |
There was a problem hiding this comment.
This a violent departure from our very consistent pattern: the input type for a method named dance_the_foxtrot method is called DanceTheFoxtrotPayload. Why does this super similar type need to be introduced?? Seems like you can simply change CreateSubnetPayload. This seems like a massive DRY violation. I really hope we can avoid this.
There was a problem hiding this comment.
The reason for a new type is that the original type is still used in tooling (e.g., ic-admin) and tests that target the mainnet version of the registry canister and thus must use the original type.
There was a problem hiding this comment.
tests that target the mainnet version
This can be addressed by letting such tests (and ic-admin?) use a legacy definition, which can be deleted once sufficiently new code is in production. Ideally, this legacy definition is sequestered in some other file, since it is not needed by the main implementation; rather, it is only needed by code that wants to ENCODE using the old definition in order to make canister method calls.
With Arg + Payload, it looks like the two are supposed to both exist indefinitely. But their roles are more or less the same, and so, we should minimize having both of them. If the overlap temporarily, that's ok.
There was a problem hiding this comment.
This can be addressed by letting such tests (and ic-admin?) use a legacy definition, which can be deleted once sufficiently new code is in production.
This is indeed the idea: the canister uses V2 and every tool uses the old version for now.
Ideally, this legacy definition is sequestered in some other file, since it is not needed by the main implementation
which file would you suggest? and what you suggest to use the same type name and only distinguish via the crate (qualification) or renaming (use ...::CreateSubnetPayload as CreateSubnetPayloadV2)?
|
This is a feat, not a chore, because it enables new calls. More precisely, it allows callers to omit a couple of fields. chore means that you do not care if this gets deployed in a timely way. |
mraszyk
changed the title
|
Conventional Commit prefix should have scope. Here, the choice is pretty obvious: registry. |
mraszyk
changed the title
| maximum_state_delta : opt nat64; | ||
| }; | ||
|
|
||
| type SubnetFeaturesInput = record { |
There was a problem hiding this comment.
V2 is wrong, because it does not supersede SubnetFeatures; SubnetFeatures is still current.
Idea: RequestedSubnetFeatures. When requesting features, it is natural that you need not explicitly mention ALL features (only the ones you care about).
Differences between my suggestion and "Input":
- SubnetFeatures is the base idea, so it should remain at the end, not be suffixed.
- At least in my experience, "request" is used more specifically in the context of RPCs. Whereas, "input" could be a command line flag or a function argument.
Ideally, we would also rename the existing SubnetFeatures to something like InEffectSubnetFeatures, but that is tangential, and disruptive. Similarly, it is natural that regardless of past requests, it must be that ALL features are either enabled or disabled.
| }; | ||
|
|
||
| type CreateSubnetPayload = record { | ||
| type CreateSubnetPayloadV2 = record { |
There was a problem hiding this comment.
Please, do not append "V2". Why not: the new version is "just an evolution" of the old one in a precise and technical way: a person who sends a request based on the previous version will still get the same/desired effect. In such cases, the name is generally kept the same, and there is no special reason to depart from that in this case.
| }; | ||
|
|
||
| type UpdateSubnetPayload = record { | ||
| type UpdateSubnetPayloadV2 = record { |
There was a problem hiding this comment.
Ditto.
| /// Input type for `create_subnet` canister method. | ||
| /// Same as `CreateSubnetPayload` but with `SubnetFeaturesInput` (all `Option<bool>`) for `features`. | ||
| #[derive(Clone, Eq, PartialEq, Debug, Default, CandidType, Deserialize, Serialize)] | ||
| pub struct CreateSubnetArg { |
There was a problem hiding this comment.
tests that target the mainnet version
This can be addressed by letting such tests (and ic-admin?) use a legacy definition, which can be deleted once sufficiently new code is in production. Ideally, this legacy definition is sequestered in some other file, since it is not needed by the main implementation; rather, it is only needed by code that wants to ENCODE using the old definition in order to make canister method calls.
With Arg + Payload, it looks like the two are supposed to both exist indefinitely. But their roles are more or less the same, and so, we should minimize having both of them. If the overlap temporarily, that's ok.
This PR makes the input fields in
SubnetFeaturesof typeopt bool(instead ofbool) in the registry canister. The motivation for this change is to allow dropping unused fields (such ascanister_sandboxing): as long as the registry canister expectsboolfor some field, dropping that field would break tooling and tests targetting the mainnet version of the registry canister which still has that field of typebool.