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: bindgen nested types init generation #2580

Merged
merged 4 commits into from
Oct 29, 2024

Conversation

MartianGreed
Copy link
Contributor

@MartianGreed MartianGreed commented Oct 25, 2024

Description

This requires cartridge-gg/cainome#67 to be merged first and version updated in Cargo.toml

Summary by CodeRabbit

Release Notes

  • New Features

    • Introduced a Buffer struct for improved string management, including methods for adding and manipulating strings.
    • Enhanced TypeScript schema generation with support for nested structures.
  • Improvements

    • Corrected TypeScript property casing for better adherence to conventions.
    • Added derive attributes to contract types for improved serialization and equality checks.
    • Updated formatting in TypeScript function generation for better code clarity.
    • Refined contract binding generation and build system configuration for clarity and specificity.
  • Tests

    • Added a new test for handling nested structures in TypeScript schema generation.

Copy link

coderabbitai bot commented Oct 25, 2024

Walkthrough

Ohayo, sensei! This pull request introduces a new Buffer struct in crates/dojo-bindgen/src/plugins/mod.rs, which encapsulates a Vec<String> and provides several methods for managing strings. Additionally, it modifies TypeScript type definitions in erc.rs, updates import management in function.rs, enhances schema generation in schema.rs, and adds derive attributes for contract types in model.rs and world.rs. The overall structure of the files remains intact, with a focus on improving string handling and TypeScript compliance.

Changes

File Path Change Summary
crates/dojo-bindgen/src/plugins/mod.rs Added Buffer struct with methods: new, has, push, insert_after, and join.
crates/dojo-bindgen/src/plugins/typescript/generator/erc.rs Corrected casing of fieldOrder in ERC__Balance, ERC__Token, and ERC__Transfer interfaces.
crates/dojo-bindgen/src/plugins/typescript/generator/function.rs Adjusted formatting in setup_function_wrapper_start and updated check_imports for import management.
crates/dojo-bindgen/src/plugins/typescript/generator/mod.rs Updated JsDefaultValue to include fieldOrder in struct type formatting.
crates/dojo-bindgen/src/plugins/typescript/generator/schema.rs Added create_test_nested_struct_token method and test_handle_nested_struct function.
crates/dojo-world/src/contracts/abi/model.rs Added derive attribute: contract_derives(Debug) in the abigen! macro.
crates/dojo-world/src/contracts/abi/world.rs Added derive attribute: contract_derives(Debug) in the abigen! macro.
Cargo.toml Updated cainome dependency version from v0.4.1 to v0.4.6 and reordered tonic features.
crates/dojo-world/abigen/src/main.rs Updated get_bindings_file_content and compile_dojo_core functions with modified internal logic.
examples/spawn-and-move/manifests/dev/deployment/manifest.json Updated transaction_hash field to a new value.
examples/spawn-and-move/manifests/dev/deployment/manifest.toml Updated transaction_hash in [world] section to a new value.

Possibly related PRs

  • feat: update bindgen for dojo.js sdk #2501: The changes in this PR involve the introduction of a new struct Buffer in crates/dojo-bindgen/src/plugins/mod.rs, which is relevant to the updates made in the dojo-bindgen library for modularity and extensibility, particularly in the context of TypeScript support and code generation.
  • feat(katana): commitment fields in block header #2560: The main PR adds commitment fields to the block header, which is relevant to the changes in the block.rs file where the PartialHeader struct is updated to include new fields for tracking various commitments and counts, enhancing the block structure.
  • fix: use name for contract interface to be consistent #2482: The main PR's focus on consistency in naming conventions for contract interfaces aligns with the changes made in the contract.rs file, where the contract_name method is renamed to name, ensuring uniformity across the codebase.
  • chore: implement From<BigUint> for address type #2488: The main PR's introduction of the From<BigUint> trait for ContractAddress relates to the changes in contract.rs, where the handling of ContractAddress is streamlined, enhancing usability and clarity in the code.
  • katana: update default predeployed accounts #2524: The main PR updates default predeployed accounts, which connects to the changes in the tests and deployment files where account addresses are updated, ensuring consistency with the new implementations.
  • chore: remove unused deps #2489: The main PR's removal of unused dependencies may relate to the overall cleanup and organization of the codebase, which is a common practice when introducing new features or refactoring existing code.

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 5

