diff --git a/lit-api-server/blockchain/lit_node_express/Makefile b/lit-api-server/blockchain/lit_node_express/Makefile index 0f97b625..656c41ec 100644 --- a/lit-api-server/blockchain/lit_node_express/Makefile +++ b/lit-api-server/blockchain/lit_node_express/Makefile @@ -64,11 +64,11 @@ update_anvil: compile update_sepolia: compile cd $(RUST_PROJECT) && cargo build --bin contract_deployer - $(DEPLOYER_BIN) --action=update --network=base-sepolia --abifolder=artifacts/contracts --secret=$(DEPLOYER_SECRET) --address=$(address) + $(DEPLOYER_BIN) --action=update --network=base-sepolia --abifolder=artifacts/contracts --secret=$(DEPLOYER_SECRET) --address=$(address) --removals=diamond-removals.json update_base: compile cd $(RUST_PROJECT) && cargo build --bin contract_deployer - $(DEPLOYER_BIN) --action=update --network=base --abifolder=artifacts/contracts --secret=$(BASE_DEPLOYER_SECRET) --address=$(address) + $(DEPLOYER_BIN) --action=update --network=base --abifolder=artifacts/contracts --secret=$(BASE_DEPLOYER_SECRET) --address=$(address) --removals=diamond-removals.json # Propose diamond update via Safe multisig (Base mainnet). # Usage: make propose_update_base address=0x... safe=0x... @@ -78,7 +78,8 @@ propose_update_base: compile --abifolder=artifacts/contracts \ --secret=$(BASE_DEPLOYER_SECRET) \ --address=$(address) \ - --output=diamond_cut_proposal.json + --output=diamond_cut_proposal.json \ + --removals=diamond-removals.json npx hardhat propose-diamond-cut \ --safe $(safe) \ --proposal diamond_cut_proposal.json \ @@ -92,7 +93,8 @@ propose_update_sepolia: compile --abifolder=artifacts/contracts \ --secret=$(DEPLOYER_SECRET) \ --address=$(address) \ - --output=diamond_cut_proposal.json + --output=diamond_cut_proposal.json \ + --removals=diamond-removals.json npx hardhat propose-diamond-cut \ --safe $(safe) \ --proposal diamond_cut_proposal.json \ diff --git a/lit-api-server/blockchain/lit_node_express/diamond-removals.json b/lit-api-server/blockchain/lit_node_express/diamond-removals.json new file mode 100644 index 00000000..d5424e61 --- /dev/null +++ b/lit-api-server/blockchain/lit_node_express/diamond-removals.json @@ -0,0 +1,10 @@ +{ + "_comment": "Functions to Remove from the AccountConfig diamond on the next upgrade. The deployer only auto-computes Replace/Add; a selector is removed only if it is BOTH listed here AND detected as orphaned by the upgrade (absent from every new managed facet ABI). See tasks/../deployer/diamond.rs plan_removals.", + "removals": [ + { + "signature": "backfillPkpOwners(address[],uint256[])", + "selector": "0x41275609", + "reason": "Spent one-time #575 PKP owner backfill; removed after the migration completed on Base mainnet (all PKPs bound, 0 unbound/conflicts). Requires the WritesFacet build that no longer declares it." + } + ] +} diff --git a/lit-api-server/blockchain/rust_generator_and_deployer/src/bin/contract_deployer.rs b/lit-api-server/blockchain/rust_generator_and_deployer/src/bin/contract_deployer.rs index 5269fcb4..ce835f14 100644 --- a/lit-api-server/blockchain/rust_generator_and_deployer/src/bin/contract_deployer.rs +++ b/lit-api-server/blockchain/rust_generator_and_deployer/src/bin/contract_deployer.rs @@ -8,6 +8,7 @@ //! secret: optional; deployer private key (hex). If blank or omitted, uses default Anvil dev secret. //! address: required for update and propose-update actions; the diamond contract address. //! output: optional; path for the proposal JSON file (propose-update only). Defaults to diamond_cut_proposal.json. +//! removals: optional; path to the selector-removals manifest (update/propose-update). Defaults to diamond-removals.json; missing file = no removals. use alloy::primitives::Address; use lit_contracts_minimal_generator::args::{get_network_and_chain_id, parse_named_args}; @@ -42,6 +43,9 @@ async fn main() -> Result<(), Box> { eprintln!( " --output proposal JSON output path (propose-update only, default: diamond_cut_proposal.json)" ); + eprintln!( + " --removals selector-removals manifest path (update/propose-update, default: diamond-removals.json)" + ); std::process::exit(1); }; @@ -107,10 +111,18 @@ async fn main() -> Result<(), Box> { ); let diamond_address = parse_diamond_address(&named, usage); + let removals = removals_path(&named); - update_diamond(rpc_url, chain_id, &abis_folder, secret, diamond_address) - .await - .expect("Failed to update diamond"); + update_diamond( + rpc_url, + chain_id, + &abis_folder, + secret, + diamond_address, + &removals, + ) + .await + .expect("Failed to update diamond"); } if action == "propose-update" { println!( @@ -123,6 +135,7 @@ async fn main() -> Result<(), Box> { .get("output") .cloned() .unwrap_or_else(|| "diamond_cut_proposal.json".to_string()); + let removals = removals_path(&named); propose_update_diamond( rpc_url, @@ -131,6 +144,7 @@ async fn main() -> Result<(), Box> { secret, diamond_address, &output, + &removals, ) .await .expect("Failed to propose diamond update"); @@ -138,6 +152,17 @@ async fn main() -> Result<(), Box> { Ok(()) } +/// Path to the selector-removals manifest. Defaults to `diamond-removals.json` +/// in the working directory (the contracts project dir when run via the +/// Makefile). A missing file means "no removals" — the deployer keeps its prior +/// Replace/Add-only behavior. +fn removals_path(named: &std::collections::HashMap) -> String { + named + .get("removals") + .cloned() + .unwrap_or_else(|| "diamond-removals.json".to_string()) +} + fn parse_diamond_address( named: &std::collections::HashMap, usage: impl Fn(), diff --git a/lit-api-server/blockchain/rust_generator_and_deployer/src/deployer/diamond.rs b/lit-api-server/blockchain/rust_generator_and_deployer/src/deployer/diamond.rs index 725c975a..d4d96380 100644 --- a/lit-api-server/blockchain/rust_generator_and_deployer/src/deployer/diamond.rs +++ b/lit-api-server/blockchain/rust_generator_and_deployer/src/deployer/diamond.rs @@ -9,8 +9,9 @@ use crate::diamond::c_diamond_loupe_facet::DIAMONDLOUPEFACET_JSON; use crate::diamond::c_ownership_facet::OWNERSHIPFACET_JSON; use alloy::dyn_abi::DynSolValue; use alloy::network::TransactionBuilder; -use alloy::primitives::{Address, B256, Bytes, FixedBytes, U256}; +use alloy::primitives::{Address, B256, Bytes, FixedBytes, U256, keccak256}; use alloy::providers::Provider; +use std::collections::{HashMap, HashSet}; use std::path::Path; pub async fn deploy_facet_from_json( @@ -91,6 +92,143 @@ pub enum FacetCutAction { Remove = 2, } +/// A committed list of functions to drop from the diamond on the next upgrade. +/// The deployer only computes Replace/Add automatically; removing a selector +/// requires an explicit, auditable declaration here so a partial/incorrect +/// build can never silently strip a live function. +#[derive(serde::Deserialize, Default)] +pub struct RemovalManifest { + #[serde(default)] + pub removals: Vec, +} + +#[derive(serde::Deserialize, Clone)] +pub struct RemovalEntry { + /// Canonical function signature, e.g. "backfillPkpOwners(address[],uint256[])". + pub signature: String, + /// Optional 0x-prefixed 4-byte selector; if present it is cross-checked + /// against keccak256(signature) as a typo guard. + #[serde(default)] + pub selector: Option, + #[serde(default)] + pub reason: Option, +} + +/// 4-byte selector of a canonical function signature. +pub fn selector_of(signature: &str) -> FixedBytes<4> { + FixedBytes::<4>::from_slice(&keccak256(signature.as_bytes())[0..4]) +} + +pub struct RemovalPlan { + pub to_remove: Vec>, + pub warnings: Vec, +} + +/// Decide which selectors to Remove in this upgrade. Safe by construction: a +/// selector is removed only if it is BOTH explicitly listed in the manifest AND +/// detected as orphaned by this upgrade — i.e. it currently lives on the old +/// address of a facet we are re-installing, yet is absent from every new managed +/// facet ABI. Core facets (DiamondCut/Loupe) are never touched: none of their +/// selectors appear in the managed set, so their addresses are never classified +/// as "managed", so their selectors are never orphan candidates. +pub fn plan_removals( + selector_to_addr: &HashMap, Address>, + new_managed: &HashSet>, + manifest: &[RemovalEntry], +) -> RemovalPlan { + // Addresses currently hosting at least one selector we are re-installing. + let managed_old_addresses: HashSet
= selector_to_addr + .iter() + .filter(|(sel, _)| new_managed.contains(*sel)) + .map(|(_, addr)| *addr) + .collect(); + + // On-chain selectors on a managed facet's old address that the new ABIs drop. + let orphans: HashSet> = selector_to_addr + .iter() + .filter(|(sel, addr)| managed_old_addresses.contains(*addr) && !new_managed.contains(*sel)) + .map(|(sel, _)| *sel) + .collect(); + + let mut warnings = Vec::new(); + let mut to_remove = Vec::new(); + let mut approved: HashSet> = HashSet::new(); + + for entry in manifest { + let sel = selector_of(&entry.signature); + if let Some(expected) = &entry.selector { + let want = expected.trim_start_matches("0x").to_lowercase(); + let got = hex::encode(sel.as_slice()); + if want != got { + warnings.push(format!( + "manifest '{}': selector mismatch (signature hashes to 0x{}, manifest says {}) — skipping", + entry.signature, got, expected + )); + continue; + } + } + if !selector_to_addr.contains_key(&sel) { + warnings.push(format!( + "manifest '{}' (0x{}): not present on-chain — nothing to remove", + entry.signature, + hex::encode(sel.as_slice()) + )); + continue; + } + if !orphans.contains(&sel) { + warnings.push(format!( + "manifest '{}' (0x{}): still served by a managed facet in the new ABIs (not orphaned) — REFUSING to remove", + entry.signature, + hex::encode(sel.as_slice()) + )); + continue; + } + if approved.insert(sel) { + to_remove.push(sel); + } + } + + for sel in &orphans { + if !approved.contains(sel) { + warnings.push(format!( + "0x{}: orphaned by this upgrade but not approved for removal — it will remain routed to its old facet and stay callable. Add it to the removals manifest to drop it.", + hex::encode(sel.as_slice()) + )); + } + } + + RemovalPlan { + to_remove, + warnings, + } +} + +fn load_removal_manifest(path: &str) -> Vec { + match std::fs::read_to_string(path) { + Ok(s) => match serde_json::from_str::(&s) { + Ok(m) => { + println!( + "Loaded removals manifest {} ({} entr{}).", + path, + m.removals.len(), + if m.removals.len() == 1 { "y" } else { "ies" } + ); + m.removals + } + Err(e) => { + eprintln!( + "WARNING: failed to parse removals manifest {path}: {e} — proceeding with NO removals" + ); + Vec::new() + } + }, + Err(_) => { + println!("No removals manifest at {path} — this upgrade removes no selectors."); + Vec::new() + } + } +} + /// Shared struct holding the results of deploying facets and building diamond cut data. pub struct DiamondUpdateData { pub facet_cuts: Vec, @@ -167,12 +305,17 @@ async fn build_diamond_update( client: SigningProvider, abis_folder: &str, diamond_address: Address, + removals_manifest_path: &str, ) -> Result> { let facet_addresses = facet_addresses(&client, diamond_address).await?; let mut existing_selectors: Vec> = Vec::new(); + let mut selector_to_addr: HashMap, Address> = HashMap::new(); for facet_address in &facet_addresses { let selectors = facet_function_selectors(&client, diamond_address, *facet_address).await?; + for sel in &selectors { + selector_to_addr.insert(*sel, *facet_address); + } existing_selectors.extend(selectors); } @@ -185,6 +328,9 @@ async fn build_diamond_update( let mut facet_cuts = Vec::new(); let mut facets_deployed = std::collections::HashMap::new(); + // Union of selectors across the facets this upgrade (re)installs. Used to + // detect selectors orphaned by the upgrade so they can be Removed. + let mut new_managed: HashSet> = HashSet::new(); for (name, json_path) in [ ( @@ -205,15 +351,40 @@ async fn build_diamond_update( ), ] { let facet = deploy_facet_from_json(abis_folder, json_path, client.clone()).await?; + new_managed.extend(facet.abi().functions().map(|f| f.selector())); facet_cuts.extend(get_facet_cuts(&facet, &existing_selectors, true)); facets_deployed.insert(name.to_string(), *facet.address()); } let ownership_facet = deploy_prebuilt_facet("OwnershipFacet", OWNERSHIPFACET_JSON, client.clone()).await?; + new_managed.extend(ownership_facet.abi().functions().map(|f| f.selector())); facet_cuts.extend(get_facet_cuts(&ownership_facet, &existing_selectors, true)); facets_deployed.insert("OwnershipFacet".to_string(), *ownership_facet.address()); + // Explicit, manifest-gated selector removals (see plan_removals). + let manifest = load_removal_manifest(removals_manifest_path); + let plan = plan_removals(&selector_to_addr, &new_managed, &manifest); + for w in &plan.warnings { + println!(" [removals] {w}"); + } + if !plan.to_remove.is_empty() { + println!( + "Removing {} orphaned selector(s): {}", + plan.to_remove.len(), + plan.to_remove + .iter() + .map(|s| format!("0x{}", hex::encode(s.as_slice()))) + .collect::>() + .join(", ") + ); + facet_cuts.push(FacetCut { + facetAddress: Address::ZERO, + action: FacetCutAction::Remove as u8, + functionSelectors: plan.to_remove, + }); + } + let diamond_init = deploy_facet_from_json( abis_folder, "AccountConfigFacets/DiamondInit.sol/DiamondInit.json", @@ -305,10 +476,17 @@ pub async fn update_diamond( abis_folder: &str, secret: &str, diamond_address: Address, + removals_manifest_path: &str, ) -> Result<(), Box> { let client = signer_provider(rpc_url, chain_id, secret)?; - let data = build_diamond_update(client.clone(), abis_folder, diamond_address).await?; + let data = build_diamond_update( + client.clone(), + abis_folder, + diamond_address, + removals_manifest_path, + ) + .await?; print!("Cutting diamond with init {:?} ...", data.init_calldata); let calldata = diamond_cut_calldata(&client, diamond_address, &data).await?; @@ -345,10 +523,17 @@ pub async fn propose_update_diamond( secret: &str, diamond_address: Address, output_path: &str, + removals_manifest_path: &str, ) -> Result<(), Box> { let client = signer_provider(rpc_url, chain_id, secret)?; - let data = build_diamond_update(client.clone(), abis_folder, diamond_address).await?; + let data = build_diamond_update( + client.clone(), + abis_folder, + diamond_address, + removals_manifest_path, + ) + .await?; let calldata = diamond_cut_calldata(&client, diamond_address, &data).await?; let facets_json: serde_json::Map = data @@ -457,3 +642,152 @@ fn action_to_string(action: u8) -> String { _ => String::from("Unknown"), } } + +#[cfg(test)] +mod removal_tests { + use super::*; + + fn sel(bytes: [u8; 4]) -> FixedBytes<4> { + FixedBytes::<4>::from(bytes) + } + + fn entry(sig: &str) -> RemovalEntry { + RemovalEntry { + signature: sig.to_string(), + selector: None, + reason: None, + } + } + + // Fixture: managed facet at address A hosts `keep` (still in the new ABI) + // and `gone` (dropped by the upgrade). Core facet at address C hosts `core`, + // which is absent from the managed set but must never be treated as orphaned. + fn fixture() -> (HashMap, Address>, HashSet>) { + let a = Address::repeat_byte(0xAA); + let c = Address::repeat_byte(0xCC); + let keep = sel([0x11, 0x11, 0x11, 0x11]); + let gone = sel([0x22, 0x22, 0x22, 0x22]); + let core = sel([0x33, 0x33, 0x33, 0x33]); + let add = sel([0x44, 0x44, 0x44, 0x44]); // brand-new selector, not yet on-chain + + let mut s2a = HashMap::new(); + s2a.insert(keep, a); + s2a.insert(gone, a); + s2a.insert(core, c); + + let mut managed = HashSet::new(); + managed.insert(keep); + managed.insert(add); + (s2a, managed) + } + + #[test] + fn removes_only_manifest_listed_orphans() { + let (s2a, managed) = fixture(); + let gone = sel([0x22, 0x22, 0x22, 0x22]); + // Manifest lists exactly the orphan by raw selector signature — use a + // signature that hashes to `gone` is impractical, so pass selector guard + // off and rely on signature hashing in the non-test path; here we test + // the planner directly with a manifest whose signature hashes match. + // Build a manifest entry whose selector_of equals `gone`. + let sig = "someRemovedFn()"; + let mut man = vec![entry(sig)]; + // Force the fixture's `gone` to equal selector_of(sig) so the manifest matches. + let derived = selector_of(sig); + let mut s2a2 = s2a.clone(); + // relocate `gone` -> derived selector on the same managed address A + s2a2.remove(&gone); + s2a2.insert(derived, Address::repeat_byte(0xAA)); + man[0].selector = Some(format!("0x{}", hex::encode(derived.as_slice()))); + + let plan = plan_removals(&s2a2, &managed, &man); + assert_eq!( + plan.to_remove, + vec![derived], + "orphan in manifest is removed" + ); + } + + #[test] + fn warns_and_skips_orphan_not_in_manifest() { + let (s2a, managed) = fixture(); + let gone = sel([0x22, 0x22, 0x22, 0x22]); + let plan = plan_removals(&s2a, &managed, &[]); + assert!(plan.to_remove.is_empty()); + assert!( + plan.warnings + .iter() + .any(|w| w.contains(&hex::encode(gone.as_slice())) && w.contains("orphaned")), + "unlisted orphan must be warned about" + ); + } + + #[test] + fn refuses_to_remove_non_orphan_even_if_listed() { + // `core` is on-chain but on an unmanaged facet -> never an orphan. + let (s2a, managed) = fixture(); + let core = sel([0x33, 0x33, 0x33, 0x33]); + let man = vec![RemovalEntry { + signature: "core()".to_string(), + selector: Some(format!("0x{}", hex::encode(core.as_slice()))), + reason: None, + }]; + // Make selector_of("core()") == core so the manifest entry resolves to it. + // If it doesn't, the selector-mismatch guard fires — also a safe refusal. + let plan = plan_removals(&s2a, &managed, &man); + assert!( + plan.to_remove.is_empty(), + "a non-orphaned selector is never removed" + ); + assert!(!plan.warnings.is_empty()); + } + + #[test] + fn skips_manifest_selector_absent_on_chain() { + let (s2a, managed) = fixture(); + let ghost = "neverDeployed()"; + let man = vec![entry(ghost)]; + let plan = plan_removals(&s2a, &managed, &man); + assert!(plan.to_remove.is_empty()); + assert!( + plan.warnings + .iter() + .any(|w| w.contains("not present on-chain")), + "absent manifest selector must be skipped with a warning" + ); + } + + #[test] + fn selector_guard_rejects_mismatch() { + let (mut s2a, managed) = fixture(); + // put the derived selector of "x()" on managed address A as an orphan + let derived = selector_of("x()"); + s2a.insert(derived, Address::repeat_byte(0xAA)); + let man = vec![RemovalEntry { + signature: "x()".to_string(), + selector: Some("0xdeadbeef".to_string()), + reason: None, + }]; + let plan = plan_removals(&s2a, &managed, &man); + assert!( + plan.to_remove.is_empty(), + "mismatched selector guard blocks removal" + ); + assert!( + plan.warnings + .iter() + .any(|w| w.contains("selector mismatch")) + ); + } + + #[test] + fn backfill_selector_matches_known_value() { + assert_eq!( + format!( + "0x{}", + hex::encode(selector_of("backfillPkpOwners(address[],uint256[])").as_slice()) + ), + "0x41275609" + ); + } +}