Address PR comments

crates/sui-graphql-rpc/src/types/dot_move/named_type.rs

@@ -2,17 +2,18 @@
 // SPDX-License-Identifier: Apache-2.0
 
 use std::collections::HashMap;
+use std::str::FromStr;
 
 use async_graphql::Context;
 use futures::future;
-use move_core_types::parser::parse_type_tag;
-use sui_types::base_types::ObjectID;
+use regex::{Captures, Regex};
+use sui_types::{base_types::ObjectID, TypeTag};
 
 use crate::error::Error;
 
 use super::{
-    config::{DotMoveServiceError, VERSIONED_NAME_UNBOUND_REG},
-    named_move_package::NamedMovePackage,
+    error::MoveRegistryError, named_move_package::NamedMovePackage,
+    on_chain::VERSIONED_NAME_UNBOUND_REG,
 };
 
 pub(crate) struct NamedType;
@@ -22,7 +23,7 @@ impl NamedType {
         ctx: &Context<'_>,
         name: &str,
         checkpoint_viewed_at: u64,
-    ) -> Result<String, Error> {
+    ) -> Result<TypeTag, Error> {
         // we do not de-duplicate the names here, as the dataloader will do this for us.
         let names = Self::parse_names(name)?;
 
@@ -41,18 +42,20 @@ impl NamedType {
 
         // doing it in reverse so we can pop instead of shift
         for name in names.into_iter().rev() {
-            // safe unwrap: we know that the amount of results has to equal the amount of names.
+            // SAFETY: we know that the amount of results has to equal the amount of names.
             let Some(package) = results.pop().unwrap() else {
-                return Err(Error::DotMove(DotMoveServiceError::NameNotFound(name)));
+                return Err(Error::MoveNameRegistry(MoveRegistryError::NameNotFound(
+                    name,
+                )));
             };
 
             name_package_id_mapping.insert(name, package.native.id());
         }
 
-        let correct_type_tag = Self::replace_names(name, &name_package_id_mapping);
+        let correct_type_tag: String = Self::replace_names(name, &name_package_id_mapping)?;
 
         // now we query the names with futures to utilize data loader
-        Ok(correct_type_tag)
+        TypeTag::from_str(&correct_type_tag).map_err(|e| Error::Client(format!("bad type: {e}")))
     }
 
     // TODO: Should we introduce some overall string limit length here?
@@ -64,41 +67,67 @@ impl NamedType {
     fn parse_names(name: &str) -> Result<Vec<String>, Error> {
         let mut names = vec![];
         let struct_tag = VERSIONED_NAME_UNBOUND_REG.replace_all(name, |m: &regex::Captures| {
-            // safe unwrap: we know that the regex will always have a match on position 0.
+            // SAFETY: we know that the regex will always have a match on position 0.
             names.push(m.get(0).unwrap().as_str().to_string());
             "0x0".to_string()
         });
 
         // We attempt to parse the type_tag with these replacements, to make sure there are no other
         // errors in the type tag (apart from the move names). That protects us from unnecessary
         // queries to resolve .move names, for a type tag that will be invalid anyway.
-        parse_type_tag(&struct_tag).map_err(|e| Error::Client(e.to_string()))?;
+        TypeTag::from_str(&struct_tag).map_err(|e| Error::Client(e.to_string()))?;
 
         Ok(names)
     }
 
     // This function replaces all the names in the type tag with their corresponding MovePackage address.
     // The names are guaranteed to be the same and exist (as long as this is called in sequence),
     // since we use the same parser to extract the names.
-    fn replace_names(type_name: &str, names: &HashMap<String, ObjectID>) -> String {
-        let struct_tag_str =
-            VERSIONED_NAME_UNBOUND_REG.replace_all(type_name, |m: &regex::Captures| {
-                // safe unwrap: we know that the regex will have a match on position 0.
+    fn replace_names(type_name: &str, names: &HashMap<String, ObjectID>) -> Result<String, Error> {
+        let struct_tag_str = replace_all(
+            &VERSIONED_NAME_UNBOUND_REG,
+            type_name,
+            |m: &regex::Captures| {
+                // SAFETY: we know that the regex will have a match on position 0.
                 let name = m.get(0).unwrap().as_str();
 
                 // if we are miss-using the function, and we cannot find the name in the hashmap,
                 // we return an empty string, which will make the type tag invalid.
                 if let Some(addr) = names.get(name) {
-                    addr.to_string()
+                    Ok(addr.to_string())
                 } else {
-                    "".to_string()
+                    Err(Error::MoveNameRegistry(MoveRegistryError::NameNotFound(
+                        name.to_string(),
+                    )))
                 }
-            });
+            },
+        )?;
 
-        struct_tag_str.to_string()
+        Ok(struct_tag_str.to_string())
     }
 }
 
+// Helper to replace all occurrences of a regex with a function that returns a string.
+// Used as a replacement of `regex`.replace_all().
+// The only difference is that this function returns a Result, so we can handle errors.
+fn replace_all(
+    re: &Regex,
+    haystack: &str,
+    replacement: impl Fn(&Captures) -> Result<String, Error>,
+) -> Result<String, Error> {
+    let mut new = String::with_capacity(haystack.len());
+    let mut last_match = 0;
+    for caps in re.captures_iter(haystack) {
+        // SAFETY: we know that the regex will have a match on position 0.
+        let m = caps.get(0).unwrap();
+        new.push_str(&haystack[last_match..m.start()]);
+        new.push_str(&replacement(&caps)?);
+        last_match = m.end();
+    }
+    new.push_str(&haystack[last_match..]);
+    Ok(new)
+}
+
 #[cfg(test)]
 mod tests {
     use std::collections::HashMap;
@@ -169,7 +198,7 @@ mod tests {
             }
 
             let replaced = NamedType::replace_names(&data.input_type, &mapping);
-            assert_eq!(replaced, data.expected_output);
+            assert_eq!(replaced.unwrap(), data.expected_output);
         }
     }
 

crates/sui-graphql-rpc/src/types/query.rs

@@ -526,11 +526,7 @@ impl Query {
         let Watermark { checkpoint, .. } = *ctx.data()?;
         let type_tag = NamedType::query(ctx, &name, checkpoint).await?;
 
-        Ok(MoveType::new(
-            TypeTag::from_str(&type_tag)
-                .map_err(|e| Error::Client(format!("Bad type: {e}")))
-                .extend()?,
-        ))
+        Ok(MoveType::new(type_tag))
     }
 
     /// The coin metadata associated with the given coin type.