🧹 Outside diff range and nitpick comments (3)
crates/dojo-bindgen/src/plugins/mod.rs (2)

Line range hint 36-64: **Ohayo sensei! The Buffer implementation needs some love ~**

The implementation has several areas that could be improved:

  1. The insert_after() method uses unwrap() which could panic if the position isn't found
  2. Public API lacks documentation
  3. Complex index manipulation in insert_after() needs validation

Here's a suggested improvement:

 #[derive(Debug)]
+/// Buffer wraps a Vec<String> to provide specialized string manipulation operations
 pub struct Buffer(Vec<String>);
 impl Buffer {
+    /// Creates a new empty Buffer
     pub fn new() -> Self {
         Self(Vec::new())
     }
 
+    /// Checks if any string in the buffer contains the given substring
     pub fn has(&self, s: &str) -> bool {
         self.0.iter().any(|b| b.contains(s))
     }
 
+    /// Adds a new string to the buffer
     pub fn push(&mut self, s: String) {
         self.0.push(s.clone());
     }
 
+    /// Inserts a string after a specific position marked by a separator
+    /// Returns Result to handle cases where position or index is invalid
-    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();
+    pub fn insert_after(&mut self, s: String, pos: &str, sep: &str, idx: usize) -> Result<(), String> {
+        let pos = self.0.iter().position(|b| b.contains(pos))
+            .ok_or_else(|| format!("Position '{}' not found in buffer", pos))?;
         if let Some(st) = self.0.get_mut(pos) {
             let indices = st.match_indices(sep).map(|(i, _)| i).collect::<Vec<usize>>();
+            if indices.len() <= idx {
+                return Err(format!("Separator index {} out of bounds", idx));
+            }
             let append_after = indices[indices.len() - idx] + 1;
             st.insert_str(append_after, &s);
+            Ok(())
+        } else {
+            Err("Failed to get mutable reference".to_string())
         }
     }
 
+    /// Joins all strings in the buffer with the given separator
     pub fn join(&mut self, sep: &str) -> String {
         self.0.join(sep)
     }
 }

Dependency on cainome PR #67 needs attention

Ohayo sensei! The verification shows that cainome PR #67 is still open and not merged, while this implementation is using cainome v0.4.1. This creates a dependency risk since the required changes are not yet available in the current version.

  • PR feat: deployment config #67 "feat: copy inner composite for struct array keys" is still in OPEN state
  • Current implementation uses cainome v0.4.1 which doesn't include these changes
🔗 Analysis chain

Line range hint 1-4: Verify dependency on cainome PR #67

This implementation depends on cainome PR #67. Let's verify the current cainome version:


Let me check if PR #67 is already merged and included in the current version being used. This will help us verify if the dependency is properly handled.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check current cainome version in Cargo.toml
rg -A 1 'cainome.*=' Cargo.toml

Length of output: 183


Script:

#!/bin/bash
# Check if PR #67 exists and its merge status in cainome repository
gh pr view 67 --repo cartridge-gg/cainome --json number,title,state,mergedAt

# Get the commit history of cainome v0.4.1 tag
gh api repos/cartridge-gg/cainome/git/refs/tags/v0.4.1

Length of output: 575

crates/dojo-bindgen/src/plugins/typescript/generator/schema.rs (1)

247-299: Consider adding documentation for the test data structure, sensei!

The nested structure being tested represents a complex scenario with arrays and enums. Adding documentation would help other developers understand the test case's purpose and the expected TypeScript output.

Add a doc comment explaining the structure:

