Please address the comments from this code review:

## Overall Comments
- lease_has_changed’s catch-all arm treats any mismatched pair as ‘changed’, which may hide logic errors—replace the wildcard with unreachable!() or explicit error handling.

## Individual Comments

### Comment 1
<location> `src/dhcp_proxy/dhcp_service.rs:296` </location>
<code_context>
+        (MozimLease::V6(old_v6), MozimLease::V6(new_v6)) => {
+            old_v6.addr != new_v6.addr || old_v6.prefix_len != new_v6.prefix_len
+        }
+        _ => true, // could have used unreachable!()
+    }
+}
</code_context>

<issue_to_address>
**suggestion (bug_risk):** Using 'true' for mismatched lease types in lease_has_changed may mask logic errors.

Using 'true' here may conceal incorrect mixing of lease types. Instead, consider unreachable!() or logging to highlight these cases during development.

```suggestion
        _ => unreachable!("Mismatched MozimLease types in lease_has_changed"),
```
</issue_to_address>

### Comment 2
<location> `src/dhcp_proxy/dhcp_service.rs:369` </location>
<code_context>
             }
+            // NO gateway logic for IPv6. This is intentional.
         }
+        _ => return Err(NetavarkError::msg("Lease type mismatch during IP update")),
     }
-
</code_context>

<issue_to_address>
**suggestion:** Returning a generic error for lease type mismatch may obscure the root cause.

Include the actual lease types in the error message to make debugging easier.

```suggestion
        _ => {
            return Err(NetavarkError::msg(format!(
                "Lease type mismatch during IP update: new lease type is {:?}, old lease type is {:?}",
                new_lease, old_lease
            )));
        }
```
</issue_to_address>

### Comment 3
<location> `src/network/dhcp.rs:67-70` </location>
<code_context>
+
+    // If IPv4 fails but IPv6 is requested, should we continue or fail all?
+    // For now, we'll let it continue.
+    let v4_lease = match get_lease_from_proxy(nvp_config_v4) {
+        Ok(l) => l,
+        Err(e) => {
+            return Err(NetavarkError::msg(format!("unable to obtain lease: {e}")));
+        }
+    };
</code_context>

<issue_to_address>
**suggestion:** Failing immediately on IPv4 lease error may prevent IPv6-only containers from working.

If ipam.ipv6_enabled is true and no IPv4 addresses are needed, consider allowing the process to continue when IPv4 lease acquisition fails.

Suggested implementation:

```rust
    let v4_lease = match get_lease_from_proxy(nvp_config_v4) {
        Ok(l) => l,
        Err(e) => {
            // If IPv6 is enabled and no IPv4 addresses are needed, allow to continue
            if ipam.ipv6_enabled && nvp_config_v4.is_none() {
                // No IPv4 lease, but that's acceptable for IPv6-only containers
                None
            } else {
                return Err(NetavarkError::msg(format!("unable to obtain lease: {e}")));
            }
        }
    };

```

You may need to ensure that `nvp_config_v4` is an `Option` and that downstream code can handle `v4_lease` being `None`. If `v4_lease` is expected to always be present, update its type to `Option<LeaseType>` (or whatever type is returned by `get_lease_from_proxy`). Also, make sure that the logic for handling leases later in the code can handle the case where `v4_lease` is `None`.
</issue_to_address>

### Comment 4
<location> `test-dhcp/002-setup.bats:33-42` </location>
<code_context>
+: <<'DISABLED_TEST'
</code_context>

<issue_to_address>
**issue (testing):** IPv6 setup test is currently disabled.

Please enable the test to verify DHCPv6 functionality. If it cannot be enabled now, add a note explaining the reason and a plan for future activation.
</issue_to_address>

