Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: contracts function name clashing #2603

Merged
merged 2 commits into from
Nov 1, 2024
Merged

fix: contracts function name clashing #2603

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7b2ba2e
fix: contracts function name clashing
MartianGreed Nov 1, 2024
48188c1
fix: formatting
glihm Nov 1, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
94 changes: 93 additions & 1 deletion crates/dojo/bindgen/src/plugins/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,17 +48,84 @@
self.0.push(s.clone());
}

/// Inserts string after the first occurrence of the separator.
///
/// * `s` - The string to insert.
/// * `pos` - The string inside inner vec to search position for.
/// * `sep` - The separator to search for.
/// * `idx` - The index of the separator to insert after.
///
pub fn insert_after(&mut self, s: String, pos: &str, sep: &str, idx: usize) {
let pos = self.0.iter().position(|b| b.contains(pos)).unwrap();
let pos = self.pos(pos).unwrap();
if let Some(st) = self.0.get_mut(pos) {
let indices = st.match_indices(sep).map(|(i, _)| i).collect::<Vec<usize>>();
let append_after = indices[indices.len() - idx] + 1;
st.insert_str(append_after, &s);
}
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Ohayo sensei! Consider adding error handling to insert_after.

The function could panic if:

  1. pos() returns None (unwrap)
  2. idx is >= indices.length

Here's a safer implementation:

 pub fn insert_after(&mut self, s: String, pos: &str, sep: &str, idx: usize) -> Option<()> {
-    let pos = self.pos(pos).unwrap();
+    let pos = self.pos(pos)?;
     if let Some(st) = self.0.get_mut(pos) {
         let indices = st.match_indices(sep).map(|(i, _)| i).collect::<Vec<usize>>();
+        if idx >= indices.len() {
+            return None;
+        }
         let append_after = indices[indices.len() - idx] + 1;
         st.insert_str(append_after, &s);
+        Some(())
+    } else {
+        None
     }
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
/// Inserts string after the first occurrence of the separator.
///
/// * `s` - The string to insert.
/// * `pos` - The string inside inner vec to search position for.
/// * `sep` - The separator to search for.
/// * `idx` - The index of the separator to insert after.
///
pub fn insert_after(&mut self, s: String, pos: &str, sep: &str, idx: usize) {
let pos = self.0.iter().position(|b| b.contains(pos)).unwrap();
let pos = self.pos(pos).unwrap();
if let Some(st) = self.0.get_mut(pos) {
let indices = st.match_indices(sep).map(|(i, _)| i).collect::<Vec<usize>>();
let append_after = indices[indices.len() - idx] + 1;
st.insert_str(append_after, &s);
}
}
/// Inserts string after the first occurrence of the separator.
///
/// * `s` - The string to insert.
/// * `pos` - The string inside inner vec to search position for.
/// * `sep` - The separator to search for.
/// * `idx` - The index of the separator to insert after.
///
pub fn insert_after(&mut self, s: String, pos: &str, sep: &str, idx: usize) -> Option<()> {
let pos = self.pos(pos)?;
if let Some(st) = self.0.get_mut(pos) {
let indices = st.match_indices(sep).map(|(i, _)| i).collect::<Vec<usize>>();
if idx >= indices.len() {
return None;
}
let append_after = indices[indices.len() - idx] + 1;
st.insert_str(append_after, &s);
Some(())
} else {
None
}
}


/// Inserts string at the specified position.
///
/// * `s` - The string to insert.
/// * `pos` - The position to insert the string at.
/// * `idx` - The index of the string to insert at.
///
pub fn insert_at(&mut self, s: String, pos: usize, idx: usize) {
if let Some(st) = self.0.get_mut(idx) {
st.insert_str(pos + 1, &s);
}
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Add bounds checking to insert_at, sensei.

The function should validate that pos is within the string's bounds.

Here's a safer implementation:

-pub fn insert_at(&mut self, s: String, pos: usize, idx: usize) {
+pub fn insert_at(&mut self, s: String, pos: usize, idx: usize) -> Option<()> {
     if let Some(st) = self.0.get_mut(idx) {
+        if pos >= st.len() {
+            return None;
+        }
         st.insert_str(pos + 1, &s);
+        Some(())
+    } else {
+        None
     }
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
/// Inserts string at the specified position.
///
/// * `s` - The string to insert.
/// * `pos` - The position to insert the string at.
/// * `idx` - The index of the string to insert at.
///
pub fn insert_at(&mut self, s: String, pos: usize, idx: usize) {
if let Some(st) = self.0.get_mut(idx) {
st.insert_str(pos + 1, &s);
}
}
/// Inserts string at the specified position.
///
/// * `s` - The string to insert.
/// * `pos` - The position to insert the string at.
/// * `idx` - The index of the string to insert at.
///
pub fn insert_at(&mut self, s: String, pos: usize, idx: usize) -> Option<()> {
if let Some(st) = self.0.get_mut(idx) {
if pos >= st.len() {
return None;
}
st.insert_str(pos + 1, &s);
Some(())
} else {
None
}
}


/// Finds position of the given string in the inner vec.
///
/// * `pos` - The string to search for.
///
pub fn pos(&self, pos: &str) -> Option<usize> {
self.0.iter().position(|b| b.contains(pos))
}

pub fn join(&mut self, sep: &str) -> String {
self.0.join(sep)
}

/// At given index, finds the first occurrence of the needle string after the search string.
///
/// * `needle` - The string to search for.
/// * `search` - The string to search after.
/// * `idx` - The index to search at.
///
pub fn get_first_after(&self, needle: &str, search: &str, idx: usize) -> Option<usize> {
if let Some(st) = self.0.get(idx) {
let indices = st.match_indices(needle).map(|(i, _)| i).collect::<Vec<usize>>();
if indices.is_empty() {
return None;

Check warning on line 101 in crates/dojo/bindgen/src/plugins/mod.rs

View check run for this annotation

Codecov / codecov/patch

crates/dojo/bindgen/src/plugins/mod.rs#L101 

Added line #L101 was not covered by tests
}

let start = indices[indices.len() - 1] + 1;
let search_indices = st.match_indices(search).map(|(i, _)| i).collect::<Vec<usize>>();
return search_indices.iter().filter(|&&i| i > start).min().copied();
}
None

Check warning on line 108 in crates/dojo/bindgen/src/plugins/mod.rs

View check run for this annotation

Codecov / codecov/patch

crates/dojo/bindgen/src/plugins/mod.rs#L107-L108 

Added lines #L107 - L108 were not covered by tests
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Optimize performance with iterator chaining, sensei.

The current implementation collects indices into vectors twice. We can make this more efficient.

Consider this more efficient implementation:

 pub fn get_first_after(&self, needle: &str, search: &str, idx: usize) -> Option<usize> {
     if let Some(st) = self.0.get(idx) {
-        let indices = st.match_indices(needle).map(|(i, _)| i).collect::<Vec<usize>>();
-        if indices.is_empty() {
-            return None;
-        }
-
-        let start = indices[indices.len() - 1] + 1;
-        let search_indices = st.match_indices(search).map(|(i, _)| i).collect::<Vec<usize>>();
-        return search_indices.iter().filter(|&&i| i > start).min().copied();
+        let last_needle = st.match_indices(needle).map(|(i, _)| i).last()?;
+        st.match_indices(search)
+            .map(|(i, _)| i)
+            .filter(|&i| i > last_needle + 1)
+            .min()
     }
-    None
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
/// At given index, finds the first occurrence of the needle string after the search string.
///
/// * `needle` - The string to search for.
/// * `search` - The string to search after.
/// * `idx` - The index to search at.
///
pub fn get_first_after(&self, needle: &str, search: &str, idx: usize) -> Option<usize> {
if let Some(st) = self.0.get(idx) {
let indices = st.match_indices(needle).map(|(i, _)| i).collect::<Vec<usize>>();
if indices.is_empty() {
return None;
}
let start = indices[indices.len() - 1] + 1;
let search_indices = st.match_indices(search).map(|(i, _)| i).collect::<Vec<usize>>();
return search_indices.iter().filter(|&&i| i > start).min().copied();
}
None
}
/// At given index, finds the first occurrence of the needle string after the search string.
///
/// * `needle` - The string to search for.
/// * `search` - The string to search after.
/// * `idx` - The index to search at.
///
pub fn get_first_after(&self, needle: &str, search: &str, idx: usize) -> Option<usize> {
if let Some(st) = self.0.get(idx) {
let last_needle = st.match_indices(needle).map(|(i, _)| i).last()?;
st.match_indices(search)
.map(|(i, _)| i)
.filter(|&i| i > last_needle + 1)
.min()
}
}


/// At given index, finds the first occurrence of the needle string before the position in
/// string
///
/// * `search` - The token to search for.
/// * `pos` - Starting position of the search.
/// * `idx` - The index to search at.
///
pub fn get_first_before_pos(&self, search: &str, pos: usize, idx: usize) -> Option<usize> {
if let Some(st) = self.0.get(idx) {
let indices = st.match_indices(search).map(|(i, _)| i).collect::<Vec<usize>>();
if indices.is_empty() {
return None;

Check warning on line 122 in crates/dojo/bindgen/src/plugins/mod.rs

View check run for this annotation

Codecov / codecov/patch

crates/dojo/bindgen/src/plugins/mod.rs#L122 

Added line #L122 was not covered by tests
}

return indices.iter().filter(|&&i| i < pos).max().copied();
}
None

Check warning on line 127 in crates/dojo/bindgen/src/plugins/mod.rs

View check run for this annotation

Codecov / codecov/patch

crates/dojo/bindgen/src/plugins/mod.rs#L126-L127 

Added lines #L126 - L127 were not covered by tests
}
Comment on lines +113 to +123
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Optimize with iterator chaining here too, sensei.

Similar to get_first_after, we can make this more efficient.

Consider this more efficient implementation:

 pub fn get_first_before_pos(&self, search: &str, pos: usize, idx: usize) -> Option<usize> {
     if let Some(st) = self.0.get(idx) {
-        let indices = st.match_indices(search).map(|(i, _)| i).collect::<Vec<usize>>();
-        if indices.is_empty() {
-            return None;
-        }
-
-        return indices.iter().filter(|&&i| i < pos).max().copied();
+        st.match_indices(search)
+            .map(|(i, _)| i)
+            .filter(|&i| i < pos)
+            .max()
     }
-    None
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
pub fn get_first_before_pos(&self, search: &str, pos: usize, idx: usize) -> Option<usize> {
if let Some(st) = self.0.get(idx) {
let indices = st.match_indices(search).map(|(i, _)| i).collect::<Vec<usize>>();
if indices.is_empty() {
return None;
}
return indices.iter().filter(|&&i| i < pos).max().copied();
}
None
}
pub fn get_first_before_pos(&self, search: &str, pos: usize, idx: usize) -> Option<usize> {
if let Some(st) = self.0.get(idx) {
st.match_indices(search)
.map(|(i, _)| i)
.filter(|&i| i < pos)
.max()
}
}

}

impl Deref for Buffer {
Expand Down Expand Up @@ -112,3 +179,28 @@
buffer: &mut Buffer,
) -> BindgenResult<String>;
}

#[cfg(test)]
mod tests {
use super::*;

#[test]
fn test_buffer_get_first_after() {
let mut buff = Buffer::new();
buff.push("import { DojoProvider } from \"@dojoengine/core\";".to_owned());
buff.push("return { actions: { changeTheme, increaseGlobalCounter, } };".to_owned());
let pos = buff.get_first_after("actions: {", "}", 1);

assert_eq!(pos, Some(56));
}

#[test]
fn test_buffer_get_first_before() {
let mut buff = Buffer::new();
buff.push("import { DojoProvider } from \"@dojoengine/core\";".to_owned());
buff.push("return { actions: { changeTheme, increaseGlobalCounter, } };".to_owned());
let pos = buff.get_first_before_pos(",", 56, 1);

assert_eq!(pos, Some(54));
}
}
114 changes: 95 additions & 19 deletions crates/dojo/bindgen/src/plugins/typescript/generator/function.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,9 +29,9 @@

fn generate_system_function(&self, contract_name: &str, token: &Function) -> String {
format!(
"\tconst {} = async ({}) => {{
"\tconst {contract_name}_{} = async ({}) => {{
\t\ttry {{
\t\t\treturn await provider.execute(\n
\t\t\treturn await provider.execute(
\t\t\t\taccount,
\t\t\t\t{{
\t\t\t\t\tcontractName: \"{contract_name}\",
Expand Down Expand Up @@ -89,22 +89,69 @@
}

fn append_function_body(&self, idx: usize, buffer: &mut Buffer, body: String) {
// check if function was already appended to body, if so, append after other functions
// check if functions was already appended to body, if so, append after other functions
let pos = if buffer.len() - idx > 2 { buffer.len() - 2 } else { idx };

buffer.insert(pos + 1, body);
}

fn setup_function_wrapper_end(&self, token: &Function, buffer: &mut Buffer) {
fn setup_function_wrapper_end(
&self,
contract_name: &str,
token: &Function,
buffer: &mut Buffer,
) {
let return_token = "\treturn {";
if !buffer.has(return_token) {
buffer
.push(format!("\treturn {{\n\t\t{},\n\t}};\n}}", token.name.to_case(Case::Camel)));
buffer.push(format!(
"\treturn {{\n\t\t{}: {{\n\t\t\t{}: {}_{},\n\t\t}},\n\t}};\n}}",
contract_name,
token.name.to_case(Case::Camel),
contract_name,
token.name.to_case(Case::Camel)
));
return;
}

// if buffer has return and has contract_name, we append in this object if contract_name is
// the same

let contract_name_token = format!("\n\t\t{}: {{\n\t\t\t", contract_name);
if buffer.has(contract_name_token.as_str()) {
// we can expect safely as condition has is true there
let return_idx = buffer.pos(return_token).expect("return token not found");
// find closing curly bracket to get closing of object `contract_name`, get the last
// comma and insert token just after it.
if let Some(pos) = buffer.get_first_after(
format!("\n\t\t{}: {{\n\t\t\t", contract_name).as_str(),
"}",
return_idx,
) {
if let Some(insert_pos) = buffer.get_first_before_pos(",", pos, return_idx) {
buffer.insert_at(
format!(
"\n\t\t\t{}: {}_{},",
token.name.to_case(Case::Camel),
contract_name,
token.name.to_case(Case::Camel)
),
insert_pos,
return_idx,
);
return;
}
}

Check warning on line 143 in crates/dojo/bindgen/src/plugins/typescript/generator/function.rs

View check run for this annotation

Codecov / codecov/patch

crates/dojo/bindgen/src/plugins/typescript/generator/function.rs#L142-L143 

Added lines #L142 - L143 were not covered by tests
}

// if buffer has return but not contract_name, we append in this object
buffer.insert_after(
format!("\n\t\t{},", token.name.to_case(Case::Camel)),
format!(
"\n\t\t{}: {{\n\t\t\t{}: {}_{},\n\t\t}},",
contract_name,
token.name.to_case(Case::Camel),
contract_name,
token.name.to_case(Case::Camel),
),
Comment on lines +98 to +154
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Complex return object manipulation needs defensive programming.

The setup_function_wrapper_end method handles complex string manipulation for the return object structure. While the logic is sound, it could benefit from additional error handling.

Consider adding these safety checks:

 fn setup_function_wrapper_end(
     &self,
     contract_name: &str,
     token: &Function,
     buffer: &mut Buffer,
 ) {
+    if contract_name.is_empty() {
+        log::warn!("Empty contract name provided");
+        return;
+    }
     let return_token = "\treturn {";
     // ... rest of the implementation

Committable suggestion skipped: line range outside the PR's diff.

return_token,
",",
1,
Expand All @@ -120,13 +167,14 @@
buffer: &mut Buffer,
) -> BindgenResult<String> {
self.check_imports(buffer);
let contract_name = naming::get_name_from_tag(&contract.tag);
let idx = self.setup_function_wrapper_start(buffer);
self.append_function_body(
idx,
buffer,
self.generate_system_function(naming::get_name_from_tag(&contract.tag).as_str(), token),
self.generate_system_function(contract_name.as_str(), token),
);
self.setup_function_wrapper_end(token, buffer);
self.setup_function_wrapper_end(contract_name.as_str(), token, buffer);
Ok(String::new())
}
}
Expand Down Expand Up @@ -167,9 +215,9 @@
fn test_generate_system_function() {
let generator = TsFunctionGenerator {};
let function = create_change_theme_function();
let expected = "\tconst changeTheme = async (account: Account, value: number) => {
let expected = "\tconst actions_changeTheme = async (account: Account, value: number) => {
\t\ttry {
\t\t\treturn await provider.execute(\n
\t\t\treturn await provider.execute(
\t\t\t\taccount,
\t\t\t\t{
\t\t\t\t\tcontractName: \"actions\",
Expand All @@ -180,7 +228,8 @@
\t\t} catch (error) {
\t\t\tconsole.error(error);
\t\t}
\t};\n";
\t};
";

let contract = create_dojo_contract();
assert_eq!(
Expand Down Expand Up @@ -233,24 +282,46 @@
let generator = TsFunctionGenerator {};
let mut buff = Buffer::new();

generator.setup_function_wrapper_end(&create_change_theme_function(), &mut buff);
generator.setup_function_wrapper_end("actions", &create_change_theme_function(), &mut buff);

let expected = "\treturn {
\t\tchangeTheme,
\t\tactions: {
\t\t\tchangeTheme: actions_changeTheme,
\t\t},
\t};
}";

assert_eq!(1, buff.len());
assert_eq!(expected, buff[0]);

generator.setup_function_wrapper_end(&create_increate_global_counter_function(), &mut buff);
generator.setup_function_wrapper_end(
"actions",
&create_increate_global_counter_function(),
&mut buff,
);
let expected_2 = "\treturn {
\t\tchangeTheme,
\t\tincreaseGlobalCounter,
\t\tactions: {
\t\t\tchangeTheme: actions_changeTheme,
\t\t\tincreaseGlobalCounter: actions_increaseGlobalCounter,
\t\t},
\t};
}";
assert_eq!(1, buff.len());
assert_eq!(expected_2, buff[0]);

generator.setup_function_wrapper_end("dojo_starter", &create_move_function(), &mut buff);
let expected_3 = "\treturn {
\t\tactions: {
\t\t\tchangeTheme: actions_changeTheme,
\t\t\tincreaseGlobalCounter: actions_increaseGlobalCounter,
\t\t},
\t\tdojo_starter: {
\t\t\tmove: dojo_starter_move,
\t\t},
\t};
}";
assert_eq!(1, buff.len());
assert_eq!(expected_3, buff[0]);
}

#[test]
Expand All @@ -259,10 +330,12 @@
let mut buffer = Buffer::new();
let change_theme = create_change_theme_function();

let _ = generator.generate(&create_dojo_contract(), &change_theme, &mut buffer);
let onchain_dash_contract = create_dojo_contract();
let _ = generator.generate(&onchain_dash_contract, &change_theme, &mut buffer);

assert_eq!(buffer.len(), 6);
let increase_global_counter = create_increate_global_counter_function();
let _ = generator.generate(&create_dojo_contract(), &increase_global_counter, &mut buffer);
let _ = generator.generate(&onchain_dash_contract, &increase_global_counter, &mut buffer);
assert_eq!(buffer.len(), 7);
}

Expand All @@ -279,6 +352,9 @@
fn create_increate_global_counter_function() -> Function {
create_test_function("increase_global_counter", vec![])
}
fn create_move_function() -> Function {
create_test_function("move", vec![])
}
Comment on lines +356 to +358
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Ohayo! Consider enhancing the move function test helper.

The move function test helper could be more realistic by including typical movement parameters.

Consider this enhancement:

 fn create_move_function() -> Function {
-    create_test_function("move", vec![])
+    create_test_function("move", vec![
+        ("x".to_owned(), Token::CoreBasic(CoreBasic { type_path: "core::integer::u32".to_owned() })),
+        ("y".to_owned(), Token::CoreBasic(CoreBasic { type_path: "core::integer::u32".to_owned() })),
+    ])
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
fn create_move_function() -> Function {
create_test_function("move", vec![])
}
fn create_move_function() -> Function {
create_test_function("move", vec![
("x".to_owned(), Token::CoreBasic(CoreBasic { type_path: "core::integer::u32".to_owned() })),
("y".to_owned(), Token::CoreBasic(CoreBasic { type_path: "core::integer::u32".to_owned() })),
])
}


fn create_test_function(name: &str, inputs: Vec<(String, Token)>) -> Function {
Function {
Expand Down
3 changes: 3 additions & 0 deletions crates/dojo/bindgen/src/plugins/typescript/writer.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,9 @@
"dojo_init",
"namespace_hash",
"world",
"dojo_name",
"upgrade",
"world_dispatcher",

Check warning on line 112 in crates/dojo/bindgen/src/plugins/typescript/writer.rs

View check run for this annotation

Codecov / codecov/patch

crates/dojo/bindgen/src/plugins/typescript/writer.rs#L110-L112 

Added lines #L110 - L112 were not covered by tests
]
.contains(&name)
})
Expand Down
Loading