/// Creates a test token representing a nested structure with:
/// - field1: Array of Direction enum (None, North, South, West, East)
/// - field2: Nested Position struct
/// 
/// Expected TypeScript output:
/// ```typescript
/// interface TestNestedStruct {
///   field1: Direction[];
///   field2: Position;
/// }
/// ```
fn create_test_nested_struct_token(name: &str) -> Composite {
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 17f2564 and 7f7d486.

📒 Files selected for processing (7)
  • crates/dojo-bindgen/src/plugins/mod.rs (1 hunks)
  • crates/dojo-bindgen/src/plugins/typescript/generator/erc.rs (1 hunks)
  • crates/dojo-bindgen/src/plugins/typescript/generator/function.rs (1 hunks)
  • crates/dojo-bindgen/src/plugins/typescript/generator/mod.rs (1 hunks)
  • crates/dojo-bindgen/src/plugins/typescript/generator/schema.rs (2 hunks)
  • crates/dojo-world/src/contracts/abi/model.rs (1 hunks)
  • crates/dojo-world/src/contracts/abi/world.rs (1 hunks)
✅ Files skipped from review due to trivial changes (1)
  • crates/dojo-bindgen/src/plugins/typescript/generator/function.rs
🔇 Additional comments (7)
crates/dojo-bindgen/src/plugins/mod.rs (1)

Line range hint 66-75: Deref implementations look solid, sensei!

The implementation follows Rust's best practices for the newtype pattern, providing transparent access to the underlying Vec when needed.

crates/dojo-bindgen/src/plugins/typescript/generator/erc.rs (1)

Line range hint 41-47: Verify field ordering with schema requirements.

The fieldOrder array for tokenMetadata matches the defined fields in the correct order. However, since this PR depends on cainome#67, we should verify that this ordering aligns with any schema changes in that dependency.

✅ Verification successful

Let me search for more specific schema definitions related to ERC tokens.

These commands will help us find relevant token metadata schemas across the codebase to verify the field ordering.


Ohayo sensei! Based on the search results, I can now provide a definitive response about the field ordering.

Field ordering in ERC__Token matches the schema definition

Looking at the codebase, specifically in crates/torii/graphql/src/mapping.rs and crates/dojo-bindgen/src/plugins/typescript/generator/erc.rs, the field ordering ['name', 'symbol', 'tokenId', 'decimals', 'contractAddress'] is consistently defined and matches across all token metadata implementations. This ordering is used in:

  • ERC__Token interface definition
  • Token metadata objects in ERC20 and ERC721 implementations
  • GraphQL type mappings

The field order is correctly maintained and aligns with the schema requirements throughout the codebase.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for related schema definitions in the codebase
rg --type rust "struct.*Token.*\{" -A 6

Length of output: 990


Script:

#!/bin/bash
# Search for ERC token related schema definitions
rg -g '*.rs' -i "erc.*token|token.*metadata" -B 2 -A 6

Length of output: 35266


Script:

#!/bin/bash
# Look for any token interface or type definitions in typescript files
fd -e ts -e tsx -x rg -l "interface.*Token|type.*Token|TokenMetadata"

Length of output: 71


Script:

#!/bin/bash
# Check for any schema files that might contain token definitions
fd "schema" -e json -e yaml -e toml -x cat {}

Length of output: 47

crates/dojo-world/src/contracts/abi/model.rs (2)

328-329: The derive attributes look good, sensei! 🎉

The addition of both derives and contract_derives with comprehensive trait implementations (Debug, Serialize, Deserialize, PartialEq, Eq, Clone) will enhance the generated types with:

  • Debugging capabilities
  • Serialization/deserialization support
  • Equality comparison
  • Cloning functionality

Let's verify the dependency on cainome:

#!/bin/bash
# Check Cargo.toml for cainome dependency
rg "cainome.*version" Cargo.toml

# Verify if these derives are supported in cainome
rg -A 5 "contract_derives" $(fd -t f -e rs . | grep -i "cainome")

Line range hint 1-2: Ohayo sensei! ⚠️ This is an auto-generated file.

The comment at the top clearly states this is an auto-generated file. Any changes should be made in the generator (cargo run --bin dojo-world-abigen) rather than directly modifying this file.

Let's verify the generator configuration:

✅ Verification successful

Let me search for the generator configuration and its source code to verify if the changes are properly made in the generator rather than directly in the generated file.


Let me search for more specific configuration details about the abigen tool and its location in the codebase.


Now let me examine the actual generator code to verify if the derive traits are properly configured in the source rather than the generated file.


Ohayo sensei! ✨ The auto-generated file is properly managed by the generator

I can see that the derive traits are correctly configured in the generator source code (crates/dojo-world/abigen/src/main.rs). The get_bindings_file_content function includes these derives in the abigen! macro template:

derives(Debug, serde::Serialize, serde::Deserialize, PartialEq, Eq, Clone)

Any changes to the generated file should be made through the generator by running:

cargo run --bin dojo-world-abigen
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for the generator configuration
rg -l "dojo-world-abigen"

# Check the generator's source code
ast-grep --pattern 'fn main() {
  $$$
}'

