refactor: Replaces the hand-rolled NNS state-tree parsing for subnet types and canister ranges with Agent::fetch_subnet_by_id from ic-agent#191
Conversation
| b"cloud_engine" => Self::CloudEngine, | ||
| _ => Self::Unknown, | ||
| } | ||
| fn map_subnet_type(subnet_id: Principal, t: Option<&AgentSubnetType>) -> Result<SubnetType, Error> { |
There was a problem hiding this comment.
Do we even need local SubnetType now? Can we use the one from the agent?
There was a problem hiding this comment.
I don't have strong opinion on it. I keep the SubnetType because AI suggested:
The disadvantages are the same as before:
Copy is lost — Unknown(String) owns heap memory, so CanisterRange and anything that holds a SubnetType can no longer be Copy. The hot-path subnet_type() would need .clone() instead of a value copy.
Public API coupling — SubnetType is pub and used in domain_canister.rs. Changing it to AgentSubnetType means a semver bump in ic-agent can break your domain policy logic.
Unknown(String) complicates matching — domain_canister.rs currently does subnet_type != Some(SubnetType::CloudEngine) and exhaustive matches. With AgentSubnetType::Unknown(String) you can no longer match Unknown as a unit variant.
The Copy loss is the most impactful in practice. Given that the hot path calls subnet_type() on every request, taking a clone there for the Unknown case (which should never occur in production) is a small but unnecessary cost. The mapping is 5 lines — I'd keep it.
There was a problem hiding this comment.
AI...
Well, okay, let's keep it, but create a From implementation for our SubnetType that would convert from the agent's one, instead of this method.
We can just map None & AgentSubnetType::Unknown(_) to our Unknown variant and this way we can simplify the conversion & make it infallible.
2 participants
#NODE-1911
Summary
Replaces the hand-rolled NNS state-tree parsing for subnet types and canister ranges with
Agent::fetch_subnet_by_idfromic-agent.Before:
SubnetsInfoFetcherperformed three distinct NNS round trips per cycle:After: The fetcher still reads NNS /subnet to discover subnet IDs, then calls
agent.fetch_subnet_by_idconcurrently for each subnet, which retrieves both the type and canister ranges from each subnet's own state tree in a single round trip per subnet — with all certificate verification and tree parsing handled byic-agent.Changes
fetch_subnets→fetch_subnet_ids: now only extracts subnet IDs from the NNS /subnet subtree; type extraction removedfetch_canister_rangesremoved: replaced byagent.fetch_subnet_by_idcalls inlined intofetch()map_subnet_type added: thin free function mappingic-agent'sSubnetTypeto the local enum, keeping the local type Copy and the public API decoupled from ic-agent internalsFrom<&[u8]> for SubnetTyperemoved: no longer needed since raw byte parsing is handled byic-agenttestdata/subnet.bin,testdata/nns_canister_ranges.bin, and all related test infrastructure (TESTNET_ROOT_KEY,read_state_response, the extended ingress-expiry workaround). Tests that required real BLS-signed certificates are replaced by a pure in-memory subnet_type_lookup test that exercisesSubnetsInfo::new→ArcSwapOption::store→load→subnet_type()directly.