Skip to content

Conversation

MartianGreed
Copy link
Collaborator

@MartianGreed MartianGreed commented Sep 24, 2025

Closes #

Introduced changes

Checklist

  • Linked relevant issue
  • Updated relevant documentation
  • Added relevant tests
  • Add a dedicated CI job for new examples
  • Performed self-review of the code

Summary by CodeRabbit

  • New Features

    • Provider now auto-generates strongly typed action methods from ABIs.
    • Direction values now use enum objects with optional variants instead of plain strings.
  • Refactor

    • Example app updated to use the new provider API for spawn/move actions.
    • React context typing adjusted to align with the provider's new shape.
  • Documentation

    • Type docs, examples, architecture overview, roadmap, and knowledge base added/updated.
  • Tests

    • Added coverage for enum and optional-enum typing/behavior.
  • Chores

    • Ignored additional web build artifacts.

Copy link

coderabbitai bot commented Sep 24, 2025

Walkthrough

Replaces direct client action calls in examples with a locally exported, typed dojoProvider; adds a runtime-instantiated dojoProvider export; implements ABI-driven dynamic action methods on DojoProvider; reworks enum/option type extraction to use CairoCustomEnum/CairoOption; updates SDK typings, tests, examples, docs, and .gitignore.

Changes

Cohort / File(s) Summary
Example app: migrate to typed provider
examples/example-vite-react-sdk/src/App.tsx, examples/example-vite-react-sdk/src/useSystemCalls.ts, examples/example-vite-react-sdk/src/types.ts
Add/export dojoProvider in types.ts; replace client.actions.* calls with dojoProvider.* (e.g., move(account!, { direction }), spawn(account!)); update imports.
Core provider: dynamic action method generation
packages/core/src/provider/DojoProvider.ts
Introduce extensive TS types for action signatures; rename/structure provider internals as DojoProviderBase; add initializeActionMethods to scan manifest/ABIs and attach strongly-typed runtime action methods with basic arg validation; expose DojoProvider constructor alias returning a typed instance.
Core enums/types & tests: option/enum support
packages/core/src/types/index.ts, packages/core/src/types/index.test.ts, packages/core/src/types/example-usage.ts, packages/core/src/types/README.md
Rework enum extraction to expose full enum objects, variantNames, and variant maps; add support for core::option::OptionCairoOption; export/import CairoCustomEnum/CairoOption; update tests and examples to new enum/option shapes and aliases.
SDK web typings & React context
packages/sdk/src/web/execute.ts, packages/sdk/src/web/react/provider.tsx
Map dojo_starter::models::Direction to CairoCustomEnum (was string union); change DojoContextType.provider typing from DojoProvider instance to typeof DojoProvider constructor type.
Docs & architecture
etc/architecture/overview.md, etc/architecture/whats-next.md, etc/knowledge-base.md
Add architecture overview, roadmap, and knowledge-base documents describing system design, planned initiatives, and tooling.
Repo hygiene & build metadata
packages/sdk/.gitignore, examples/example-vite-react-sdk/tsconfig.app.tsbuildinfo
Ignore tsup.web.bundled_*; update tsbuildinfo roots to include new/removed source entries.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant App as App / Hook
  participant Types as examples/.../types.ts (dojoProvider)
  participant Prov as DojoProvider (instance)
  participant Manifest as World Manifest / ABI
  participant Chain as StarkNet RPC

  Note over Types,Prov: provider is constructed on module load
  Prov->>Manifest: read manifest & contract ABIs
  Prov-->>Prov: initializeActionMethods() — generate action methods (spawn, move...)
  Prov-->>Types: exported dojoProvider (instance with methods)

  Note over App,Types: runtime usage
  App->>Types: dojoProvider.move(account, {direction})
  Types->>Prov: call generated move method
  Prov->>Chain: build calldata & invoke function
  Chain-->>Prov: tx response
  Prov-->>App: Promise with invoke/receipt
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related PRs

Poem

I hop through ABIs with a twitchy ear,
I stitch up methods so calls are clear.
Enums and Options wear tidy names,
I bind the hops and call the chains.
Carrots, types, and cozy dev cheer. 🥕🐇

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Description Check ⚠️ Warning The pull request description remains a bare template with placeholder content and lacks any summary of the actual changes, linked issues, documentation updates, or testing details. The Introduced changes section is empty and the checklist items are all unchecked, providing no actionable context for reviewers. This makes it impossible to understand the scope and purpose of the PR without inspecting the diffs manually. Please update the PR description to include a concise summary of the changes, reference any related issues, describe documentation and test additions, and mark completed checklist items to give reviewers clear context and ensure all quality steps have been addressed.
✅ Passed checks (2 passed)
Check name Status Explanation
Title Check ✅ Passed The title succinctly indicates adding typed provider support for handling CairoCustomEnum, which aligns with the main change set introducing dynamic typified provider methods and CairoCustomEnum type handling. It avoids vague wording and clearly relates to the core enhancement. However, its phrasing is slightly awkward and could be clearer to help teammates scanning history.
Docstring Coverage ✅ Passed No functions found in the changes. Docstring coverage check skipped.
✨ Finishing touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch feat/types-cairo-option

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@MartianGreed MartianGreed force-pushed the feat/types-cairo-option branch from b3b6f47 to d86e8c1 Compare September 24, 2025 13:19
@MartianGreed MartianGreed changed the title feat(types): fix provider + handle CairoCustomEnum feat(core): fix provider + handle CairoCustomEnum Sep 24, 2025
@MartianGreed MartianGreed changed the title feat(core): fix provider + handle CairoCustomEnum feat(core): typed provider + handle CairoCustomEnum Sep 24, 2025
@MartianGreed MartianGreed changed the title feat(core): typed provider + handle CairoCustomEnum feat(core): typed provider handle CairoCustomEnum Sep 24, 2025
@MartianGreed MartianGreed changed the title feat(core): typed provider handle CairoCustomEnum feat: typed provider handle CairoCustomEnum Sep 24, 2025
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: 7

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)
packages/sdk/src/web/execute.ts (2)

93-99: Accept both ProviderInterface and AccountInterface

The proxy should accept either. Narrowing to AccountInterface makes it harder to reuse.