Length of output: 456


Script:

#!/bin/bash
# Let's find the abigen binary source
fd -t f "dojo-world-abigen" -x cat {}

# Also check for any configuration files related to abigen
rg -l "abigen" Cargo.toml

Length of output: 76


Script:

#!/bin/bash
# Let's check the Cargo.toml for abigen configuration
cat Cargo.toml | grep -A 5 "abigen"

# Search for any build scripts or configuration files
fd -t f "build.rs|\.toml$" -x grep -l "abigen" {}

# Look for the binary in the crates directory
fd -t f -e rs . crates/ | grep -i "abigen"

Length of output: 589


Script:

#!/bin/bash
# Let's check the abigen main source code
cat crates/dojo-world/abigen/src/main.rs

# Also check the abigen Cargo.toml for dependencies
cat crates/dojo-world/abigen/Cargo.toml

Length of output: 5606

crates/dojo-bindgen/src/plugins/typescript/generator/mod.rs (1)

127-134: Verify compatibility with cainome dependency.

Since this PR depends on changes in the cainome repository (PR #67), we should ensure the new format is compatible with those changes.

Let's verify the cainome dependency version:

✅ Verification successful

Let me gather more information about the format compatibility.

This will help us verify if the format is compatible with the cainome dependency by:

  1. Checking the format implementation in cainome repository
  2. Looking for any related tests in our codebase that might validate this format

Let me check one more thing to verify the cainome version compatibility.

This will help us understand if there were any recent changes to the fieldOrder format and if we have test coverage for it.


Ohayo sensei! Based on the git history and codebase analysis, I can now provide a conclusive response:

The fieldOrder format is consistent with recent changes ✨

The format change from { field1: value1 } to { fieldOrder: ['field1'], field1: value1 } was intentionally implemented across the codebase, as evidenced by:

  • Recent git commits showing the standardization of fieldOrder casing
  • Comprehensive test coverage validating the format
  • Consistent implementation in ERC types and other interfaces
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check the cainome dependency version in Cargo.toml
rg -A 1 'cainome.*=' Cargo.toml

Length of output: 183


Script:

#!/bin/bash
# Check the format usage in cainome repository
gh api repos/cartridge-gg/cainome/contents/src/abigen/typescript.rs -H "Accept: application/vnd.github.raw" | rg -A 5 'fieldOrder'

# Check if there are any related tests in our repository
rg -g '*.rs' 'fieldOrder' .

Length of output: 2166


Script:

#!/bin/bash
# Check the git history for any recent changes to fieldOrder format
git log -p --since="3 months ago" | rg -B 3 -A 3 'fieldOrder'

# Also check the tests directory for any format-related tests
fd -t f -e rs . tests | xargs rg 'fieldOrder'

Length of output: 3796

crates/dojo-world/src/contracts/abi/world.rs (2)

1240-1241: LGTM! The derive attributes enhance type functionality.

The additions look good:

  1. Type alias for Config::Event improves code readability
  2. Contract derives ensure generated types implement essential traits for serialization, comparison, and cloning

Line range hint 1-1: Ohayo sensei! Be cautious about modifying auto-generated files.

This file is marked as auto-generated with a clear instruction to use cargo run --bin dojo-world-abigen for regenerating bindings.

Let me verify if the abigen binary exists:

✅ Verification successful

Ohayo sensei! The auto-generated file warning is accurate and helpful!

I've confirmed that the dojo-world-abigen binary exists as a proper Rust project in crates/dojo-world/abigen/ with its own Cargo.toml and main.rs. This validates that the warning about using cargo run --bin dojo-world-abigen to regenerate the bindings is correct and actionable.

  • The binary is properly set up in crates/dojo-world/abigen/
  • It has the necessary dependencies for ABI generation (cairo-lang-starknet, scarb)
  • The warning helps prevent manual modifications that would be lost on next generation
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check if the abigen binary exists in the workspace
fd -t f "dojo-world-abigen" -x echo "Found abigen binary at: {}"

Length of output: 64


Script:

#!/bin/bash
# Let's check for the binary in Cargo.toml and any build scripts
rg -l "dojo-world-abigen" --type toml

# Also check for any build scripts or source files related to abigen
fd -t f "build.rs|abigen" -x echo "Found potential build-related file: {}"

# Look for any main.rs files in bin directory that might contain the abigen binary
fd -t f "main.rs" -p "bin" -x echo "Found binary entry point: {}"

Length of output: 811


Script:

#!/bin/bash
# Let's check the contents of the abigen directory since we found it in Cargo.toml
fd . "crates/dojo-world/abigen" -x echo "Found in abigen dir: {}"

# Check for any source files in the abigen directory
rg -l "fn main" "crates/dojo-world/abigen"

# Look at the Cargo.toml content to understand the binary configuration
cat "crates/dojo-world/abigen/Cargo.toml"

Length of output: 759

@@ -34,11 +34,11 @@ const ERC_TORII_TYPES: &str = "\n\t\tERC__Balance: ERC__Balance,\n\t\tERC__Token
ERC__Token,\n\t\tERC__Transfer: ERC__Transfer,";
const ERC_TORII_INIT: &str = "
\t\tERC__Balance: {
\t\t\tfieldorder: ['balance', 'type', 'tokenmetadata'],
\t\t\tfieldOrder: ['balance', 'type', 'tokenmetadata'],
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! Fix inconsistent casing in fieldOrder array.

The field 'tokenmetadata' in the fieldOrder array uses lowercase, while the actual field is defined as 'tokenMetadata'. This inconsistency could cause field matching issues.

Apply this fix:

-       fieldOrder: ['balance', 'type', 'tokenmetadata'],
+       fieldOrder: ['balance', 'type', 'tokenMetadata'],
📝 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
\t\t\tfieldOrder: ['balance', 'type', 'tokenmetadata'],
fieldOrder: ['balance', 'type', 'tokenMetadata'],

Comment on lines +127 to +134
"{{ fieldOrder: [{}], {} }}",
value.inners.iter().map(|i| format!("'{}'", i.name)).collect::<Vec<_>>().join(", "),
value
.inners
.iter()
.map(|i| format!("{}: {},", i.name, JsDefaultValue::from(&i.token)))
.collect::<Vec<_>>()
.join("\n")
.join(" ")
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! Test case needs updating to match new format.

The implementation now uses space-separated fields, but the test case test_generate_type_init still expects newline-separated fields. This inconsistency should be addressed.

Update the test case to match the new format:

     let expected = "{
\t\t\tfieldOrder: ['field1', 'field2', 'field3'],
-\t\t\tfield1: 0,
-\t\t\tfield2: 0,
-\t\t\tfield3: 0,
+\t\t\tfield1: 0, field2: 0, field3: 0,
\t\t}";

Committable suggestion was skipped due to low confidence.


🛠️ Refactor suggestion

Consider maintaining newline separation for better readability.

The change to space-separated fields makes the output more compact but potentially harder to read, especially for structs with many fields or complex nested types.

Consider keeping the newline separation for better readability:

-                    .join(" ")
+                    .join("\n\t\t\t")
📝 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
"{{ fieldOrder: [{}], {} }}",
value.inners.iter().map(|i| format!("'{}'", i.name)).collect::<Vec<_>>().join(", "),
value
.inners
.iter()
.map(|i| format!("{}: {},", i.name, JsDefaultValue::from(&i.token)))
.collect::<Vec<_>>()
.join("\n")
.join(" ")
"{{ fieldOrder: [{}], {} }}",
value.inners.iter().map(|i| format!("'{}'", i.name)).collect::<Vec<_>>().join(", "),
value
.inners
.iter()
.map(|i| format!("{}: {},", i.name, JsDefaultValue::from(&i.token)))
.collect::<Vec<_>>()
.join("\n\t\t\t")

Comment on lines +200 to +207
#[test]
fn test_handle_nested_struct() {
let generator = TsSchemaGenerator {};
let mut buffer = Buffer::new();
let nested_struct = create_test_nested_struct_token("TestNestedStruct");
let _res = generator.generate(&nested_struct, &mut buffer);
assert_eq!(buffer.len(), 3);
}
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! The test assertions need improvement.

The test only verifies the buffer length without checking the actual content. Consider adding assertions to verify:

  • The correct TypeScript type definitions are generated
  • The nested structure is properly handled
  • The array type is correctly transformed

Here's a suggested improvement:

 #[test]
 fn test_handle_nested_struct() {
     let generator = TsSchemaGenerator {};
     let mut buffer = Buffer::new();
     let nested_struct = create_test_nested_struct_token("TestNestedStruct");
     let _res = generator.generate(&nested_struct, &mut buffer);
-    assert_eq!(buffer.len(), 3);
+    // Verify import
+    assert!(buffer[0].contains("import type { SchemaType }"));
+    
+    // Verify type definition
+    assert!(buffer[1].contains("TestNestedStruct: TestNestedStruct"));
+    
+    // Verify schema initialization
+    assert!(buffer[2].contains("fieldOrder: ['field1', 'field2']"));
+    assert!(buffer[2].contains("field1: []"));  // Array initialization
+    assert!(buffer[2].contains("field2: {"));   // Nested struct initialization
 }

Committable suggestion was skipped due to low confidence.

Comment on lines +239 to +313
pub fn create_test_nested_struct_token(name: &str) -> Composite {
Composite {
type_path: format!("onchain_dash::{name}"),
inners: vec![
CompositeInner {
index: 0,
name: "field1".to_owned(),
kind: CompositeInnerKind::Key,
token: Token::Array(cainome::parser::tokens::Array {
type_path: "core::array::Array::<onchain_dah::Direction>".to_owned(),
inner: Box::new(Token::Composite(Composite {
type_path: "onchain_dah::Direction".to_owned(),
inners: vec![
CompositeInner {
index: 0,
name: "None".to_owned(),
kind: CompositeInnerKind::Key,
token: Token::CoreBasic(CoreBasic {
type_path: "core::fetl252".to_owned(),
}),
},
CompositeInner {
index: 1,
name: "North".to_owned(),
kind: CompositeInnerKind::Key,
token: Token::CoreBasic(CoreBasic {
type_path: "core::fetl252".to_owned(),
}),
},
CompositeInner {
index: 2,
name: "South".to_owned(),
kind: CompositeInnerKind::Key,
token: Token::CoreBasic(CoreBasic {
type_path: "core::fetl252".to_owned(),
}),
},
CompositeInner {
index: 3,
name: "West".to_owned(),
kind: CompositeInnerKind::Key,
token: Token::CoreBasic(CoreBasic {
type_path: "core::fetl252".to_owned(),
}),
},
CompositeInner {
index: 4,
name: "East".to_owned(),
kind: CompositeInnerKind::Key,
token: Token::CoreBasic(CoreBasic {
type_path: "core::fetl252".to_owned(),
}),
},
],
generic_args: vec![],
r#type: CompositeType::Enum,
is_event: false,
alias: None,
})),
is_legacy: false,
}),
},
CompositeInner {
index: 1,
name: "field2".to_owned(),
kind: CompositeInnerKind::Key,
token: Token::Composite(create_test_struct_token("Position")),
},
],
generic_args: vec![],
r#type: CompositeType::Struct,
is_event: false,
alias: None,
}
}
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Multiple issues found in the test helper function, sensei!

  1. The function is marked as pub but is only used within the tests module
  2. There are typos in the type paths:
    • "fetl252" instead of "felt252"
    • "onchain_dah" instead of "onchain_dash"

Apply these fixes:

-    pub fn create_test_nested_struct_token(name: &str) -> Composite {
+    fn create_test_nested_struct_token(name: &str) -> Composite {
         Composite {
             type_path: format!("onchain_dash::{name}"),
             inners: vec![
                 CompositeInner {
                     index: 0,
                     name: "field1".to_owned(),
                     kind: CompositeInnerKind::Key,
                     token: Token::Array(cainome::parser::tokens::Array {
-                        type_path: "core::array::Array::<onchain_dah::Direction>".to_owned(),
+                        type_path: "core::array::Array::<onchain_dash::Direction>".to_owned(),
                         inner: Box::new(Token::Composite(Composite {
-                            type_path: "onchain_dah::Direction".to_owned(),
+                            type_path: "onchain_dash::Direction".to_owned(),
                             // ... other fields ...
                             token: Token::CoreBasic(CoreBasic {
-                                type_path: "core::fetl252".to_owned(),
+                                type_path: "core::felt252".to_owned(),
                             }),

Committable suggestion was skipped due to low confidence.

Copy link

codecov bot commented Oct 25, 2024

Codecov Report

Attention: Patch coverage is 98.87640% with 1 line in your changes missing coverage. Please review.

Project coverage is 70.32%. Comparing base (6f845a7) to head (ee67d00).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
crates/dojo-world/abigen/src/main.rs 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2580      +/-   ##
==========================================
+ Coverage   70.24%   70.32%   +0.08%     
==========================================
  Files         402      402              
  Lines       51131    51214      +83     
==========================================
+ Hits        35916    36016     +100     
+ Misses      15215    15198      -17     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Outside diff range and nitpick comments (1)
crates/dojo-world/abigen/src/main.rs (1)

88-88: Consider documenting the contract_derives behavior.

Since this is a new feature affecting code generation, it would be helpful to add a comment explaining the purpose and impact of contract_derives(Debug) for future maintainers.

         Clone),contract_derives(Debug)\n);",
+        // Note: contract_derives applies Debug trait to the generated contract types
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between b2b4b82 and ee67d00.

⛔ Files ignored due to path filters (3)
  • Cargo.lock is excluded by !**/*.lock
  • spawn-and-move-db.tar.gz is excluded by !**/*.gz
  • types-test-db.tar.gz is excluded by !**/*.gz
📒 Files selected for processing (6)
  • Cargo.toml (2 hunks)
  • crates/dojo-world/abigen/src/main.rs (1 hunks)
  • crates/dojo-world/src/contracts/abi/model.rs (1 hunks)
  • crates/dojo-world/src/contracts/abi/world.rs (1 hunks)
  • examples/spawn-and-move/manifests/dev/deployment/manifest.json (1 hunks)
  • examples/spawn-and-move/manifests/dev/deployment/manifest.toml (1 hunks)
✅ Files skipped from review due to trivial changes (2)
  • examples/spawn-and-move/manifests/dev/deployment/manifest.json
  • examples/spawn-and-move/manifests/dev/deployment/manifest.toml
🚧 Files skipped from review as they are similar to previous changes (3)
  • Cargo.toml
  • crates/dojo-world/src/contracts/abi/model.rs
  • crates/dojo-world/src/contracts/abi/world.rs
🔇 Additional comments (1)
crates/dojo-world/abigen/src/main.rs (1)

88-88: Ohayo! Verify the contract_derives attribute compatibility.

The addition of contract_derives(Debug) looks good, sensei! However, since this change depends on cartridge-gg/cainome#67, we should ensure compatibility.

✅ Verification successful

Ohayo! The contract_derives(Debug) addition is compatible, sensei!

The codebase is using cainome v0.4.6 which supports the contract_derives attribute, and this feature is already being used in other parts of the codebase (world.rs and model.rs). The change aligns with existing patterns and doesn't require any external PR dependencies.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify cainome version compatibility
# Test: Check if Cargo.toml references the correct cainome version that supports contract_derives
rg -A 5 "cainome.*version" Cargo.toml

Length of output: 39


Script:

#!/bin/bash
# Let's check all Cargo.toml files in the repository for cainome dependency
fd Cargo.toml --exec rg -l "cainome"

# Then let's examine the content of those files
fd Cargo.toml --exec sh -c 'echo "=== {} ==="; cat {}'

Length of output: 57285


Script:

#!/bin/bash
# Let's check the crates/dojo-world/abigen/Cargo.toml for cainome dependency
echo "=== Checking abigen's Cargo.toml ==="
fd "Cargo.toml" crates/dojo-world/abigen/ --exec cat {}

# Let's also check the actual implementation to understand the contract_derives usage
echo -e "\n=== Checking contract_derives implementation ==="
rg -A 5 "contract_derives" crates/dojo-world/abigen/src/

Length of output: 1087


Script:

#!/bin/bash
# Let's check the workspace dependencies for cainome version
echo "=== Checking workspace cainome dependency ==="
rg "cainome.*=.*" -A 2 Cargo.toml

# Let's also check if there are any other contract_derives usages
echo -e "\n=== Checking other contract_derives usages ==="
rg -l "contract_derives" .

Length of output: 556

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants