node: Address change-shard review feedback

Author: erayack

node/src/manager/commands/chain.rs

@@ -22,7 +22,6 @@ use graph::{
 use graph_chain_ethereum::EthereumAdapter;
 use graph_chain_ethereum::EthereumAdapterTrait as _;
 use graph_chain_ethereum::chain::BlockFinality;
-use graph_store_postgres::AsyncPgConnection;
 use graph_store_postgres::BlockStore;
 use graph_store_postgres::ChainStore;
 use graph_store_postgres::PoolCoordinator;
@@ -238,59 +237,6 @@ pub async fn update_chain_genesis(
     Ok(())
 }
 
-struct ChainSwapOutcome {
-    reused_previous_backup: bool,
-    allocated_chain: bool,
-}
-
-#[derive(Clone, Copy, Debug, PartialEq, Eq)]
-enum ExistingBackupDisposition<'a> {
-    ProceedFresh,
-    PromptReuse,
-    AbortWrongShard(&'a str),
-}
-
-fn backup_name(chain: &str, base: &str) -> String {
-    format!("{chain}-{base}")
-}
-
-fn suffixed_backup_name(backup_name: &str, suffix: usize) -> String {
-    format!("{backup_name}-{suffix}")
-}
-
-fn existing_backup_disposition<'a>(
-    existing_backup_shard: Option<&'a str>,
-    target_shard: &str,
-) -> ExistingBackupDisposition<'a> {
-    match existing_backup_shard {
-        None => ExistingBackupDisposition::ProceedFresh,
-        Some(shard) if shard == target_shard => ExistingBackupDisposition::PromptReuse,
-        Some(shard) => ExistingBackupDisposition::AbortWrongShard(shard),
-    }
-}
-
-async fn next_temporary_backup_name(
-    conn: &mut AsyncPgConnection,
-    chain: &str,
-    base: &str,
-) -> Result<String, StoreError> {
-    let backup_name = backup_name(chain, base);
-    if find_chain(conn, &backup_name).await?.is_none() {
-        return Ok(backup_name);
-    }
-
-    let mut suffix = 1usize;
-
-    loop {
-        let candidate = suffixed_backup_name(&backup_name, suffix);
-        if find_chain(conn, &candidate).await?.is_none() {
-            return Ok(candidate);
-        }
-
-        suffix += 1;
-    }
-}
-
 pub async fn change_block_cache_shard(
     primary_store: ConnectionPool,
     store: BlockStore,
@@ -316,28 +262,33 @@ pub async fn change_block_cache_shard(
         .ok_or_else(|| anyhow!("unknown chain: {}", &chain_name))?;
     let ident = chain_store.chain_identifier().await?;
     let target_shard = Shard::new(shard.clone())?;
-    let existing_backup_disposition = existing_backup_disposition(
-        existing_backup.as_ref().map(|backup| backup.shard.as_str()),
-        target_shard.as_str(),
-    );
-    let reuse_existing_backup = matches!(
-        existing_backup_disposition,
-        ExistingBackupDisposition::PromptReuse
-    );
 
-    match existing_backup_disposition {
-        ExistingBackupDisposition::ProceedFresh => {}
-        ExistingBackupDisposition::AbortWrongShard(backup_shard) => {
+    let reuse_existing_backup = match existing_backup.as_ref() {
+        None => false,
+        Some(backup) if backup.shard != target_shard => {
             bail!(
                 "`{}` already exists on shard `{}`. Remove it with `graphman chain remove {}` before changing `{}` to shard `{}`",
                 canonical_backup_name,
-                backup_shard,
+                backup.shard,
                 canonical_backup_name,
                 chain_name,
                 target_shard,
             );
         }
-        ExistingBackupDisposition::PromptReuse => {
+        Some(backup) => {
+            let backup_ident = backup.network_identifier()?;
+            if backup_ident != ident {
+                bail!(
+                    "`{}` has a different chain identifier ({}) than `{}` ({}). Remove it with `graphman chain remove {}` before changing `{}` to shard `{}`",
+                    canonical_backup_name,
+                    backup_ident,
+                    chain_name,
+                    ident,
+                    canonical_backup_name,
+                    chain_name,
+                    target_shard,
+                );
+            }
             let prompt = format!(
                 "`{}` already exists on shard `{}` and will be reused as the active `{}` chain.\nProceed?",
                 canonical_backup_name, target_shard, chain_name
@@ -349,8 +300,9 @@ pub async fn change_block_cache_shard(
                 );
                 return Ok(());
             }
+            true
         }
-    }
+    };
 
     let existing_backup_store = if reuse_existing_backup {
         store.chain_store(&canonical_backup_name).await
@@ -367,26 +319,32 @@ pub async fn change_block_cache_shard(
         Some(chain)
     };
 
-    let outcome = conn.transaction::<ChainSwapOutcome, StoreError, _>(|conn| {
+    let temp_backup_name = format!("{chain_name}-old-temp");
+    if reuse_existing_backup && find_chain(&mut conn, &temp_backup_name).await?.is_some() {
+        bail!(
+            "`{}` already exists. Remove it with `graphman chain remove {}` before changing `{}` to shard `{}`",
+            temp_backup_name,
+            temp_backup_name,
+            chain_name,
+            target_shard,
+        );
+    }
+
+    conn.transaction::<(), StoreError, _>(|conn| {
         async {
             sql_query(
                 "alter table deployment_schemas drop constraint deployment_schemas_network_fkey;",
             )
             .execute(conn).await?;
 
-            let temp_backup_name = if let Some(backup) = existing_backup.as_ref() {
-                let temp_name = next_temporary_backup_name(conn, &chain_name, "old").await?;
-                update_chain_name(conn, &backup.name, &temp_name).await?;
-                Some(temp_name)
-            } else {
-                None
-            };
+            if let Some(backup) = existing_backup.as_ref() {
+                update_chain_name(conn, &backup.name, &temp_backup_name).await?;
+            }
 
             update_chain_name(conn, &chain_name, &canonical_backup_name).await?;
 
             if reuse_existing_backup {
-                debug_assert!(temp_backup_name.is_some());
-                update_chain_name(conn, temp_backup_name.as_ref().unwrap(), &chain_name).await?;
+                update_chain_name(conn, &temp_backup_name, &chain_name).await?;
             } else {
                 add_chain(conn, &chain_name, &target_shard, ident.clone()).await?;
             }
@@ -396,21 +354,13 @@ pub async fn change_block_cache_shard(
             )
             .execute(conn).await?;
 
-            Ok(ChainSwapOutcome {
-                reused_previous_backup: reuse_existing_backup,
-                allocated_chain: allocated_chain.is_some(),
-            })
+            Ok(())
         }.scope_boxed()
     }).await?;
 
-    let ChainSwapOutcome {
-        reused_previous_backup,
-        allocated_chain,
-    } = outcome;
-
     chain_store.update_name(&canonical_backup_name).await?;
 
-    if reused_previous_backup && let Some(backup_store) = existing_backup_store.as_ref() {
+    if reuse_existing_backup && let Some(backup_store) = existing_backup_store.as_ref() {
         backup_store.update_name(&chain_name).await?;
     }
 
@@ -420,14 +370,14 @@ pub async fn change_block_cache_shard(
     );
     println!("Latest backup recorded as `{}`", canonical_backup_name);
 
-    if reused_previous_backup {
+    if reuse_existing_backup {
         println!(
             "Reused existing backup `{}` as the active `{}` chain",
             canonical_backup_name, chain_name
         );
     }
 
-    if allocated_chain {
+    if allocated_chain.is_some() {
         println!(
             "Allocated new chain state for `{}` on shard {}",
             chain_name, shard
@@ -467,45 +417,3 @@ pub async fn ingest(
     }
     Ok(())
 }
-
-#[cfg(test)]
-mod tests {
-    use super::{
-        ExistingBackupDisposition, backup_name, existing_backup_disposition, suffixed_backup_name,
-    };
-
-    #[test]
-    fn backup_name_uses_plain_name_first() {
-        assert_eq!(backup_name("mainnet", "old"), "mainnet-old");
-    }
-
-    #[test]
-    fn suffixed_backup_name_adds_numeric_suffixes() {
-        assert_eq!(suffixed_backup_name("mainnet-old", 1), "mainnet-old-1");
-        assert_eq!(suffixed_backup_name("mainnet-old", 42), "mainnet-old-42");
-    }
-
-    #[test]
-    fn existing_backup_disposition_proceeds_when_backup_missing() {
-        assert_eq!(
-            existing_backup_disposition(None, "shard_b"),
-            ExistingBackupDisposition::ProceedFresh
-        );
-    }
-
-    #[test]
-    fn existing_backup_disposition_prompts_when_backup_matches_target_shard() {
-        assert_eq!(
-            existing_backup_disposition(Some("shard_b"), "shard_b"),
-            ExistingBackupDisposition::PromptReuse
-        );
-    }
-
-    #[test]
-    fn existing_backup_disposition_aborts_when_backup_is_on_another_shard() {
-        assert_eq!(
-            existing_backup_disposition(Some("shard_a"), "shard_b"),
-            ExistingBackupDisposition::AbortWrongShard("shard_a")
-        );
-    }
-}