feat: add i128 + use BigNumberish type #2705
Conversation
WalkthroughOhayo, sensei! This pull request introduces updates to the TypeScript generator within the Dojo bindgen crate. Key changes enhance type handling for function parameters, transitioning from Changes
Possibly related PRs
Suggested reviewers
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? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
crates/dojo/bindgen/src/plugins/typescript/generator/interface.rs (1)
17-20: Nice buffer check implementation!The conditional check prevents duplicate imports, which is a good practice. However, consider extracting common import handling logic into a shared utility if similar patterns exist in other generators.
crates/dojo/bindgen/src/plugins/typescript/generator/mod.rs (1)
Line range hint
197-225: Test coverage looks solid, but could be enhanced!The test updates properly verify the BigNumberish changes, but we could make them even better.
Consider adding specific test cases for the new
i128type to ensure it behaves correctly in all scenarios:#[test] fn test_i128_type() { assert_eq!( "BigNumberish", JsType::from(&Token::CoreBasic(CoreBasic { type_path: "core::integer::i128".to_owned() })) ); }crates/dojo/bindgen/src/plugins/typescript/generator/function.rs (1)
272-272: Consider consolidating duplicate test casesOhayo sensei! I noticed that
test_format_function_inputsandtest_format_function_inputs_complexhave identical expectations and test logic. Consider consolidating these tests or differentiating their test cases to improve test coverage.- #[test] - fn test_format_function_inputs_complex() { - let generator = TsFunctionGenerator {}; - let function = create_change_theme_function(); - let expected = "snAccount: Account, value: BigNumberish"; - assert_eq!(expected, generator.format_function_inputs(&function)) - }Also applies to: 280-280
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
crates/dojo/bindgen/src/plugins/typescript/generator/function.rs(3 hunks)crates/dojo/bindgen/src/plugins/typescript/generator/interface.rs(2 hunks)crates/dojo/bindgen/src/plugins/typescript/generator/mod.rs(4 hunks)
🔇 Additional comments (7)
crates/dojo/bindgen/src/plugins/typescript/generator/interface.rs (3)
7-8: Ohayo! LGTM on the import constant!
The constant BIGNUMNERISH_IMPORT is well-defined and follows the pattern of importing types from starknet.
12-12: Method signature change looks good, sensei!
The change from &Buffer to &mut Buffer is necessary to support the new import functionality.
91-92: Test assertions updated correctly!
The test assertions have been properly updated to reflect the type change from number to BigNumberish, maintaining consistency with the PR objectives.
Please ensure that all other test files that might be asserting against this interface's output are also updated accordingly.
crates/dojo/bindgen/src/plugins/typescript/generator/mod.rs (2)
57-67: Ohayo sensei! Comprehensive type mapping update looks good!
The transition from number to BigNumberish for all numeric types is consistent with the PR objectives. The addition of i128 type maintains the pattern.
Consider if smaller integer types (u8, u16) really need BigNumberish, or if they could remain as regular numbers for better performance in cases where the values are guaranteed to be small.
116-116: Clean and consistent default value addition!
The default value for i128 follows the established pattern for numeric types.
crates/dojo/bindgen/src/plugins/typescript/generator/function.rs (2)
15-15: LGTM! Import statement updated correctly
Ohayo sensei! The addition of BigNumberish import alongside Account aligns perfectly with the PR's objective to enhance type support.
238-239: LGTM! Test expectations properly updated
The test expectations now correctly use BigNumberish instead of number for the value parameter, maintaining consistency with the new type system.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2705 +/- ##
=======================================
Coverage ? 56.22%
=======================================
Files ? 410
Lines ? 52898
Branches ? 0
=======================================
Hits ? 29742
Misses ? 23156
Partials ? 0 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
MartianGreed
force-pushed
the
fix/bindgen-bn
branch
from
6a9e5ca
to
be29262
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
crates/dojo/bindgen/src/plugins/typescript/generator/interface.rs (1)
7-7: Ohayo! Fix typo in constant name, sensei!The constant name contains a typo:
BIGNUMNERISHshould beBIGNUMBERISHto match the actual type nameBigNumberish.-const BIGNUMNERISH_IMPORT: &str = "import type { BigNumberish } from 'starknet';"; +const BIGNUMBERISH_IMPORT: &str = "import type { BigNumberish } from 'starknet';";crates/dojo/bindgen/src/plugins/typescript/generator/mod.rs (1)
Line range hint
1-324: Consider adding explicit tests for i128, sensei! 🎍While the implementation for
i128is correct, it would be beneficial to add specific test cases for this new type to ensure it behaves correctly in all scenarios (basic types, tuples, and arrays).Would you like me to help draft these additional test cases?
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
crates/dojo/bindgen/src/plugins/typescript/generator/function.rs(3 hunks)crates/dojo/bindgen/src/plugins/typescript/generator/interface.rs(2 hunks)crates/dojo/bindgen/src/plugins/typescript/generator/mod.rs(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/dojo/bindgen/src/plugins/typescript/generator/function.rs
🔇 Additional comments (5)
crates/dojo/bindgen/src/plugins/typescript/generator/interface.rs (2)
12-19: Ohayo! Clean implementation of import management, sensei!
The changes to handle BigNumberish imports are well-implemented:
- Proper buffer mutability for import management
- Clean duplicate prevention using
hascheck - Follows Rust best practices
91-92: Test expectations properly updated for BigNumberish type!
The test case correctly verifies the generation of TypeScript interfaces with BigNumberish types, providing good coverage of the new type system changes.
crates/dojo/bindgen/src/plugins/typescript/generator/mod.rs (3)
57-67: Ohayo! Consistent type mapping for numeric types looks good, sensei! 🎋
The transition from number to BigNumberish for all numeric types (felt252, u8, u16, u32, u64, u128, u256, U256, i128) aligns well with the PR objectives and ensures proper handling of large numbers from torii.
116-116: Default value for i128 is properly aligned with other numeric types
The addition of i128 with a default value of "0" maintains consistency with other numeric type defaults.
197-203: Test cases properly validate BigNumberish type generation, sensei! 🍜
The test cases have been appropriately updated to verify:
- Basic type conversion for
u8andfelt252toBigNumberish - Tuple type handling with
BigNumberishcomponents - Array type handling with nested
BigNumberishtypes
This comprehensive test coverage ensures the type generation remains reliable.
Also applies to: 211-225
There was a problem hiding this comment.
May be easier to change using some constants @MartianGreed. 👍
But not a blocker for me, happy to merge if more urgent stuff is on-going.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
crates/dojo/bindgen/src/plugins/typescript/generator/mod.rs (2)
2-9: Ohayo sensei! Nice architectural improvement with constants extraction!The introduction of a dedicated constants module and organized imports improves maintainability and follows Rust's module system best practices.
Line range hint
186-232: Consider adding more test cases for comprehensive coverageThe test updates look good, sensei! However, consider adding these test cases:
- Edge cases for BigNumberish (max/min values)
- Nested array structures with mixed types
- Complex tuple combinations
Example test case structure:
#[test] fn test_complex_nested_types() { // Test array of tuples containing BigNumberish and string assert_eq!( format!("Array<[{}, string]>", JS_BIGNUMBERISH).as_str(), JsType::from(&Token::Array(/* ... */)) ); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
crates/dojo/bindgen/src/plugins/typescript/generator/constants.rs(1 hunks)crates/dojo/bindgen/src/plugins/typescript/generator/mod.rs(6 hunks)
✅ Files skipped from review due to trivial changes (1)
- crates/dojo/bindgen/src/plugins/typescript/generator/constants.rs
🔇 Additional comments (2)
crates/dojo/bindgen/src/plugins/typescript/generator/mod.rs (2)
112-123: Default values look solid, sensei!
The default values are type-safe, consistent, and follow TypeScript/JavaScript conventions. The numeric types defaulting to "0" aligns well with the BigNumberish type system.
63-74: Verify type compatibility with torii component
The type mappings look good, sensei! The transition to BigNumberish for numeric types aligns perfectly with the PR objectives. However, let's verify the compatibility:
✅ Verification successful
Type mappings are correctly implemented and compatible with torii
Ohayo sensei! After reviewing the codebase, I can confirm that the type mappings are perfectly aligned and compatible:
- The example TypeScript file shows that numeric types are already using
BigNumberishin the generated code - The torii client integration in the codebase correctly handles these types through the
valueToToriiValueAndOperatorfunction - The interface generator properly imports and uses
BigNumberishtype from 'starknet' - The type mappings in the constants file are consistent with the PR changes
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify type usage consistency across the codebase
# Check for any remaining direct 'number' type usage that should be BigNumberish
rg -g '*.ts' 'type.*:.*\bnumber\b'
# Check torii component type definitions for compatibility
rg -g '*.ts' 'type.*BigNumberish'
Length of output: 72
Script:
#!/bin/bash
# Let's check the repository structure first to locate TypeScript files
fd -e ts
# Then let's search for BigNumberish type imports and usage
rg -l "BigNumberish"
# Check for number type declarations in TypeScript interfaces and types
rg --type typescript "interface.*\{[\s\S]*?number.*?\}" -A 5
# Look for type definitions specifically in torii related files
fd -e ts torii
Length of output: 400
Script:
#!/bin/bash
# Let's examine the content of the example TypeScript file
cat crates/dojo/bindgen/src/test_data/mocks/dojo_examples.ts
# Check the constants file where BigNumberish is used
cat crates/dojo/bindgen/src/plugins/typescript/generator/constants.rs
# Look for any type definitions in the function generator
cat crates/dojo/bindgen/src/plugins/typescript/generator/function.rs
# Check interface generator implementation
cat crates/dojo/bindgen/src/plugins/typescript/generator/interface.rs
Length of output: 27959
| @@ -12,7 +12,7 @@ impl TsFunctionGenerator { | |||
| fn check_imports(&self, buffer: &mut Buffer) { | |||
| if !buffer.has("import { DojoProvider } from ") { | |||
| buffer.insert(0, "import { DojoProvider } from \"@dojoengine/core\";".to_owned()); | |||
| buffer.insert(1, "import { Account } from \"starknet\";".to_owned()); | |||
| buffer.insert(1, "import { Account, BigNumberish } from \"starknet\";".to_owned()); | |||
There was a problem hiding this comment.
| buffer.insert(1, "import { Account, BigNumberish } from \"starknet\";".to_owned()); | |
| buffer.insert(1, "import { Account, JS_BIGNUMBERISH } from \"starknet\";".to_owned()); |
Use the newly defined constant.
MartianGreed
force-pushed
the
fix/bindgen-bn
branch
from
dd55a6f
to
33d0043
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
crates/dojo/bindgen/src/plugins/typescript/generator/mod.rs (2)
112-123: Consider documenting BigNumberish default valuesWhile the default values remain as "0" for all numeric types including the new i128, it would be helpful to document that these are now interpreted as BigNumberish values.
+ // Default values are represented as strings but should be interpreted as BigNumberish match value { CAIRO_FELT252 => JsDefaultValue("0".to_string()),
204-210: Consider adding BigNumberish edge case testsThe tests have been updated to use
JS_BIGNUMBERISH, but we could strengthen them by adding edge cases specific to BigNumberish values.Consider adding tests for:
- Maximum safe integer values
- Values requiring BigNumberish representation
- Negative values for i128
Also applies to: 218-218, 232-232
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
crates/dojo/bindgen/src/plugins/typescript/generator/constants.rs(1 hunks)crates/dojo/bindgen/src/plugins/typescript/generator/function.rs(4 hunks)crates/dojo/bindgen/src/plugins/typescript/generator/mod.rs(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/dojo/bindgen/src/plugins/typescript/generator/constants.rs
- crates/dojo/bindgen/src/plugins/typescript/generator/function.rs
🔇 Additional comments (2)
crates/dojo/bindgen/src/plugins/typescript/generator/mod.rs (2)
2-6: Ohayo sensei! Clean organization of constants!
The separation of constants into a dedicated module and the well-structured imports improve code maintainability.
Also applies to: 9-9
63-74: Verify callers are ready for BigNumberish transition
The transition from number to BigNumberish for all numeric types is consistent with the PR objectives. However, we should verify that all consumers are prepared for this change.
✅ Verification successful
Ohayo sensei! The BigNumberish transition is properly implemented
The code generator correctly handles the BigNumberish type transition:
- Automatically imports
BigNumberishfrom 'starknet' in generated files - Consistently maps all numeric Cairo types to
BigNumberish - Test files confirm the correct type mappings
- The generator's test suite validates the numeric type conversions
The mock files still using number are test fixtures and not actual generated code, so they don't affect the implementation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for direct number type usage in TypeScript files
rg -t ts ":\s*number"
# Search for potential BigNumberish imports
rg -t ts "BigNumberish"
Length of output: 515
Script:
#!/bin/bash
# Let's check the test files and generated code for BigNumberish usage
fd -e ts -x cat {} | grep -i "bignumberish"
# Let's check the actual TypeScript output directory for generated files
fd "\.ts$" crates/dojo/bindgen/src/test_data/output -x cat {} | grep -E ":\s*(number|BigNumberish)"
# Let's also check for any type definitions or interfaces that might show the type mappings
ast-grep --pattern 'interface $_ {
$$$
}'
Length of output: 3166
Script:
#!/bin/bash
# Let's check the TypeScript generator implementation for where these types are used
ast-grep --pattern 'function $name($$$) {
$$$
JS_BIGNUMBERISH
$$$
}'
# Let's also check for any test files that might show the generated output
fd -e rs "test" -x cat {} | grep -A 5 -B 5 "JS_BIGNUMBERISH"
# And check where the JS type constants are defined
rg "JS_BIGNUMBERISH|JS_NUMBER" -A 2 -B 2
Length of output: 6477
Description
numbertype toBigNumberishto allow types returned from torii to work.i128Checklist
scripts/prettier.sh,scripts/rust_fmt.sh,scripts/cairo_fmt.sh)scripts/clippy.sh,scripts/docs.sh)Summary by CodeRabbit
New Features
BigNumberishfor improved type safety.starknetlibrary.Bug Fixes