-export function createWorldProxy<T extends readonly ContractDefinition[]>(
-    contractDefinitions: T,
-    providerOrAccount: AccountInterface
-): WorldContracts<T> {
+export function createWorldProxy<T extends readonly ContractDefinition[]>(
+    contractDefinitions: T,
+    providerOrAccount: ProviderInterface | AccountInterface
+): WorldContracts<T> {

110-114: Incorrect Contract constructor signature (will fail at runtime and type-check)

starknet.js expects positional args: new Contract(abi, address, providerOrAccount). The options-object form isn’t supported.

-        const contract = new Contract({
-            abi: contractDef.abi as AbiItem[],
-            address: contractDef.address,
-            providerOrAccount: providerOrAccount,
-        });
+        const contract = new Contract(
+            contractDef.abi as any,
+            contractDef.address,
+            providerOrAccount
+        );

If you want to preserve types strictly, cast to the proper ABI type exported by starknet instead of any.

🧹 Nitpick comments (4)
examples/example-vite-react-sdk/src/useSystemCalls.ts (1)

54-56: Guard against missing account; remove dead code

Non-null assertions can crash when not connected. Also remove the commented legacy call.

-            // await client.actions.spawn(account!);
-            await dojoProvider.spawn(account!);
+            if (!account) {
+                throw new Error("Connect your wallet before spawning.");
+            }
+            await dojoProvider.spawn(account);
examples/example-vite-react-sdk/src/App.tsx (1)

120-122: Avoid non-null assertion; noop when not connected

Prevents runtime errors on clicks before connect.

-                                        await dojoProvider.move(account!, {
-                                            direction: direction,
-                                        });
+                                        if (!account) return;
+                                        await dojoProvider.move(account, {
+                                            direction,
+                                        });
examples/example-vite-react-sdk/src/types.ts (1)

1-6: Consider adding error handling for provider initialization.

The provider is instantiated at module load time without any error handling. If the manifest or RPC URL is invalid, this could cause runtime errors during module initialization.

Consider wrapping the provider initialization in a try-catch block or using a factory function:

-export const dojoProvider = new DojoProvider<DojoStarterActions>(
-    dojoConfig.manifest,
-    dojoConfig.rpcUrl
-);
+export const createDojoProvider = () => {
+    try {
+        return new DojoProvider<DojoStarterActions>(
+            dojoConfig.manifest,
+            dojoConfig.rpcUrl
+        );
+    } catch (error) {
+        console.error("Failed to initialize DojoProvider:", error);
+        throw new Error(`DojoProvider initialization failed: ${error}`);
+    }
+};
+
+export const dojoProvider = createDojoProvider();
packages/core/src/provider/DojoProvider.ts (1)

115-116: Consider making action method initialization optional or lazy.

The initializeActionMethods() is called in the constructor, which could fail silently if the manifest structure is unexpected. Consider adding error handling or making this initialization configurable.

 constructor(
     manifest?: any,
     url: string = LOCAL_KATANA,
-    logLevel: LogLevel = "none"
+    logLevel: LogLevel = "none",
+    autoInitializeActions: boolean = true
 ) {
     super(manifest.world.address);
     this.provider = new RpcProvider({
         nodeUrl: url,
     });

     this.contract = new Contract({
         abi: manifest.world.abi,
         address: this.getWorldAddress(),
         providerOrAccount: this.provider,
     });
     this.manifest = manifest;
     this.logger = new ConsoleLogger({ level: logLevel });

-    this.initializeActionMethods();
+    if (autoInitializeActions) {
+        try {
+            this.initializeActionMethods();
+        } catch (error) {
+            this.logger.error("Failed to initialize action methods:", error);
+            if (logLevel !== "none") {
+                throw error;
+            }
+        }
+    }
 }
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 17df0bb and d86e8c1.

📒 Files selected for processing (11)
  • examples/example-vite-react-sdk/src/App.tsx (2 hunks)
  • examples/example-vite-react-sdk/src/types.ts (1 hunks)
  • examples/example-vite-react-sdk/src/useSystemCalls.ts (2 hunks)
  • packages/core/src/provider/DojoProvider.ts (3 hunks)
  • packages/core/src/types/README.md (1 hunks)
  • packages/core/src/types/example-usage.ts (4 hunks)
  • packages/core/src/types/index.test.ts (4 hunks)
  • packages/core/src/types/index.ts (4 hunks)
  • packages/sdk/.gitignore (1 hunks)
  • packages/sdk/src/web/execute.ts (3 hunks)
  • packages/sdk/src/web/react/provider.tsx (1 hunks)
🧰 Additional context used
🧬 Code graph analysis (6)
packages/sdk/src/web/react/provider.tsx (1)
packages/core/src/provider/DojoProvider.ts (1)
  • DojoProvider (430-431)
examples/example-vite-react-sdk/src/useSystemCalls.ts (1)
examples/example-vite-react-sdk/src/types.ts (1)
  • dojoProvider (11-14)
examples/example-vite-react-sdk/src/types.ts (3)
packages/core/src/types/index.ts (1)
  • ExtractAbiTypes (519-525)
examples/example-vite-react-sdk/src/dojo_starter_dev.ts (1)
  • compiledAbi (4-3663)
packages/core/src/provider/DojoProvider.ts (1)
  • DojoProvider (430-431)
packages/core/src/provider/DojoProvider.ts (3)
packages/create-burner/src/connectors/burner.ts (1)
  • account (78-80)
packages/core/src/logger/logger.ts (1)
  • LogLevel (14-14)
examples/example-vite-react-app-recs/src/dojo/DojoContext.tsx (1)
  • DojoProvider (14-58)
packages/core/src/types/example-usage.ts (2)
packages/core/src/provider/DojoProvider.ts (1)
  • DojoProvider (430-431)
examples/example-vite-react-sdk/src/dojo_starter_dev.ts (1)
  • compiledAbi (4-3663)
examples/example-vite-react-sdk/src/App.tsx (1)
examples/example-vite-react-sdk/src/types.ts (1)
  • dojoProvider (11-14)
⏰ 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 (19)
packages/sdk/.gitignore (1)

4-4: Add web bundle artifacts to ignore — good catch

Ignoring tsup.web.bundled_* mirrors the node pattern and prevents noisy artifacts from being tracked.

packages/core/src/types/index.ts (5)

1-1: Public surface import expansion is appropriate

Exporting CairoCustomEnum and CairoOption aligns with new enum handling.


328-334: Variant map utility looks solid

CairoEnumVariantMap cleanly maps variant payloads via MapCairoType.


335-347: Enum variant extraction is correct

First-match-by-name avoids unions from duplicates. Good.


348-365: CairoCustomEnum/CairoOption typing is well-modeled

Option specialization and metadata props for variant names are a practical compromise without deep generic modeling.


380-383: Exposing variantNames and variants on enums is useful

This simplifies downstream typing and IDE hints.

packages/core/src/types/README.md (2)

51-52: Enum docs update reads clearly

Calling out CairoCustomEnum/CairoOption and variant metadata improves discoverability.


57-64: Good examples for Direction and OptionalDirection

The split between enum wrapper and value/variants is clear and consistent with the types.

examples/example-vite-react-sdk/src/useSystemCalls.ts (1)

5-5: Switching to dojoProvider import — good

Keeps example aligned with the new provider surface.

examples/example-vite-react-sdk/src/App.tsx (1)

15-15: Importing dojoProvider — good

Matches the new action surface usage.

packages/core/src/types/index.test.ts (3)

2-2: Testing against CairoCustomEnum/CairoOption — good coverage

Imports match the new public surface.


41-56: Option enums added to sample ABI — appropriate

Covers nested enum-in-option and primitive option cases.


116-169: Type assertions validate the new enum model well

Checks for enum value type, variant names, and option Some/None payloads look correct.

examples/example-vite-react-sdk/src/types.ts (1)

7-14: LGTM! Clean type extraction pattern.

The type extraction pattern using ExtractAbiTypes and accessing the specific interface through the nested path is well-structured and maintains good type safety.

packages/core/src/types/example-usage.ts (3)

44-45: Fix typo in type name: "OptionnalNumber".

There's a spelling error in the type alias name.
[raise_minor_issue]

-type OptionnalNumber = MyAbiEnums["core::option::Option::<core::integer::u32>"];
+type OptionalNumber = MyAbiEnums["core::option::Option::<core::integer::u32>"];

62-75: LGTM! Excellent demonstration of the new provider API.

The examples effectively showcase the typed action methods with proper CairoCustomEnum usage for Direction and other enum types. This provides clear guidance for developers using the new API.


96-104: LGTM! Comprehensive CairoOption usage examples.

The examples clearly demonstrate both None and Some variants of CairoOption with proper type annotations, providing excellent reference material for handling optional values.

packages/core/src/provider/DojoProvider.ts (2)

22-80: LGTM! Well-designed type system for action methods.

The type definitions for dynamic action method generation are sophisticated and well-structured:

  • Proper handling of optional inputs
  • Clean separation between union and intersection types
  • Type-safe normalization of action interfaces

This provides excellent compile-time safety for the generated action methods.


420-431: LGTM! Clean type exports with proper abstraction.

The type exports elegantly hide the base implementation while providing a strongly-typed constructor interface. The use of type assertion to transform the base class into a constructor type is a clever pattern for achieving the desired API surface.

Comment on lines +329 to +404
private initializeActionMethods(): void {
if (!this.manifest?.contracts) {
return;
}

const host = this as unknown as Record<
string,
ActionMethodImplementation
>;

for (const contract of this.manifest.contracts as Array<any>) {
if (
!contract?.systems?.length ||
typeof contract.tag !== "string"
) {
continue;
}

const names = this.parseContractTag(contract.tag);
if (!names) {
continue;
}

const abiItems = Array.isArray(contract.abi) ? contract.abi : [];

for (const systemName of contract.systems as Array<string>) {
if (systemName in host) {
continue;
}

const interfaceAbi = abiItems.find(
(item: any) =>
item?.type === "interface" &&
item?.items.find(
(i: any) =>
i?.type === "function" && i?.name === systemName
)
);

if (!interfaceAbi) {
continue;
}
const functionAbi = interfaceAbi.items.find(
(i: any) => i?.type === "function" && i?.name === systemName
);
if (!functionAbi) {
continue;
}

const expectsArgs = Array.isArray(functionAbi.inputs)
? functionAbi.inputs.length > 0
: false;

host[systemName] = async (
account: Account | AccountInterface,
args?: Record<string, unknown>
) => {
if (expectsArgs && args === undefined) {
throw new Error(
`Missing arguments for action "${systemName}"`
);
}

return this.execute(
account,
{
contractName: names.contractName,
entrypoint: systemName,
calldata: (args ?? []) as unknown as ArgsOrCalldata,
},
names.namespace
);
};
}
}
}
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Add validation and error handling in action method generation.

The initializeActionMethods function has several areas where additional validation would improve robustness:

  1. The contract tag parsing could be more defensive
  2. Missing validation for ABI structure
  3. No handling for duplicate system names across contracts
 private initializeActionMethods(): void {
     if (!this.manifest?.contracts) {
         return;
     }

     const host = this as unknown as Record<
         string,
         ActionMethodImplementation
     >;

+    const systemRegistry = new Set<string>();
+
     for (const contract of this.manifest.contracts as Array<any>) {
         if (
             !contract?.systems?.length ||
             typeof contract.tag !== "string"
         ) {
             continue;
         }

         const names = this.parseContractTag(contract.tag);
         if (!names) {
+            this.logger.warn(`Invalid contract tag format: ${contract.tag}`);
             continue;
         }

         const abiItems = Array.isArray(contract.abi) ? contract.abi : [];
+        if (abiItems.length === 0) {
+            this.logger.warn(`Empty ABI for contract: ${contract.tag}`);
+            continue;
+        }

         for (const systemName of contract.systems as Array<string>) {
-            if (systemName in host) {
+            if (systemRegistry.has(systemName)) {
+                this.logger.warn(
+                    `Duplicate system name "${systemName}" found in contract "${contract.tag}". Skipping.`
+                );
                 continue;
             }
+            systemRegistry.add(systemName);

             const interfaceAbi = abiItems.find(
                 (item: any) =>
                     item?.type === "interface" &&
                     item?.items.find(
                         (i: any) =>
                             i?.type === "function" && i?.name === systemName
                     )
             );

             if (!interfaceAbi) {
+                this.logger.warn(
+                    `No interface found for system "${systemName}" in contract "${contract.tag}"`
+                );
                 continue;
             }
             const functionAbi = interfaceAbi.items.find(
                 (i: any) => i?.type === "function" && i?.name === systemName
             );
             if (!functionAbi) {
                 continue;
             }

             const expectsArgs = Array.isArray(functionAbi.inputs)
                 ? functionAbi.inputs.length > 0
                 : false;

             host[systemName] = async (
                 account: Account | AccountInterface,
                 args?: Record<string, unknown>
             ) => {
                 if (expectsArgs && args === undefined) {
                     throw new Error(
                         `Missing arguments for action "${systemName}"`
                     );
                 }
+                if (!expectsArgs && args !== undefined) {
+                    this.logger.warn(
+                        `Unexpected arguments provided for action "${systemName}"`
+                    );
+                }

                 return this.execute(
                     account,
                     {
                         contractName: names.contractName,
                         entrypoint: systemName,
                         calldata: (args ?? []) as unknown as ArgsOrCalldata,
                     },
                     names.namespace
                 );
             };
         }
     }
 }

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

🤖 Prompt for AI Agents
packages/core/src/provider/DojoProvider.ts lines 329-404: the action method
generator should be more defensive — first ensure parseContractTag returns a
valid object with expected properties (e.g., contractName and namespace) before
using them; validate ABI shapes before accessing nested arrays (confirm
contract.abi is an array, interfaceAbi.items is an array, functionAbi.inputs is
either undefined or an array) and bail out with a clear error/log when the ABI
shape is unexpected; prevent silent collisions by detecting duplicate systemName
across contracts and either skip adding a duplicate method and log a warning or
throw a descriptive error (do not overwrite an existing host[systemName]
silently); and add a guard so expectsArgs only treats inputs as present if they
are a real array — implement these checks and short-circuit with informative
errors/logs where validation fails.

Comment on lines +406 to +418
private parseContractTag(
tag: string
): { namespace: string; contractName: string } | null {
const separatorIndex = tag.lastIndexOf("-");
if (separatorIndex === -1) {
return null;
}

return {
namespace: tag.slice(0, separatorIndex),
contractName: tag.slice(separatorIndex + 1),
};
}
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Improve contract tag parsing robustness.

The parseContractTag method assumes a specific format with a single separator. Consider validating the parsed values.

 private parseContractTag(
     tag: string
 ): { namespace: string; contractName: string } | null {
+    if (!tag || typeof tag !== "string") {
+        return null;
+    }
+
     const separatorIndex = tag.lastIndexOf("-");
-    if (separatorIndex === -1) {
+    if (separatorIndex === -1 || separatorIndex === 0 || separatorIndex === tag.length - 1) {
         return null;
     }

-    return {
-        namespace: tag.slice(0, separatorIndex),
-        contractName: tag.slice(separatorIndex + 1),
-    };
+    const namespace = tag.slice(0, separatorIndex).trim();
+    const contractName = tag.slice(separatorIndex + 1).trim();
+
+    if (!namespace || !contractName) {
+        return null;
+    }
+
+    return { namespace, contractName };
 }
📝 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
private parseContractTag(
tag: string
): { namespace: string; contractName: string } | null {
const separatorIndex = tag.lastIndexOf("-");
if (separatorIndex === -1) {
return null;
}
return {
namespace: tag.slice(0, separatorIndex),
contractName: tag.slice(separatorIndex + 1),
};
}
private parseContractTag(
tag: string
): { namespace: string; contractName: string } | null {
if (!tag || typeof tag !== "string") {
return null;
}
const separatorIndex = tag.lastIndexOf("-");
if (separatorIndex === -1 || separatorIndex === 0 || separatorIndex === tag.length - 1) {
return null;
}
const namespace = tag.slice(0, separatorIndex).trim();
const contractName = tag.slice(separatorIndex + 1).trim();
if (!namespace || !contractName) {
return null;
}
return { namespace, contractName };
}
🤖 Prompt for AI Agents
In packages/core/src/provider/DojoProvider.ts around lines 406 to 418, the
parseContractTag currently splits on the last "-" and returns whatever
substrings result without validation; update it to validate and sanitize the
parsed values by ensuring the separator exists, both namespace and contractName
are non-empty after trimming, and optionally match expected patterns (e.g.,
alphanumeric, dashes/underscores) before returning; if validation fails, return
null (or throw consistently with surrounding behavior), and add comments/tests
to cover malformed tags (empty parts, leading/trailing separators, or invalid
characters).

Comment on lines +51 to +60
const provider = new DojoProvider<[DojoStarterActions, WorldActions]>(
compiledAbi
);
const account = new Account({
provider: provider as unknown as ProviderInterface,
address: "0x0",
signer: "0x0",
cairoVersion: "1",
transactionVersion: ETransactionVersion.V3,
});
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

🧩 Analysis chain

Verify provider type assertion safety.

The provider is cast to unknown then to ProviderInterface when creating the Account. This type assertion could hide type incompatibilities.


🏁 Script executed:

#!/bin/bash
# Check if DojoProvider properly implements or extends ProviderInterface
ast-grep --pattern 'class DojoProviderBase extends $_ {
  $$$
}'

# Check for ProviderInterface implementation in the codebase
rg -n --type=ts "implements ProviderInterface|extends.*ProviderInterface" packages/core/src/provider/

Length of output: 27302


🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "Searching for ProviderInterface in repo..."
rg -n --hidden --glob '!node_modules' "ProviderInterface" || true

echo
echo "Show top imports + class signature of DojoProvider.ts"
sed -n '1,140p' packages/core/src/provider/DojoProvider.ts || true

echo
echo "Show example-usage.ts around lines 1-120"
sed -n '1,140p' packages/core/src/types/example-usage.ts || true

echo
echo "Search for Account constructor/type definitions in repo (class Account or AccountInterface)"
rg -n --hidden --glob '!node_modules' "class Account|interface AccountInterface|type AccountInterface" || true

echo
echo "Search for imports of Account in packages/core"
rg -n --hidden --glob '!node_modules' "import .*Account" packages/core || true

Length of output: 9933


🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "Files matching DojoProvider usage:"
rg -n --hidden --glob '!node_modules' "DojoProvider" || true

echo
echo "Show full DojoProvider.ts"
sed -n '1,999p' packages/core/src/provider/DojoProvider.ts || true

echo
echo "Show provider base file"
sed -n '1,999p' packages/core/src/provider/provider.ts || true

echo
echo "Show index/export in packages/core/src/provider"
sed -n '1,240p' packages/core/src/provider/index.ts || true

Length of output: 16823


Stop using as unknown as ProviderInterface — make the provider shape explicit.

The example casts the DojoProvider instance to ProviderInterface (packages/core/src/types/example-usage.ts) while DojoProviderBase only extends your local Provider and is exported via an unsafe unknown-cast (packages/core/src/provider/DojoProvider.ts); that hidden cast can mask structural mismatches and cause runtime errors.
Either implement the external ProviderInterface on DojoProviderBase or provide an adapter/wrapper that satisfies ProviderInterface, or pass the underlying RpcProvider (e.g. provider.provider) to Account instead of casting.
Locations: packages/core/src/types/example-usage.ts (account creation), packages/core/src/provider/DojoProvider.ts (class/export).

@@ -1,4 +1,4 @@
import { type AccountInterface, Contract } from "starknet";
import { type AccountInterface, CairoCustomEnum, Contract } from "starknet";
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Import ProviderInterface and fix Contract ctor usage

You’ll need ProviderInterface for proper typing once the ctor is fixed (see below).

Apply this diff:

-import { type AccountInterface, CairoCustomEnum, Contract } from "starknet";
+import {
+  type AccountInterface,
+  type ProviderInterface,
+  CairoCustomEnum,
+  Contract,
+} from "starknet";
📝 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
import { type AccountInterface, CairoCustomEnum, Contract } from "starknet";
import {
type AccountInterface,
type ProviderInterface,
CairoCustomEnum,
Contract,
} from "starknet";
🤖 Prompt for AI Agents
In packages/sdk/src/web/execute.ts around line 1, the module import and Contract
construction are incorrect: add ProviderInterface to the import list and update
the Contract constructor call to match the library signature by passing ABI,
address, and the provider (typed as ProviderInterface) in the correct order;
ensure the provider variable is typed as ProviderInterface and passed into new
Contract(...) rather than using the old ctor argument order.

Comment on lines +30 to 31
"dojo_starter::models::Direction": CairoCustomEnum;
}
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Don’t hardcode app-specific FQNs in SDK type mapping

Mapping "dojo_starter::models::Direction" here bakes app semantics into SDK. Prefer:

  • derive enum names from ABI and map all custom enums to CairoCustomEnum (and Option to CairoOption), or
  • allow a caller-provided mapping to extend defaults.

If you need a quick stopgap, move this mapping into example/app code rather than SDK.

🤖 Prompt for AI Agents
In packages/sdk/src/web/execute.ts around lines 30-31, the SDK currently
hardcodes the app-specific FQN "dojo_starter::models::Direction" into its type
mapping; remove this hardcoded entry and instead: implement a generic mapping
that derives enum names from the ABI (treat any custom enum types as
CairoCustomEnum and any Option<T>-style types as CairoOption), and add a
parameter or config hook so callers can supply/extend custom FQN→type mappings;
as a quick alternative, move the current hardcoded mapping out of the SDK into
the example/app code so the SDK contains only the generic ABI-driven defaults.

Comment on lines +183 to +185
// await world.actions.move({
// direction: new CairoCustomEnum({ Left: "()" }),
// });
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

🧩 Analysis chain

Verify unit variant construction for CairoCustomEnum

Using "()" for zero-sized variants is common in examples, but starknet.js may accept { Up: {} } as well. Please confirm the current API.


🌐 Web query:

How should unit variants be constructed for CairoCustomEnum in starknet.js? Is `{ Up: "()" }` correct, or should it be `{ Up: {} }`?

💡 Result:

Use an empty object. For unit (empty) variants create the enum like: new CairoCustomEnum({ Up: {} }). [1][2]

Sources: Starknet.js Cairo Enums guide and CairoCustomEnum API. [1] https://starknetjs.com/docs/guides/cairo_enum/ (example uses { Empty: {} }) [2] https://starknetjs.com/docs/API/classes/CairoCustomEnum/


Use {} for unit variants (CairoCustomEnum)

starknet.js expects unit variants as an empty object — replace new CairoCustomEnum({ Left: "()" }) with new CairoCustomEnum({ Left: {} }).

File: packages/sdk/src/web/execute.ts (lines 183-185)

🤖 Prompt for AI Agents
In packages/sdk/src/web/execute.ts around lines 183 to 185, the commented
example constructs a CairoCustomEnum unit variant incorrectly using the string
"()" — replace the unit variant value with an empty object instead (i.e., use
Left: {}), so the CairoCustomEnum is created with the expected unit variant
shape for starknet.js.

sdk: SDK<Schema>;
/** The Dojo provider */
provider: DojoProvider;
provider: typeof DojoProvider;
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Type mismatch: provider is an instance, not a constructor

Context declares typeof DojoProvider (ctor) but passes an instance. This breaks typing.

-    provider: typeof DojoProvider;
+    provider: InstanceType<typeof DojoProvider>;

And this aligns with provider: dojoProvider below.

📝 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
provider: typeof DojoProvider;
provider: InstanceType<typeof DojoProvider>;
🤖 Prompt for AI Agents
In packages/sdk/src/web/react/provider.tsx around line 27, the prop type is
declared as `typeof DojoProvider` (a constructor) but the code passes an
instance (`dojoProvider`); change the type to the instance type — either
`DojoProvider` (if the class/interface is exported as a type) or
`InstanceType<typeof DojoProvider>` — and ensure the appropriate import/export
exists so the prop signature matches the actual instance being passed.

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

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (3)
packages/core/src/provider/DojoProvider.ts (3)

97-116: Constructor uses optional manifest but dereferences it unguarded (runtime crash).

manifest is optional but used immediately (manifest.world.address, manifest.world.abi). This will throw if manifest is undefined or malformed.

Apply this diff to require a valid manifest and fail fast:

-    constructor(
-        manifest?: any,
-        url: string = LOCAL_KATANA,
-        logLevel: LogLevel = "none"
-    ) {
-        super(manifest.world.address);
+    constructor(
+        manifest: any,
+        url: string = LOCAL_KATANA,
+        logLevel: LogLevel = "none"
+    ) {
+        if (!manifest?.world?.address || !manifest?.world?.abi) {
+            throw new Error("Invalid manifest: missing world.address or world.abi");
+        }
+        super(manifest.world.address);
         this.provider = new RpcProvider({
             nodeUrl: url,
         });
 
         this.contract = new Contract({
-            abi: manifest.world.abi,
+            abi: manifest.world.abi,
             address: this.getWorldAddress(),
             providerOrAccount: this.provider,
         });
         this.manifest = manifest;
         this.logger = new ConsoleLogger({ level: logLevel });
 
         this.initializeActionMethods();
     }

240-257: Don’t use optional chaining on a required parameter; add a guard or remove ?..

If account were ever undefined, you’d return undefined and violate the return type. Use a guard and call directly.

-        try {
-            return await account?.execute(calls, details);
+        try {
+            if (!account) {
+                throw new Error("Account is required");
+            }
+            return await account.execute(calls, details);
         } catch (error) {
             this.logger.error("Error occured: ", error);
             throw error;
         }

Also consider renaming nameSpace to namespace for consistency across the API.


209-224: Destructure and validate result from callContract
The RpcProvider.callContract v6 returns { result: string[] }, not a bare array—destructure result, guard with Array.isArray, and parse with radix 10:

-            const result = await this.provider.callContract({
+            const { result } = await this.provider.callContract({
                 contractAddress: this.getWorldAddress(),
                 entrypoint: WorldEntryPoints.uuid,
                 calldata: [],
             });
-            if (result && result.length === 1) {
-                return parseInt(result[0]);
+            if (Array.isArray(result) && result.length === 1) {
+                return parseInt(result[0], 10);
             }
             throw new Error("Contract did not return expected uuid");
🧹 Nitpick comments (2)
packages/core/src/provider/DojoProvider.ts (2)

159-178: Incorrect return type cast; avoid casting a value to Promise.

You’re casting the awaited value to a Promise type. In an async function, return the value (TS will wrap it). The current cast hides type errors.

Apply this diff:

-    ): Promise<Array<Array<bigint>>> {
+    ): Promise<Array<Array<bigint>>> {
         try {
-            return (await this.contract.call(WorldEntryPoints.entities, [
+            return (await this.contract.call(WorldEntryPoints.entities, [
                 shortString.encodeShortString(model),
                 index,
                 values,
                 valuesLength,
                 valuesLayout,
-            ])) as unknown as Promise<Array<Array<bigint>>>;
+            ])) as unknown as Array<Array<bigint>>;
         } catch (error) {
             this.logger.error("Error occured: ", error);
             throw error;
         }
     }

145-146: Fix typos in log messages (“occured” → “occurred”).

Minor polish for logs.

Also applies to: 175-176, 192-193, 254-255

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d86e8c1 and c1531d6.

📒 Files selected for processing (1)
  • packages/core/src/provider/DojoProvider.ts (3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/core/src/provider/DojoProvider.ts (2)
packages/create-burner/src/connectors/burner.ts (1)
  • account (78-80)
packages/core/src/logger/logger.ts (1)
  • LogLevel (14-14)
⏰ 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 (5)
packages/core/src/provider/DojoProvider.ts (5)

329-404: Harden dynamic action method generation (defensive checks, duplicates, ABI shape).

Add guards for malformed tags, empty/invalid ABI, duplicate system names, and unexpected args to prevent silent footguns.

     private initializeActionMethods(): void {
         if (!this.manifest?.contracts) {
             return;
         }
 
         const host = this as unknown as Record<
             string,
             ActionMethodImplementation
         >;
 
+        const systemRegistry = new Set<string>();
+
         for (const contract of this.manifest.contracts as Array<any>) {
             if (
                 !contract?.systems?.length ||
                 typeof contract.tag !== "string"
             ) {
                 continue;
             }
 
             const names = this.parseContractTag(contract.tag);
             if (!names) {
+                this.logger.warn(`Invalid contract tag format: ${contract.tag}`);
                 continue;
             }
 
-            const abiItems = Array.isArray(contract.abi) ? contract.abi : [];
+            const abiItems = Array.isArray(contract.abi) ? contract.abi : [];
+            if (abiItems.length === 0) {
+                this.logger.warn(`Empty ABI for contract: ${contract.tag}`);
+                continue;
+            }
 
             for (const systemName of contract.systems as Array<string>) {
-                if (systemName in host) {
+                if (systemRegistry.has(systemName)) {
+                    this.logger.warn(
+                        `Duplicate system name "${systemName}" found in contract "${contract.tag}". Skipping.`
+                    );
                     continue;
                 }
+                systemRegistry.add(systemName);
 
                 const interfaceAbi = abiItems.find(
                     (item: any) =>
                         item?.type === "interface" &&
-                        item?.items.find(
+                        Array.isArray(item?.items) && item?.items.find(
                             (i: any) =>
                                 i?.type === "function" && i?.name === systemName
                         )
                 );
 
                 if (!interfaceAbi) {
+                    this.logger.warn(
+                        `No interface found for system "${systemName}" in contract "${contract.tag}"`
+                    );
                     continue;
                 }
-                const functionAbi = interfaceAbi.items.find(
+                const functionAbi = Array.isArray(interfaceAbi.items) && interfaceAbi.items.find(
                     (i: any) => i?.type === "function" && i?.name === systemName
                 );
                 if (!functionAbi) {
                     continue;
                 }
 
                 const expectsArgs = Array.isArray(functionAbi.inputs)
                     ? functionAbi.inputs.length > 0
                     : false;
 
                 host[systemName] = async (
                     account: Account | AccountInterface,
                     args?: Record<string, unknown>
                 ) => {
                     if (expectsArgs && args === undefined) {
                         throw new Error(
                             `Missing arguments for action "${systemName}"`
                         );
                     }
+                    if (!expectsArgs && args !== undefined) {
+                        this.logger.warn(
+                            `Unexpected arguments provided for action "${systemName}"`
+                        );
+                    }
 
                     return this.execute(
                         account,
                         {
                             contractName: names.contractName,
                             entrypoint: systemName,
                             calldata: (args ?? []) as unknown as ArgsOrCalldata,
                         },
                         names.namespace
                     );
                 };
             }
         }
     }

406-418: Make contract tag parsing robust (validate, trim, early null on bad formats).

-    private parseContractTag(
-        tag: string
-    ): { namespace: string; contractName: string } | null {
-        const separatorIndex = tag.lastIndexOf("-");
-        if (separatorIndex === -1) {
-            return null;
-        }
-
-        return {
-            namespace: tag.slice(0, separatorIndex),
-            contractName: tag.slice(separatorIndex + 1),
-        };
-    }
+    private parseContractTag(
+        tag: string
+    ): { namespace: string; contractName: string } | null {
+        if (!tag || typeof tag !== "string") return null;
+        const separatorIndex = tag.lastIndexOf("-");
+        if (
+            separatorIndex === -1 ||
+            separatorIndex === 0 ||
+            separatorIndex === tag.length - 1
+        ) {
+            return null;
+        }
+        const namespace = tag.slice(0, separatorIndex).trim();
+        const contractName = tag.slice(separatorIndex + 1).trim();
+        if (!namespace || !contractName) return null;
+        return { namespace, contractName };
+    }

22-75: Type-level surface for action typing looks solid.

The normalization and input inference logic is clear and idiomatic.

If you expect empty-input functions to accept zero args and not require an args object, current DojoActionMethod typing achieves that. Confirm downstream usage matches.


421-434: Public API pattern (typed constructor cast) is acceptable.

The as unknown as DojoProviderConstructor trick is a common TS pattern for typed factories; safe given runtime is DojoProviderBase.


107-112: No action needed: starknet.js v6+ supports object-form new Contract({ abi, address, providerOrAccount }). Both occurrences (lines 107–112 and 279–283) are valid.

@MartianGreed MartianGreed force-pushed the feat/types-cairo-option branch from c1531d6 to c649af3 Compare September 25, 2025 18:54
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: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (5)
examples/example-vite-react-sdk/src/useSystemCalls.ts (1)

24-26: Avoid non-null assertion on account; guard before use.

This will throw when wallet is disconnected.

Apply:

-    const generateEntityId = () => {
-        return getEntityIdFromKeys([BigInt(account!.address)]);
-    };
+    const generateEntityId = () => {
+        if (!account?.address) throw new Error("Connect your wallet first.");
+        return getEntityIdFromKeys([BigInt(account.address)]);
+    };
packages/core/src/provider/DojoProvider.ts (4)

1-14: Import CallResult as a type-only import to avoid runtime import errors.

If CallResult is a type (as with other Starknet types), importing it as a value can cause bundling/runtime issues.

Apply this diff:

 import {
     type Account,
     type AccountInterface,
     type AllowArray,
     type ArgsOrCalldata,
     type Call,
-    type CallContractResponse,
-    CallResult,
+    type CallContractResponse,
+    type CallResult,
     Contract,
     type InvokeFunctionResponse,
     RpcProvider,
     shortString,
     type UniversalDetails,
 } from "starknet";

97-116: Make manifest required and validate before using it.

Constructor uses manifest.world.* (including in super()) but the parameter is optional. This can throw at runtime before any guard runs. Make manifest required and validate early.

Apply this change:

- constructor(
-        manifest?: any,
-        url: string = LOCAL_KATANA,
-        logLevel: LogLevel = "none"
-    ) {
-        super(manifest.world.address);
+ constructor(
+        manifest: any,
+        url: string = LOCAL_KATANA,
+        logLevel: LogLevel = "none"
+    ) {
+        if (!manifest?.world?.address || !manifest?.world?.abi) {
+            throw new Error("Invalid manifest: world.address and world.abi are required");
+        }
+        super(manifest.world.address);
         this.provider = new RpcProvider({
             nodeUrl: url,
         });
 
         this.contract = new Contract({
             abi: manifest.world.abi,
             address: this.getWorldAddress(),
             providerOrAccount: this.provider,
         });
         this.manifest = manifest;
         this.logger = new ConsoleLogger({ level: logLevel });
 
         this.initializeActionMethods();
     }

165-175: Fix incorrect cast to Promise in entities().

You await the result and then cast it to Promise<...>. Return the resolved value type instead.

-            ])) as unknown as Promise<Array<Array<bigint>>>;
+            ])) as unknown as Array<Array<bigint>>;

251-256: Do not optional-chain account.execute; validate the account.

Optional-chaining can return undefined, violating the declared return type and silently swallowing errors. Throw if account is absent and call execute directly.

-            return await account?.execute(calls, details);
+            if (!account) {
+                throw new Error("Account is required for execute()");
+            }
+            return await account.execute(calls, details);
🧹 Nitpick comments (12)
etc/architecture/overview.md (1)

7-13: Link packages to their repos/docs for quick navigation.

Adding direct links to each package improves discoverability.

etc/architecture/whats-next.md (3)

5-13: Define migration acceptance criteria per stream.

Add explicit “done” checks (compat tests passing, examples migrated, docs landed) to reduce ambiguity.


23-31: Call out provider swap examples.

A short code sample showing DI of a non-Starknet.js transport will help implementers.


59-63: Add privacy/PII guardrails for observability.

Brief do/don’t list (no wallet addresses in PII logs, sampling, opt-in config) would be useful.

examples/example-vite-react-sdk/src/useSystemCalls.ts (3)

33-36: Guard spawn when no connected account.

Prevents runtime errors and confusing UX.

-    const spawn = async () => {
+    const spawn = async () => {
+        if (!account) {
+            throw new Error("Connect your wallet before spawning.");
+        }

54-56: Remove commented legacy call.

Keeps example clean.

-            // await client.actions.spawn(account!);
             await dojoProvider.spawn(account!);

58-63: Consider bounded waits.

If the predicate never matches, this may hang. Add a timeout/cancel if supported by waitForEntityChange.

examples/example-vite-react-sdk/src/App.tsx (4)

15-15: Standardize import style.

Use consistent extension usage across the example (either with .ts consistently or omit extensions) to avoid module resolution surprises.

-import { dojoProvider } from "./types.ts";
+import { dojoProvider } from "./types";

77-80: Render a readable direction label.

unwrap() likely returns an object; direct render may show “[object Object]”. Extract the variant name.

-                                {moves && moves.last_direction.isSome()
-                                    ? moves.last_direction.unwrap()
-                                    : ""}
+                                {moves && moves.last_direction.isSome()
+                                    ? Object.keys(moves.last_direction.unwrap())[0]
+                                    : ""}

86-115: Avoid recreating enum objects on every render.

Hoist the directions array outside the component or memoize it.


30-47: Optional: Disable queries/buttons when no account.

You can skip the KeysClause and disable controls until account exists to reduce noise and avoid no-op calls.

etc/knowledge-base.md (1)

5-5: Fix link label/path mismatch.

The label says docs/architecture/overview.md but the link points to architecture/overview.md (which resolves to etc/architecture/overview.md). Align the label or link to avoid confusion.

- - Reference diagram: [docs/architecture/overview.md](architecture/overview.md)
+ - Reference diagram: [etc/architecture/overview.md](architecture/overview.md)
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between c1531d6 and c649af3.

📒 Files selected for processing (7)
  • etc/architecture/overview.md (1 hunks)
  • etc/architecture/whats-next.md (1 hunks)
  • etc/knowledge-base.md (1 hunks)
  • examples/example-vite-react-sdk/src/App.tsx (3 hunks)
  • examples/example-vite-react-sdk/src/useSystemCalls.ts (3 hunks)
  • examples/example-vite-react-sdk/tsconfig.app.tsbuildinfo (1 hunks)
  • packages/core/src/provider/DojoProvider.ts (3 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
examples/example-vite-react-sdk/src/App.tsx (2)
packages/sdk/src/web/react/hooks/index.ts (1)
  • useDojoSDK (19-24)
examples/example-vite-react-sdk/src/types.ts (1)
  • dojoProvider (11-14)
examples/example-vite-react-sdk/src/useSystemCalls.ts (2)
packages/sdk/src/web/react/hooks/index.ts (1)
  • useDojoSDK (19-24)
examples/example-vite-react-sdk/src/types.ts (1)
  • dojoProvider (11-14)
packages/core/src/provider/DojoProvider.ts (2)
packages/create-burner/src/connectors/burner.ts (1)
  • account (78-80)
packages/core/src/logger/logger.ts (1)
  • LogLevel (14-14)
⏰ 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 (6)
etc/architecture/overview.md (1)

1-72: Solid architecture overview.

Clear layering, responsibilities, and the Mermaid flow are coherent.

examples/example-vite-react-sdk/src/useSystemCalls.ts (1)

70-72: Confirm confirmTransaction semantics on failure.

Currently called in finally even after revertOptimisticUpdate. Ensure this represents “settled” and not “successful”.

packages/core/src/provider/DojoProvider.ts (4)

114-116: Good: action methods initialized at construction.

Eager initialization ensures action methods are available immediately.


329-404: Harden dynamic action generation (defensive checks, duplicate detection, warnings).

Currently duplicates are silently skipped and ABI/tag shapes are assumed. Add guards and warnings to prevent footguns and aid debugging.

Apply this diff:

     private initializeActionMethods(): void {
         if (!this.manifest?.contracts) {
             return;
         }
 
         const host = this as unknown as Record<
             string,
             ActionMethodImplementation
         >;
 
+        const systemRegistry = new Set<string>();
+
         for (const contract of this.manifest.contracts as Array<any>) {
             if (
                 !contract?.systems?.length ||
                 typeof contract.tag !== "string"
             ) {
                 continue;
             }
 
             const names = this.parseContractTag(contract.tag);
             if (!names) {
+                this.logger.warn(`Invalid contract tag format: ${contract.tag}`);
                 continue;
             }
 
             const abiItems = Array.isArray(contract.abi) ? contract.abi : [];
+            if (abiItems.length === 0) {
+                this.logger.warn(`Empty ABI for contract: ${contract.tag}`);
+                continue;
+            }
 
             for (const systemName of contract.systems as Array<string>) {
-                if (systemName in host) {
+                if (systemRegistry.has(systemName) || systemName in host) {
+                    this.logger.warn(
+                        `Duplicate or colliding system name "${systemName}" in contract "${contract.tag}". Skipping.`
+                    );
                     continue;
                 }
+                systemRegistry.add(systemName);
 
                 const interfaceAbi = abiItems.find(
                     (item: any) =>
                         item?.type === "interface" &&
-                        item?.items.find(
+                        item?.items?.find(
                             (i: any) =>
                                 i?.type === "function" && i?.name === systemName
                         )
                 );
 
                 if (!interfaceAbi) {
+                    this.logger.warn(
+                        `No interface found for system "${systemName}" in contract "${contract.tag}"`
+                    );
                     continue;
                 }
-                const functionAbi = interfaceAbi.items.find(
+                if (!Array.isArray(interfaceAbi.items)) {
+                    this.logger.warn(
+                        `Malformed ABI: "items" is not an array for contract "${contract.tag}"`
+                    );
+                    continue;
+                }
+                const functionAbi = interfaceAbi.items.find(
                     (i: any) => i?.type === "function" && i?.name === systemName
                 );
                 if (!functionAbi) {
                     continue;
                 }
 
                 const expectsArgs = Array.isArray(functionAbi.inputs)
                     ? functionAbi.inputs.length > 0
                     : false;
 
                 host[systemName] = async (
                     account: Account | AccountInterface,
                     args?: Record<string, unknown>
                 ) => {
                     if (expectsArgs && args === undefined) {
                         throw new Error(
                             `Missing arguments for action "${systemName}"`
                         );
                     }
+                    if (!expectsArgs && args !== undefined) {
+                        this.logger.warn(
+                            `Unexpected arguments provided for action "${systemName}"`
+                        );
+                    }
 
                     return this.execute(
                         account,
                         {
                             contractName: names.contractName,
                             entrypoint: systemName,
                             calldata: (args ?? []) as unknown as ArgsOrCalldata,
                         },
                         names.namespace
                     );
                 };
             }
         }
     }

406-418: Validate and sanitize contract tag parsing.

Guard against malformed tags (missing/edge separators, empty parts).

-    private parseContractTag(
-        tag: string
-    ): { namespace: string; contractName: string } | null {
-        const separatorIndex = tag.lastIndexOf("-");
-        if (separatorIndex === -1) {
-            return null;
-        }
-
-        return {
-            namespace: tag.slice(0, separatorIndex),
-            contractName: tag.slice(separatorIndex + 1),
-        };
-    }
+    private parseContractTag(
+        tag: string
+    ): { namespace: string; contractName: string } | null {
+        if (!tag || typeof tag !== "string") {
+            return null;
+        }
+        const separatorIndex = tag.lastIndexOf("-");
+        if (
+            separatorIndex === -1 ||
+            separatorIndex === 0 ||
+            separatorIndex === tag.length - 1
+        ) {
+            return null;
+        }
+        const namespace = tag.slice(0, separatorIndex).trim();
+        const contractName = tag.slice(separatorIndex + 1).trim();
+        if (!namespace || !contractName) {
+            return null;
+        }
+        return { namespace, contractName };
+    }

421-434: Public constructor alias looks good.

Type surface (DojoProviderInstance and DojoActionMethodMap) aligns with runtime dynamic method injection.

Comment on lines 119 to 123
onClick={async () => {
await client.actions.move(
account!,
direction
);
await dojoProvider.move(account!, {
direction: direction,
});
}}
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Guard move when wallet is disconnected.

Prevents crashes from account! and improves UX.

-                                    onClick={async () => {
-                                        await dojoProvider.move(account!, {
-                                            direction: direction,
-                                        });
-                                    }}
+                                    onClick={async () => {
+                                        if (!account) {
+                                            console.warn("Connect wallet to move.");
+                                            return;
+                                        }
+                                        await dojoProvider.move(account, { direction });
+                                    }}
📝 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
onClick={async () => {
await client.actions.move(
account!,
direction
);
await dojoProvider.move(account!, {
direction: direction,
});
}}
onClick={async () => {
if (!account) {
console.warn("Connect wallet to move.");
return;
}
await dojoProvider.move(account, { direction });
}}
🤖 Prompt for AI Agents
In examples/example-vite-react-sdk/src/App.tsx around lines 119 to 123, the
onClick handler calls dojoProvider.move(account!, ...) using a non-null
assertion which will crash when wallet is disconnected; guard the handler by
checking that account is defined before calling move (e.g. early return if
!account), remove the non-null assertion, and optionally disable the button or
show a connect-wallet prompt when account is falsy to improve UX.

@@ -1 +1 @@
{"root":["./src/app.tsx","./src/events.tsx","./src/historical-events.tsx","./src/main.tsx","./src/starknet-provider.tsx","./src/usesystemcalls.ts","./src/vite-env.d.ts","./src/wallet-account.tsx","./src/typescript/contracts.gen.ts","./src/typescript/models.gen.ts"],"version":"5.9.2"} No newline at end of file
{"root":["./src/app.tsx","./src/dojo_starter_dev.ts","./src/events.tsx","./src/historical-events.tsx","./src/main.tsx","./src/starknet-provider.tsx","./src/types.ts","./src/usesystemcalls.ts","./src/vite-env.d.ts","./src/wallet-account.tsx","./src/typescript/contracts.gen.ts","./src/typescript/models.gen.ts"],"version":"5.9.2"} No newline at end of file
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion | 🟠 Major

🧩 Analysis chain

Do not commit .tsbuildinfo artifacts.

These are transient, platform-specific, and cause noisy diffs (also shows case-mismatched paths). Remove from VCS and ignore going forward.

Apply:

-{"root":["./src/app.tsx","./src/dojo_starter_dev.ts","./src/events.tsx","./src/historical-events.tsx","./src/main.tsx","./src/starknet-provider.tsx","./src/types.ts","./src/usesystemcalls.ts","./src/vite-env.d.ts","./src/wallet-account.tsx","./src/typescript/contracts.gen.ts","./src/typescript/models.gen.ts"],"version":"5.9.2"}

Add to .gitignore (if not present):

+.tsbuildinfo
+**/tsconfig*.tsbuildinfo
+**/dist/
+**/build/
+**/.turbo/
+**/.next/

Run to verify no .tsbuildinfo remain:


🏁 Script executed:

#!/bin/bash
fd -u -t f -a --glob "*.tsbuildinfo"

Length of output: 378


Remove committed .tsbuildinfo files and update .gitignore

  • Untrack existing .tsbuildinfo artifacts:
    git rm --cached examples/example-vite-phaser-sdk/tsconfig.tsbuildinfo
    git rm --cached \
      examples/example-vite-react-sdk/tsconfig.app.tsbuildinfo \
      examples/example-vite-react-sdk/tsconfig.node.tsbuildinfo \
      examples/example-vite-react-sql/tsconfig.app.tsbuildinfo \
      examples/example-vite-react-sql/tsconfig.node.tsbuildinfo
    
  • Update .gitignore:
    # ignore TypeScript incremental build info
    *.tsbuildinfo
    **/tsconfig*.tsbuildinfo
    
    # ignore common build directories
    **/dist/
    **/build/
    **/.turbo/
    **/.next/
  • Verify no remaining artifacts:
    fd -u -t f -a --glob "*.tsbuildinfo"
🤖 Prompt for AI Agents
In examples/example-vite-react-sdk/tsconfig.app.tsbuildinfo around line 1, a
TypeScript incremental build artifact (.tsbuildinfo) was accidentally committed;
remove the tracked artifact (git rm --cached
examples/example-vite-react-sdk/tsconfig.app.tsbuildinfo and any other committed
.tsbuildinfo files listed), add ignore rules to .gitignore (e.g., *.tsbuildinfo
and **/tsconfig*.tsbuildinfo plus common build dirs), commit the .gitignore
change, and verify no remaining tracked artifacts (e.g., fd -u -t f -a --glob
"*.tsbuildinfo") before committing the cleanup.

@MartianGreed MartianGreed merged commit 68a5b38 into main Sep 25, 2025
8 of 10 checks passed
@MartianGreed MartianGreed deleted the feat/types-cairo-option branch September 25, 2025 19:11
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.

1 participant