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.

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

fix: contracts function name clashing #2603

Merged
merged 2 commits into from
Nov 1, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
89 changes: 88 additions & 1 deletion crates/dojo/bindgen/src/plugins/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -48,17 +48,79 @@
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);
}
}

/// 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);
}
}

/// 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 97 in crates/dojo/bindgen/src/plugins/mod.rs

View check run for this annotation

Codecov / codecov/patch

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

Added line #L97 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 104 in crates/dojo/bindgen/src/plugins/mod.rs

View check run for this annotation

Codecov / codecov/patch

crates/dojo/bindgen/src/plugins/mod.rs#L103-L104

Added lines #L103 - L104 were not covered by tests
}

/// 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 117 in crates/dojo/bindgen/src/plugins/mod.rs

View check run for this annotation

Codecov / codecov/patch

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

Added line #L117 was not covered by tests
}

return indices.iter().filter(|&&i| i < pos).max().copied();
}
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#L121-L122

Added lines #L121 - L122 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 +174,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));
}
}
115 changes: 96 additions & 19 deletions crates/dojo/bindgen/src/plugins/typescript/generator/function.rs
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,10 @@
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 +229,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 +283,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 +331,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 +353,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
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