### Comment 5
<location> `test-dhcp/helpers.bash:344-353` </location>
<code_context>
+function run_dhcp() {
</code_context>

<issue_to_address>
**suggestion (testing):** No negative/edge case tests for DHCPv6 lease failures.

Please add tests for scenarios such as no available addresses, misconfigured dnsmasq, and timeouts to ensure proper error handling and messaging.
</issue_to_address>

### Comment 6
<location> `src/dhcp_proxy/dhcp_service.rs:300` </location>
<code_context>
+    }
+}
+
 fn update_lease_ip(
     netns: &str,
     interface: &str,
</code_context>

<issue_to_address>
**issue (complexity):** Consider refactoring the large match blocks in `update_lease_ip` and `lease_has_changed` into smaller helper functions for each IP version to improve readability and maintainability.

Here’s one way to tame the nesting in `update_lease_ip` (and the free‐fn `lease_has_changed`) by pulling each IP/version case into its own small helper.  You’ll end up with a single `match` that just calls into two focused routines.

```rust
fn update_lease_ip(
    netns: &str,
    interface: &str,
    old_lease: &MozimLease,
    new_lease: &MozimLease,
) -> NetavarkResult<()> {
    let (_, ns) = core_utils::open_netlink_sockets(netns)
        .wrap("failed to open netlink socket in netns")?;
    let mut sock = ns.netlink;

    match (old_lease, new_lease) {
        (MozimLease::V4(o), MozimLease::V4(n)) =>
            update_lease_ip_v4(&mut sock, interface, o, n),
        (MozimLease::V6(o), MozimLease::V6(n)) =>
            update_lease_ip_v6(&mut sock, interface, o, n),
        _ => Err(NetavarkError::msg("Lease type mismatch during IP update")),
    }
}

fn update_lease_ip_v4(
    sock: &mut NetlinkSocket, interface: &str,
    old: &MozimV4Lease, new: &MozimV4Lease
) -> NetavarkResult<()> {
    let old_net = ipnet::Ipv4Net::with_netmask(old.yiaddr, old.subnet_mask)?;
    let new_net = ipnet::Ipv4Net::with_netmask(new.yiaddr, new.subnet_mask)?;
    if new_net != old_net {
        let link = sock.get_link(LinkID::Name(interface.to_string()))?
            .wrap("get interface in netns")?;
        sock.add_addr(link.header.index, &ipnet::IpNet::V4(new_net))
            .wrap("add new addr")?;
        sock.del_addr(link.header.index, &ipnet::IpNet::V4(old_net))
            .wrap("remove old addr")?;
    }
    // ... default‐gateway logic here ...
    Ok(())
}

fn update_lease_ip_v6(
    sock: &mut NetlinkSocket, interface: &str,
    old: &MozimV6Lease, new: &MozimV6Lease
) -> NetavarkResult<()> {
    let old_net = ipnet::Ipv6Net::new(old.addr, old.prefix_len)?;
    let new_net = ipnet::Ipv6Net::new(new.addr, new.prefix_len)?;
    if new_net != old_net {
        let link = sock.get_link(LinkID::Name(interface.to_string()))?
            .wrap("get interface in netns")?;
        sock.add_addr(link.header.index, &ipnet::IpNet::V6(new_net))?;
        sock.del_addr(link.header.index, &ipnet::IpNet::V6(old_net))?;
    }
    Ok(())
}
```

And likewise push `lease_has_changed` onto the enum itself:

```rust
impl MozimLease {
    fn has_changed(&self, other: &MozimLease) -> bool {
        match (self, other) {
          (MozimLease::V4(a), MozimLease::V4(b)) =>
            a.yiaddr   != b.yiaddr ||
            a.subnet_mask != b.subnet_mask ||
            a.gateways != b.gateways,
          (MozimLease::V6(a), MozimLease::V6(b)) =>
            a.addr     != b.addr ||
            a.prefix_len != b.prefix_len,
          _ => true,
        }
    }
}
```

This flattening pulls each version’s logic out of one huge `match` block and into small, self‐contained functions, making both the control flow and each protocol’s behavior easier to read & maintain.
</issue_to_address>

### Comment 7
<location> `src/network/dhcp.rs:54` </location>
<code_context>
+    // --- Perform DHCPv4 Lease ---
+    // For now, we assume IPv4 is always desired unless explicitly disabled in future logic.
+    // You could make this conditional based on `ipam.container_addresses` if needed.
+    let nvp_config_v4 = NetworkConfig {
+        version: 0, // Explicitly set for IPv4
         host_iface: host_network_interface.to_string(),
</code_context>

<issue_to_address>
**issue (complexity):** Consider extracting a helper function for NetworkConfig creation and using Option::or to simplify merging logic.

```rust
// 1. Extract a helper to remove the v4/v6 NetworkConfig duplication:
fn build_network_config(
    host_iface: &str,
    container_iface: &str,
    ns_path: &str,
    mac: &str,
    hostname: &str,
    container_id: &str,
    version: u8,
) -> NetworkConfig {
    NetworkConfig {
        version,
        host_iface: host_iface.to_string(),
        container_iface: container_iface.to_string(),
        ns_path: ns_path.to_string(),
        container_mac_addr: mac.to_string(),
        host_name: hostname.to_string(),
        container_id: container_id.to_string(),
        domain_name: "".into(),
    }
}

// Then in get_dhcp_lease:
let nvp_v4 = build_network_config(
    host_network_interface,
    container_network_interface,
    ns_path,
    container_macvlan_mac,
    container_hostname,
    container_id,
    0,
);
let v4_lease = get_lease_from_proxy(nvp_v4)?;
// ...
if ipam.ipv6_enabled {
    let nvp_v6 = build_network_config(
        host_network_interface,
        container_network_interface,
        ns_path,
        container_macvlan_mac,
        container_hostname,
        container_id,
        1,
    );
    let (v6_subnet, v6_dns, v6_domain) = parse_lease(get_lease_from_proxy(nvp_v6)?)?;
    subnets.push(v6_subnet);

    // 2. Simplify the merge logic with `Option::or`:
    all_dns_servers   = all_dns_servers.or(v6_dns);
    all_domain_names  = all_domain_names.or(v6_domain);
}
```
</issue_to_address>

### Comment 8
<location> `src/dhcp_proxy/lib.rs:117` </location>
<code_context>

+    // DhcpV6Lease
+    // here the fields of DhcpV4Lease are repurposed tarry their equivalents in the DhcpV6Lease
+    impl From<DhcpV6Lease> for Lease {
+        fn from(l: DhcpV6Lease) -> Lease {
+            let mut domain_name = String::new();
</code_context>

<issue_to_address>
**issue (complexity):** Consider refactoring the option-parsing loop into separate helper functions for clarity and maintainability.

```markdown
You can collapse that 70-line loop into three small, self-contained parsers. For example:

```rust
impl From<DhcpV6Lease> for Lease {
    fn from(l: DhcpV6Lease) -> Lease {
        let dns_servers   = parse_dns_servers(&l.dhcp_opts);
        let domain_name   = parse_domain_search(&l.dhcp_opts);
        let ntp_servers   = parse_ntp_servers(&l.dhcp_opts);

        Lease {
            t1:            l.t1,
            t2:            l.t2,
            lease_time:    l.valid_life,
            yiaddr:        l.addr.to_string(),
            siaddr:        l.srv_ip.to_string(),
            srv_id:        hex::encode(l.srv_duid),
            is_v6:         true,
            domain_name,
            dns_servers,
            ntp_servers,
            subnet_mask:   l.prefix_len.to_string(),
            mac_address:   String::new(),
            mtu:           0,
            broadcast_addr:String::new(),
            gateways:      Vec::new(),
            host_name:     String::new(),
        }
    }
}

fn parse_dns_servers(opts: &[DhcpOption]) -> Vec<String> {
    opts.iter()
        .find_map(|opt| {
            if let DhcpOption::DomainNameServers(servers) = opt {
                Some(servers.iter().map(ToString::to_string).collect())
            } else {
                None
            }
        })
        .unwrap_or_default()
}

fn parse_domain_search(opts: &[DhcpOption]) -> String {
    opts.iter()
        .find_map(|opt| {
            if let DhcpOption::DomainSearchList(domains) = opt {
                Some(domains.iter()
                            .map(ToString::to_string)
                            .collect::<Vec<_>>()
                            .join(" "))
            } else {
                None
            }
        })
        .unwrap_or_default()
}

fn parse_ntp_servers(opts: &[DhcpOption]) -> Vec<String> {
    opts.iter()
        .filter_map(|opt| match opt {
            DhcpOption::NtpServer(subopts) => Some(subopts),
            _ => None,
        })
        .flat_map(|subopts| subopts.iter().filter_map(|sub| match sub {
            NtpSuboption::ServerAddress(addr)
            | NtpSuboption::MulticastAddress(addr) => Some(addr.to_string()),
            NtpSuboption::FQDN(name) => Some(name.to_string()),
        }))
        .collect()
}
```

This keeps all the functionality but breaks the logic into focused, testable units.
</issue_to_address>