feat(core): improve provider and internalize calldata compiler #514
Conversation
📝 WalkthroughWalkthroughAdds ABI-based calldata compilation and ABI lookup utilities; DojoProvider compiles calldata before calls and disambiguates duplicate system names at runtime; new unit tests validate ABI resolution and provider behavior. Changes
Sequence DiagramsequenceDiagram
participant Client
participant Provider as DojoProvider
participant Compiler as compileDojoCalldata
participant Resolver as findFunctionAbiByNamespace
participant Contract as StarkNet_Contract
Client->>Provider: invoke action (namespace, contractName, systemName, args, account?)
Provider->>Provider: compute functionCounts → derive methodName
Provider->>Compiler: compileDojoCalldata(abi, namespace, contractName, systemName, args)
Compiler->>Resolver: findFunctionAbiByNamespace(abi, namespace, contractName, systemName)
Resolver-->>Compiler: FunctionAbi | undefined
alt FunctionAbi found
Compiler->>Compiler: normalize args, validate required inputs, parse fields
Compiler-->>Provider: compiled calldata (with __compiled__ flag)
Provider->>Contract: call(methodName, compiled calldata, account?)
Contract-->>Provider: result / error
Provider-->>Client: return result / propagate error
else Not found / validation error
Compiler-->>Provider: throw error
Provider-->>Client: propagate error
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/core/src/utils/index.ts (1)
37-54: Potential null pointer dereference if contract not found.
getContractByNameusesArray.find()which returnsundefinedif no match is found. If the contract doesn't exist in the manifest, accessingcontract.addressandcontract.abion lines 45-47 will throw a runtime error.🔎 Proposed fix with guard clause
export const parseDojoCall = ( manifest: any, nameSpace: string, call: DojoCall | Call ): Call => { if ("contractName" in call) { const contract = getContractByName( manifest, nameSpace, call.contractName ); + + if (!contract) { + throw new Error( + `Contract "${call.contractName}" not found in namespace "${nameSpace}"` + ); + } return { contractAddress: contract.address, calldata: compileDojoCalldata(
🧹 Nitpick comments (4)
packages/core/src/_test_/compile.test.ts (1)
7-76: Consider adding tests forcompileDojoCalldata.The test suite covers
findFunctionAbiByNamespacewell with multiple scenarios including the fallback behavior and nonexistent function case. However,compileDojoCalldatais also a new exported function with error-throwing paths (missing method, missing arguments) that would benefit from test coverage.🔎 Example test cases for compileDojoCalldata
describe("compileDojoCalldata", () => { it("should throw when method not found", () => { expect(() => compileDojoCalldata( abi, "pistols", "admin", "nonexistent_method", {} ) ).toThrow(/Method "nonexistent_method" not found/); }); it("should throw when required argument is missing", () => { expect(() => compileDojoCalldata( abi, "pistols", "admin", "set_paused", {} // missing "paused" argument ) ).toThrow(/Missing argument "paused"/); }); it("should compile calldata with valid arguments", () => { const result = compileDojoCalldata( abi, "pistols", "admin", "set_paused", { paused: true } ); expect(result).toBeDefined(); expect(Array.isArray(result)).toBe(true); }); });packages/core/src/provider/DojoProvider.ts (1)
358-400: Well-designed duplicate detection with a minor naming edge case.The logic for detecting duplicate system names and generating disambiguated method names is sound. However, using underscores as separators in
${names.namespace}_${names.contractName}_${systemName}could cause collisions if namespace or contractName contain underscores themselves.For example:
namespace="foo_bar",contractName="baz"→foo_bar_baz_systemNamenamespace="foo",contractName="bar_baz"→foo_bar_baz_systemNameThis is likely rare in practice, but consider using a more distinct separator (e.g.,
::or__) if Dojo naming conventions allow underscores.packages/core/src/utils/compile.ts (2)
22-82: Consider extracting the repeated function-finding logic.The pattern
fn?.type === "function" && fn?.name === functionNameis repeated 6 times. Extracting this to a helper would improve maintainability.🔎 Proposed helper extraction
+const isFunctionMatch = (fn: any, functionName: string): boolean => + fn?.type === "function" && fn?.name === functionName; + export function findFunctionAbiByNamespace( abi: Abi, namespace: string, contractName: string, functionName: string ): FunctionAbi | undefined { const contractPattern = `::${contractName}::`; const exactMatch = abi.find((item): item is InterfaceAbi => { if (item?.type !== "interface") return false; const name = item.name || ""; if (!name.startsWith(`${namespace}::`)) return false; if (!name.includes(contractPattern)) return false; return ( Array.isArray(item.items) && - item.items.some( - (fn: any) => - fn?.type === "function" && fn?.name === functionName - ) + item.items.some((fn: any) => isFunctionMatch(fn, functionName)) ); }); if (exactMatch) { return exactMatch.items.find( - (fn: any) => fn?.type === "function" && fn?.name === functionName + (fn: any) => isFunctionMatch(fn, functionName) ) as FunctionAbi | undefined; } // ... apply similarly to other occurrences
112-129: Array arguments bypass validation.When
argsCalldatais an array (line 113-114), the arguments are used directly without validation againstabiMethod.inputs. This could lead to runtime errors if the wrong number or types of arguments are provided, whereas the object format validates required fields.Consider adding array length validation for improved developer experience:
🔎 Proposed validation for array arguments
let args: any[]; if (Array.isArray(argsCalldata)) { + const expectedCount = abiMethod.inputs.filter( + (input) => !isLen(input.name) + ).length; + if (argsCalldata.length !== expectedCount) { + throw new Error( + `Expected ${expectedCount} arguments for method "${method}", got ${argsCalldata.length}` + ); + } args = argsCalldata; } else {
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/core/src/_test_/compile.test.ts(1 hunks)packages/core/src/provider/DojoProvider.ts(8 hunks)packages/core/src/utils/compile.ts(1 hunks)packages/core/src/utils/index.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx,js,jsx,json}
📄 CodeRabbit inference engine (AGENTS.md)
Apply code formatting with 4 spaces, 80 character line width, double quotes, trailing commas, and semicolons
Files:
packages/core/src/_test_/compile.test.tspackages/core/src/utils/compile.tspackages/core/src/utils/index.tspackages/core/src/provider/DojoProvider.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx,js,jsx}: Keep imports grouped: node/external, blank line, workspace, then relative
React hooks/components follow strict dependency arrays and early returns
Prefer guard clauses and rethrow or surface context in error handling; avoid silent catches
Files:
packages/core/src/_test_/compile.test.tspackages/core/src/utils/compile.tspackages/core/src/utils/index.tspackages/core/src/provider/DojoProvider.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Maintain explicit return types for exported APIs when inference is unclear
Avoidanytype; lean on generics, discriminated unions, and utility types
Files:
packages/core/src/_test_/compile.test.tspackages/core/src/utils/compile.tspackages/core/src/utils/index.tspackages/core/src/provider/DojoProvider.ts
**/*.test.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Tests should remain deterministic; clean up observables/subscriptions between test cases
Files:
packages/core/src/_test_/compile.test.ts
🧬 Code graph analysis (4)
packages/core/src/_test_/compile.test.ts (1)
packages/core/src/utils/compile.ts (1)
findFunctionAbiByNamespace(22-82)
packages/core/src/utils/compile.ts (1)
packages/create-burner/src/connectors/burner.ts (1)
name(92-94)
packages/core/src/utils/index.ts (2)
packages/core/src/utils/compile.ts (1)
compileDojoCalldata(84-157)packages/core/src/provider/DojoProvider.ts (1)
call(284-319)
packages/core/src/provider/DojoProvider.ts (1)
packages/core/src/utils/compile.ts (1)
compileDojoCalldata(84-157)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build-examples
- GitHub Check: build-packages
- GitHub Check: build
🔇 Additional comments (7)
packages/core/src/_test_/compile.test.ts (1)
1-6: LGTM! Well-structured test setup.The test imports and setup are clean. The
as anycast on line 5 is acceptable given the JSON manifest typing limitations.packages/core/src/provider/DojoProvider.ts (3)
300-307: LGTM! Proper calldata compilation before contract call.The integration of
compileDojoCalldatacorrectly passes all required parameters (ABI, namespace, contractName, entrypoint, calldata) and uses the compiled result for the contract call.
429-454: LGTM! View method implementation is consistent.The view method correctly uses
methodNamefor both the host property key and error messages, maintaining consistency with the disambiguation logic.
456-484: LGTM! External method implementation mirrors view method pattern.The external (non-view) method implementation follows the same pattern as view methods with proper account validation and argument handling.
packages/core/src/utils/compile.ts (3)
14-20: LGTM! Clean helper functions.
isLenandisCairo1Typeare concise, appropriately scoped helpers that encapsulate the naming conventions for Cairo length fields and type identifiers.
131-157: LGTM! Calldata array construction is well-implemented.The reduce-based construction correctly handles Cairo length-prefixed fields and uses starknet.js's
parseCalldataFieldfor proper serialization. The__compiled__marker is appropriately attached as a non-enumerable property to prevent serialization issues.
102-110: VerifyisNoConstructorValidhandles undefinedabiMethodcorrectly.
isNoConstructorValid(method, argsCalldata, abiMethod)is called on line 102 before the null check onabiMethodat line 106. Confirm that starknet'sisNoConstructorValidfunction safely handles undefined values, or reorder the checks.
MartianGreed
force-pushed
the
feat/compile-calldata
branch
from
d5999c7 to
08287c9
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
packages/core/src/utils/compile.ts (3)
38-39: Replaceanytype with proper type guard.The use of
anytype violates the coding guidelines. Consider using a proper type guard instead.🔎 Proposed fix
item.items.some( - (fn: any) => - fn?.type === "function" && fn?.name === functionName + (fn): fn is FunctionAbi => + fn?.type === "function" && + fn?.name === functionName )Apply similar changes to lines 46, 58, 65, and 72.
As per coding guidelines, avoid
anytype and lean on generics, discriminated unions, and utility types.
22-82: Document the fallback strategy for ABI resolution.The function implements a four-level fallback strategy (exact match → namespace match → interface scan → top-level function), but this precedence is not documented. Consider adding a JSDoc comment explaining when each strategy is used and the implications for namespace collisions.
132-148: Document the_leninput skipping logic.The logic on line 134 skips
_leninputs unless they are Cairo1 types. This behavior is not immediately obvious and could benefit from a brief comment explaining why this distinction is necessary.🔎 Proposed addition
const callArray: string[] = abiMethod.inputs.reduce( (acc: string[], input: AbiEntry) => { + // Skip _len inputs for Cairo 0 types; Cairo 1 handles length explicitly if (isLen(input.name) && !isCairo1Type(input.type)) { return acc; }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.changeset/yellow-garlics-judge.md(1 hunks)packages/core/src/_test_/compile.test.ts(1 hunks)packages/core/src/provider/DojoProvider.ts(8 hunks)packages/core/src/utils/compile.ts(1 hunks)packages/core/src/utils/index.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/core/src/provider/DojoProvider.ts
- packages/core/src/test/compile.test.ts
- packages/core/src/utils/index.ts
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx,js,jsx,json}
📄 CodeRabbit inference engine (AGENTS.md)
Apply code formatting with 4 spaces, 80 character line width, double quotes, trailing commas, and semicolons
Files:
packages/core/src/utils/compile.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx,js,jsx}: Keep imports grouped: node/external, blank line, workspace, then relative
React hooks/components follow strict dependency arrays and early returns
Prefer guard clauses and rethrow or surface context in error handling; avoid silent catches
Files:
packages/core/src/utils/compile.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Maintain explicit return types for exported APIs when inference is unclear
Avoidanytype; lean on generics, discriminated unions, and utility types
Files:
packages/core/src/utils/compile.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build-examples
- GitHub Check: build-packages
- GitHub Check: build
🔇 Additional comments (4)
.changeset/yellow-garlics-judge.md (1)
2-2: Verify changeset level matches feature scope.The changeset is marked as
patchlevel, but the description usesfeat(core):which typically indicates a new feature that would warrant aminorversion bump under semantic versioning. Please confirm whether this should be aminorchangeset instead.packages/core/src/utils/compile.ts (3)
1-12: LGTM!The imports are correctly grouped and include all necessary types and utilities from starknet for ABI-based calldata compilation.
14-20: LGTM!Both helper functions correctly identify their respective patterns:
_lensuffixes for length parameters and::for Cairo 1 types.
150-154: LGTM!The non-enumerable
__compiled__marker is a nice touch that prevents recompilation and keeps the property hidden from enumeration.
| let args: any[]; | ||
| if (Array.isArray(argsCalldata)) { | ||
| args = argsCalldata; | ||
| } else { | ||
| args = abiMethod.inputs | ||
| .filter((input) => !isLen(input.name)) | ||
| .map((input) => { | ||
| const value = (argsCalldata as Record<string, unknown>)[ | ||
| input.name | ||
| ]; | ||
| if (value === undefined) { | ||
| throw new Error( | ||
| `Missing argument "${input.name}" for method "${method}"` | ||
| ); | ||
| } | ||
| return value; | ||
| }); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add type validation for argsCalldata parameter.
The function assumes argsCalldata is either an array or an object, but there's no explicit validation. If it's neither (e.g., null, undefined, or a primitive), the code will fail silently or throw a cryptic error.
Additionally, args is typed as any[] which violates the coding guidelines.
🔎 Proposed fix
- let args: any[];
+ let args: unknown[];
if (Array.isArray(argsCalldata)) {
args = argsCalldata;
- } else {
+ } else if (
+ typeof argsCalldata === "object" &&
+ argsCalldata !== null
+ ) {
args = abiMethod.inputs
.filter((input) => !isLen(input.name))
.map((input) => {
const value = (argsCalldata as Record<string, unknown>)[
input.name
];
if (value === undefined) {
throw new Error(
`Missing argument "${input.name}" for method "${method}"`
);
}
return value;
});
+ } else {
+ throw new Error(
+ `Invalid argsCalldata type for method "${method}". Expected array or object.`
+ );
}As per coding guidelines, avoid any type and lean on generics, discriminated unions, and utility types.
📝 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.
| let args: any[]; | |
| if (Array.isArray(argsCalldata)) { | |
| args = argsCalldata; | |
| } else { | |
| args = abiMethod.inputs | |
| .filter((input) => !isLen(input.name)) | |
| .map((input) => { | |
| const value = (argsCalldata as Record<string, unknown>)[ | |
| input.name | |
| ]; | |
| if (value === undefined) { | |
| throw new Error( | |
| `Missing argument "${input.name}" for method "${method}"` | |
| ); | |
| } | |
| return value; | |
| }); | |
| } | |
| let args: unknown[]; | |
| if (Array.isArray(argsCalldata)) { | |
| args = argsCalldata; | |
| } else if ( | |
| typeof argsCalldata === "object" && | |
| argsCalldata !== null | |
| ) { | |
| args = abiMethod.inputs | |
| .filter((input) => !isLen(input.name)) | |
| .map((input) => { | |
| const value = (argsCalldata as Record<string, unknown>)[ | |
| input.name | |
| ]; | |
| if (value === undefined) { | |
| throw new Error( | |
| `Missing argument "${input.name}" for method "${method}"` | |
| ); | |
| } | |
| return value; | |
| }); | |
| } else { | |
| throw new Error( | |
| `Invalid argsCalldata type for method "${method}". Expected array or object.` | |
| ); | |
| } |
MartianGreed
force-pushed
the
feat/compile-calldata
branch
from
08287c9 to
3ba2b29
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/core/src/utils/compile.ts (1)
112-129: Add type validation forargsCalldataparameter.This was flagged in a previous review. The function assumes
argsCalldatais either an array or an object, but there's no explicit validation. If it's neither (e.g.,null,undefined, or a primitive), the code will fail silently or throw a cryptic error. Additionally,argsis typed asany[].
🧹 Nitpick comments (4)
packages/core/src/utils/compile.ts (2)
38-39: Avoidanytype in function ABI lookups.Multiple occurrences of
anytype when filtering/finding function items. Consider typing these more strictly using theFunctionAbitype from starknet imports.🔎 Suggested improvement
- item.items.some( - (fn: any) => - fn?.type === "function" && fn?.name === functionName - ) + item.items.some( + (fn: FunctionAbi) => + fn?.type === "function" && fn?.name === functionName + )Apply similar typing to all
.find()and.some()callbacks on lines 38-39, 46, 57-58, 65, and 72-73.As per coding guidelines, avoid
anytype; lean on generics, discriminated unions, and utility types.Also applies to: 46-46, 57-58, 65-65, 72-73
30-77: Consider extracting repeated interface search logic.The pattern of searching for a function within interface items is repeated three times with slight variations (exact match, namespace match, any interface). Extracting a helper would reduce duplication.
🔎 Possible helper extraction
function findFunctionInInterface( iface: InterfaceAbi, functionName: string ): FunctionAbi | undefined { return iface.items?.find( (fn): fn is FunctionAbi => fn?.type === "function" && fn?.name === functionName ); } function matchesInterface( item: unknown, functionName: string, namePredicate?: (name: string) => boolean ): item is InterfaceAbi { if ((item as any)?.type !== "interface") return false; const name = (item as InterfaceAbi).name || ""; if (namePredicate && !namePredicate(name)) return false; return !!findFunctionInInterface(item as InterfaceAbi, functionName); }packages/core/src/_test_/compile.test.ts (2)
5-5: Avoidanytype cast for manifest ABI.Consider creating a proper type for the manifest structure or using a more specific type assertion.
🔎 Suggested improvement
+import type { Abi } from "starknet"; + -const abi = manifest.abis as any; +const abi = manifest.abis as Abi;As per coding guidelines, avoid
anytype.
7-75: Consider adding tests forcompileDojoCalldata.The test suite only covers
findFunctionAbiByNamespace. ThecompileDojoCalldatafunction is also exported and would benefit from direct unit tests covering:
- Array vs object calldata input
- Missing argument error handling
- The
__compiled__flag behavior- Cairo1 vs legacy type handling
Would you like me to generate test cases for
compileDojoCalldata?
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
.changeset/yellow-garlics-judge.mdpackages/core/src/_test_/DojoProvider.test.tspackages/core/src/_test_/compile.test.tspackages/core/src/_test_/manifest_pistols_dev.jsonpackages/core/src/provider/DojoProvider.tspackages/core/src/utils/compile.tspackages/core/src/utils/index.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- .changeset/yellow-garlics-judge.md
- packages/core/src/utils/index.ts
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx,js,jsx,json}
📄 CodeRabbit inference engine (AGENTS.md)
Apply code formatting with 4 spaces, 80 character line width, double quotes, trailing commas, and semicolons
Files:
packages/core/src/_test_/DojoProvider.test.tspackages/core/src/_test_/compile.test.tspackages/core/src/utils/compile.tspackages/core/src/provider/DojoProvider.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx,js,jsx}: Keep imports grouped: node/external, blank line, workspace, then relative
React hooks/components follow strict dependency arrays and early returns
Prefer guard clauses and rethrow or surface context in error handling; avoid silent catches
Files:
packages/core/src/_test_/DojoProvider.test.tspackages/core/src/_test_/compile.test.tspackages/core/src/utils/compile.tspackages/core/src/provider/DojoProvider.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Maintain explicit return types for exported APIs when inference is unclear
Avoidanytype; lean on generics, discriminated unions, and utility types
Files:
packages/core/src/_test_/DojoProvider.test.tspackages/core/src/_test_/compile.test.tspackages/core/src/utils/compile.tspackages/core/src/provider/DojoProvider.ts
**/*.test.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Tests should remain deterministic; clean up observables/subscriptions between test cases
Files:
packages/core/src/_test_/DojoProvider.test.tspackages/core/src/_test_/compile.test.ts
🧬 Code graph analysis (2)
packages/core/src/_test_/DojoProvider.test.ts (1)
packages/core/src/provider/DojoProvider.ts (2)
DojoProvider(507-507)DojoProvider(515-516)
packages/core/src/provider/DojoProvider.ts (1)
packages/core/src/utils/compile.ts (1)
compileDojoCalldata(84-157)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build-packages
- GitHub Check: build-examples
- GitHub Check: build
🔇 Additional comments (11)
packages/core/src/utils/compile.ts (1)
79-82: LGTM!The fallback to top-level function search handles edge cases where functions aren't wrapped in interfaces.
packages/core/src/_test_/compile.test.ts (1)
8-20: LGTM!The test cases effectively validate the function lookup across different interfaces with varying input signatures.
packages/core/src/_test_/DojoProvider.test.ts (4)
4-15: LGTM!The starknet mock setup correctly preserves actual module functionality while stubbing RpcProvider and Contract for isolated unit testing.
130-135: LGTM!Good practice recreating the provider in
beforeEachfor test isolation.
137-179: LGTM!Comprehensive tests for method naming behavior with duplicate vs unique functions. The
as anycasts are acceptable here since methods are dynamically generated based on manifest content.
182-206: LGTM!Good coverage of method signature requirements including account validation for external methods and argument validation.
packages/core/src/provider/DojoProvider.ts (5)
19-20: LGTM!Import of
compileDojoCalldatais correctly grouped with other utility imports.
300-307: LGTM!Clean integration of
compileDojoCalldatafor thecall()method with proper parameter passing.
358-368: LGTM!Pre-computing function counts enables correct duplicate detection without repeated iterations.
393-400: LGTM!Clear logic for generating prefixed method names when duplicates exist, preventing naming collisions.
429-484: LGTM!Consistent use of
methodNamefor dynamic method registration and error messages ensures users see the correct method identifier (prefixed or not) in all contexts.
MartianGreed
force-pushed
the
feat/compile-calldata
branch
from
3ba2b29 to
3086e64
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Fix all issues with AI Agents 🤖
In @packages/core/src/provider/DojoProvider.ts:
- Around line 300-307: Add unit/integration tests that exercise the
compileDojoCalldata integration path used in DojoProvider (the code that calls
compileDojoCalldata and then contract.call). Create tests that invoke the
provider's call flow (or directly call compileDojoCalldata then contract.call
via a mocked contract) with: complex argument types including nested structs,
arrays, and enum-like values; missing required arguments to assert proper error
propagation; invalid argument types to assert validation errors; and very large
payloads to ensure performance/serialization stability. Use mocks or a test
double for contract.call to capture the compiled calldata output and assert it
matches expected encoding for each case, and add tests verifying that errors
thrown by compileDojoCalldata are propagated by the surrounding call path.
♻️ Duplicate comments (1)
packages/core/src/utils/compile.ts (1)
112-129: Address the unresolved type safety issue forargsCalldata.The previous review comment regarding
any[]usage and missing type validation forargsCalldatahas not been addressed. This remains a concern:
argsis typed asany[], violating coding guidelines- No validation ensures
argsCalldatais an array or object (could benull,undefined, or primitive)- Invalid inputs will produce cryptic errors instead of clear validation messages
🔎 Recommended fix
- let args: any[]; + let args: unknown[]; if (Array.isArray(argsCalldata)) { args = argsCalldata; - } else { + } else if ( + typeof argsCalldata === "object" && + argsCalldata !== null + ) { args = abiMethod.inputs .filter((input) => !isLen(input.name)) .map((input) => { const value = (argsCalldata as Record<string, unknown>)[ input.name ]; if (value === undefined) { throw new Error( `Missing argument "${input.name}" for method "${method}"` ); } return value; }); + } else { + throw new Error( + `Invalid argsCalldata type for method "${method}". Expected array or object.` + ); }As per coding guidelines, avoid
anytype and add explicit validation.
🧹 Nitpick comments (1)
packages/core/src/utils/compile.ts (1)
22-82: Avoidanytype for function items in predicates.The cascading ABI lookup logic is sound, but the code uses
fn: anyin multiple predicates (lines 38, 39, 46, 57, 58, 65, 72, 73). Per coding guidelines, avoidanyand use explicit types or utility types.🔎 Suggested type-safe approach
Define a type guard or use a more specific type for ABI items:
+type AbiItem = AbiEntry & { type?: string; name?: string }; + export function findFunctionAbiByNamespace( abi: Abi, namespace: string, contractName: string, functionName: string ): FunctionAbi | undefined { const contractPattern = `::${contractName}::`; const exactMatch = abi.find((item): item is InterfaceAbi => { if (item?.type !== "interface") return false; const name = item.name || ""; if (!name.startsWith(`${namespace}::`)) return false; if (!name.includes(contractPattern)) return false; return ( Array.isArray(item.items) && item.items.some( - (fn: any) => + (fn: AbiItem) => fn?.type === "function" && fn?.name === functionName ) ); }); if (exactMatch) { return exactMatch.items.find( - (fn: any) => fn?.type === "function" && fn?.name === functionName + (fn: AbiItem) => fn?.type === "function" && fn?.name === functionName ) as FunctionAbi | undefined; } // ... apply similar changes to remaining predicatesAs per coding guidelines, lean on utility types instead of
any.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
.changeset/yellow-garlics-judge.mdpackages/core/src/_test_/DojoProvider.test.tspackages/core/src/_test_/compile.test.tspackages/core/src/_test_/manifest_pistols_dev.jsonpackages/core/src/provider/DojoProvider.tspackages/core/src/utils/compile.tspackages/core/src/utils/index.ts
✅ Files skipped from review due to trivial changes (1)
- packages/core/src/test/compile.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/core/src/utils/index.ts
- packages/core/src/test/DojoProvider.test.ts
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx,js,jsx,json}
📄 CodeRabbit inference engine (AGENTS.md)
Apply code formatting with 4 spaces, 80 character line width, double quotes, trailing commas, and semicolons
Files:
packages/core/src/provider/DojoProvider.tspackages/core/src/utils/compile.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx,js,jsx}: Keep imports grouped: node/external, blank line, workspace, then relative
React hooks/components follow strict dependency arrays and early returns
Prefer guard clauses and rethrow or surface context in error handling; avoid silent catches
Files:
packages/core/src/provider/DojoProvider.tspackages/core/src/utils/compile.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Maintain explicit return types for exported APIs when inference is unclear
Avoidanytype; lean on generics, discriminated unions, and utility types
Files:
packages/core/src/provider/DojoProvider.tspackages/core/src/utils/compile.ts
🧬 Code graph analysis (1)
packages/core/src/provider/DojoProvider.ts (1)
packages/core/src/utils/compile.ts (1)
compileDojoCalldata(84-157)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build-examples
- GitHub Check: build-packages
- GitHub Check: build
🔇 Additional comments (7)
.changeset/yellow-garlics-judge.md (1)
1-5: LGTM! Changeset properly documents the patch.The changeset correctly declares a patch-level version bump with an appropriate feature description.
packages/core/src/provider/DojoProvider.ts (3)
19-19: LGTM! Import is clean and properly placed.The import follows coding guidelines for import grouping.
430-471: LGTM! Consistent use of methodName in dynamic methods and errors.The code correctly:
- Uses
methodNamefor both view and external method definitions- References
methodNamein all error messages for missing arguments and account requirements- Maintains the distinction between
methodName(user-facing) andsystemName(ABI-facing)
359-396: Duplicate method name disambiguation is properly tested.The duplicate detection and prefixing logic is confirmed sound, and comprehensive test coverage validates all scenarios:
- Multiple contracts with the same system name produce prefixed methods (
pistols_game_commit_moves,pistols_bot_player_commit_moves)- Unique system names remain unprefixed (
set_paused,reveal_moves,reply_duel,get_status)- Prefixed methods are independent and don't shadow each other
- No unprefixed version is created when duplicates exist
packages/core/src/utils/compile.ts (3)
14-20: LGTM! Helper functions are clear and focused.The
isLenandisCairo1Typeutility functions are straightforward and serve their purpose well.
84-111: LGTM! ABI resolution and error handling are clear.The function correctly:
- Resolves ABI method using
findFunctionAbiByNamespace- Handles constructor validation short-circuit
- Provides descriptive error messages when methods aren't found
The overall structure is sound. The type safety concern for
argsCalldatais flagged separately.
131-157: LGTM! Calldata construction follows starknet.js patterns.The code correctly:
- Uses
parseCalldataFieldwith proper context (structs, enums, parser)- Filters length-only inputs for non-Cairo1 types
- Marks the resulting array with a non-enumerable
__compiled__property
| const compiledCalldata = compileDojoCalldata( | ||
| contractInfos.abi, | ||
| nameSpace, | ||
| call.contractName, | ||
| call.entrypoint, | ||
| call.calldata as ArgsOrCalldata | ||
| call.calldata | ||
| ); | ||
| return await contract.call(call.entrypoint, compiledCalldata); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, verify the actual code at lines 300-307 in DojoProvider.ts
sed -n '295,310p' packages/core/src/provider/DojoProvider.tsRepository: dojoengine/dojo.js
Length of output: 760
🏁 Script executed:
# Check for other calldata compilation patterns that might need updating
rg -n 'new CallData\(' --type=ts packages/core/src | head -20Repository: dojoengine/dojo.js
Length of output: 44
🏁 Script executed:
# Find test files that exercise DojoProvider.call method
fd -e test.ts -e test.tsx packages/core | xargs rg -l 'DojoProvider' 2>/dev/null | head -10Repository: dojoengine/dojo.js
Length of output: 760
🏁 Script executed:
# Check for tests of compileDojoCalldata itself
fd -e test.ts -e test.tsx packages/core | xargs rg -l 'compileDojoCalldata' 2>/dev/nullRepository: dojoengine/dojo.js
Length of output: 547
🏁 Script executed:
# Verify the compileDojoCalldata function signature and documentation
rg -A 10 'export.*function compileDojoCalldata' --type=ts packages/core/srcRepository: dojoengine/dojo.js
Length of output: 740
🏁 Script executed:
# Check the test file for DojoProvider to see what's covered
cat -n packages/core/src/_test_/DojoProvider.test.ts | head -100Repository: dojoengine/dojo.js
Length of output: 4252
🏁 Script executed:
# Search for specific tests of the call method with compileDojoCalldata
rg -A 20 'call.*contractName.*entrypoint' packages/core/src/_test_/DojoProvider.test.tsRepository: dojoengine/dojo.js
Length of output: 44
🏁 Script executed:
# Check if there are tests for edge cases like complex argument types
rg -B 5 -A 10 'compileDojoCalldata' packages/core/src/_test_/DojoProvider.test.tsRepository: dojoengine/dojo.js
Length of output: 44
🏁 Script executed:
# See more of the test file to check what tests exist
tail -100 packages/core/src/_test_/DojoProvider.test.tsRepository: dojoengine/dojo.js
Length of output: 3880
🏁 Script executed:
# Search for test names related to 'call' method
rg 'it\(' packages/core/src/_test_/DojoProvider.test.tsRepository: dojoengine/dojo.js
Length of output: 695
🏁 Script executed:
# Check if there are any tests that call DojoProvider methods at all
rg -B 2 -A 5 'provider\.' packages/core/src/_test_/DojoProvider.test.ts | head -50Repository: dojoengine/dojo.js
Length of output: 44
Add test coverage for the compileDojoCalldata integration path.
The parameter mapping and error handling are correct. However, there are no tests covering the call method's calldata compilation path. Add tests exercising compileDojoCalldata with complex argument types (structs, arrays, enums) and edge cases such as missing arguments, invalid argument types, and large data structures to ensure the integration works correctly across the full range of contract method signatures.
🤖 Prompt for AI Agents
In @packages/core/src/provider/DojoProvider.ts around lines 300-307, Add
unit/integration tests that exercise the compileDojoCalldata integration path
used in DojoProvider (the code that calls compileDojoCalldata and then
contract.call). Create tests that invoke the provider's call flow (or directly
call compileDojoCalldata then contract.call via a mocked contract) with: complex
argument types including nested structs, arrays, and enum-like values; missing
required arguments to assert proper error propagation; invalid argument types to
assert validation errors; and very large payloads to ensure
performance/serialization stability. Use mocks or a test double for
contract.call to capture the compiled calldata output and assert it matches
expected encoding for each case, and add tests verifying that errors thrown by
compileDojoCalldata are propagated by the surrounding call path.
MartianGreed
force-pushed
the
feat/compile-calldata
branch
from
3086e64 to
d4944ad
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/core/src/utils/compile.ts (1)
112-137: Type validation forargsCalldataandany[]usage flagged in prior review.The past review correctly identified that:
argsis typed asany[]which violates coding guidelines- There's no validation for when
argsCalldatais neither an array nor an object (e.g.,null,undefined, or a primitive)Please address the suggestions from the prior review to add explicit type validation and replace
any[]withunknown[].
🧹 Nitpick comments (1)
packages/core/src/utils/compile.ts (1)
36-41: Consider typingfnparameter more precisely.The
fn: anyannotations throughout this function could be replaced with a more specific type to improve type safety. Since you're checkingfn?.type === "function", consider using a discriminated union or theFunctionAbitype from starknet.🔎 Proposed fix
- item.items.some( - (fn: any) => - fn?.type === "function" && fn?.name === functionName - ) + item.items.some( + (fn: FunctionAbi | { type: string; name?: string }) => + fn?.type === "function" && fn?.name === functionName + )As per coding guidelines, avoid
anytype and lean on discriminated unions and utility types.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
.changeset/yellow-garlics-judge.mdpackages/core/src/_test_/DojoProvider.test.tspackages/core/src/_test_/compile.test.tspackages/core/src/_test_/manifest_pistols_dev.jsonpackages/core/src/provider/DojoProvider.tspackages/core/src/utils/compile.tspackages/core/src/utils/index.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/core/src/utils/index.ts
- packages/core/src/test/compile.test.ts
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx,js,jsx,json}
📄 CodeRabbit inference engine (AGENTS.md)
Apply code formatting with 4 spaces, 80 character line width, double quotes, trailing commas, and semicolons
Files:
packages/core/src/utils/compile.tspackages/core/src/provider/DojoProvider.tspackages/core/src/_test_/DojoProvider.test.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx,js,jsx}: Keep imports grouped: node/external, blank line, workspace, then relative
React hooks/components follow strict dependency arrays and early returns
Prefer guard clauses and rethrow or surface context in error handling; avoid silent catches
Files:
packages/core/src/utils/compile.tspackages/core/src/provider/DojoProvider.tspackages/core/src/_test_/DojoProvider.test.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Maintain explicit return types for exported APIs when inference is unclear
Avoidanytype; lean on generics, discriminated unions, and utility types
Files:
packages/core/src/utils/compile.tspackages/core/src/provider/DojoProvider.tspackages/core/src/_test_/DojoProvider.test.ts
**/*.test.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Tests should remain deterministic; clean up observables/subscriptions between test cases
Files:
packages/core/src/_test_/DojoProvider.test.ts
🧬 Code graph analysis (2)
packages/core/src/provider/DojoProvider.ts (1)
packages/core/src/utils/compile.ts (1)
compileDojoCalldata(84-165)
packages/core/src/_test_/DojoProvider.test.ts (1)
packages/core/src/provider/DojoProvider.ts (2)
DojoProvider(507-507)DojoProvider(515-516)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build-packages
- GitHub Check: build-examples
- GitHub Check: build
🔇 Additional comments (12)
.changeset/yellow-garlics-judge.md (1)
1-5: LGTM!The changeset correctly declares a patch version for
@dojoengine/corewith an appropriate feature description that matches the PR scope.packages/core/src/utils/compile.ts (2)
14-20: LGTM!The helper functions are concise and correctly identify length parameters and Cairo1 types.
158-164: LGTM!Using a non-enumerable, non-writable property as a compilation marker is a clean approach to prevent double-compilation scenarios.
packages/core/src/_test_/DojoProvider.test.ts (4)
1-14: LGTM!The mock setup correctly replaces starknet's
RpcProviderandContractwhile preserving other exports. The import order (mock before DojoProvider import) ensures proper module interception.
16-127: LGTM!The mock manifest comprehensively covers the key test scenarios: duplicate function names across contracts (
commit_moves), unique functions, and different state mutabilities (viewvsexternal).
129-179: LGTM!The
initializeActionMethodstests comprehensively verify the duplicate detection and dynamic method naming behavior. The test cases cover prefixed names for duplicates, simple names for unique functions, and ensure no shadowing occurs.
181-205: LGTM!The action method signature tests properly validate error handling for missing accounts and arguments. The test for view methods correctly verifies they don't require an account.
As flagged in a prior review, consider adding tests that exercise the
compileDojoCalldatapath with complex argument types (structs, arrays, enums) to ensure full integration coverage.packages/core/src/provider/DojoProvider.ts (5)
19-21: LGTM!The import is correctly placed with other local utility imports and follows the grouping guidelines.
300-307: LGTM!The integration with
compileDojoCalldatais correctly implemented. The parameter mapping is accurate, and error handling is preserved.As noted in a prior review, consider adding integration tests for this path with complex argument types.
358-368: LGTM!The duplicate detection logic efficiently builds a count map in a single pass to identify function name collisions across contracts.
392-400: LGTM!The duplicate handling correctly generates prefixed method names for collisions while preserving simple names for unique functions. The early return prevents method overwrites.
429-484: LGTM!The dynamic method generation correctly uses
methodNamefor both host assignments and error messages, ensuring consistency between the callable method name and error diagnostics.
Closes #
Introduced changes
Checklist
Summary by CodeRabbit
Tests
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.