Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: fix react-sdk example + predeployed connector + historical events #367

Merged
merged 3 commits into from
Jan 6, 2025

Conversation

MartianGreed
Copy link
Collaborator

@MartianGreed MartianGreed commented Jan 4, 2025

Requires dojoengine/dojo-starter#98 to be merged

Summary by CodeRabbit

  • New Features

    • Added Starknet wallet integration with support for multiple wallet connectors.
    • Introduced historical events tracking functionality.
    • Enhanced SDK with improved event message handling.
    • Added a new WalletAccount component for wallet connection management.
    • Introduced a new StarknetProvider component for interacting with the StarkNet blockchain.
    • Added a new HistoricalEvents component for displaying historical events.
  • Dependencies

    • Added @dojoengine/predeployed-connector.
    • Added @starknet-react/chains.
    • Added @starknet-react/core.
  • Improvements

    • Refined type system for game state management.
    • Updated contract interaction methods.
    • Streamlined account management and connection processes.
    • Enhanced handling of historical event data and parsing logic.
    • Improved flexibility in event message handling to support multiple result formats.

Copy link

coderabbitai bot commented Jan 4, 2025

Walkthrough

This pull request introduces significant updates to the Dojo React SDK example project, focusing on enhancing wallet connectivity, refining event handling, and updating type definitions. Key changes include the addition of new dependencies for StarkNet and Dojo, the introduction of new components such as StarknetProvider and HistoricalEvents, and the replacement of Schema with SchemaType across multiple files. Additionally, several files have been deleted as part of the refactoring process.

Changes

File Change Summary
examples/example-vite-react-sdk/package.json Added dependencies: @dojoengine/predeployed-connector, @starknet-react/chains, @starknet-react/core
examples/example-vite-react-sdk/src/App.tsx, examples/example-vite-react-sdk/src/main.tsx, examples/example-vite-react-sdk/src/useModel.tsx, examples/example-vite-react-sdk/src/useSystemCalls.ts Replaced Schema imports with SchemaType, updated type references
examples/example-vite-react-sdk/src/typescript/models.gen.ts Introduced comprehensive type definitions, new interfaces, enums, and SchemaType
New Files Added examples/example-vite-react-sdk/src/starknet-provider.tsx, examples/example-vite-react-sdk/src/historical-events.tsx, examples/example-vite-react-sdk/src/wallet-account.tsx for enhanced wallet and event management
Deleted Files Removed examples/example-vite-react-sdk/src/bindings.ts, examples/example-vite-react-sdk/src/contracts.gen.ts for refactoring
SDK Package Updated getEventMessages, subscribeEventQuery, and related type definitions to support historical events

Sequence Diagram

sequenceDiagram
    participant User
    participant WalletAccount
    participant StarknetProvider
    participant DojoContextProvider
    participant App

    User->>WalletAccount: Connect Wallet
    WalletAccount->>StarknetProvider: Establish Connection
    StarknetProvider->>DojoContextProvider: Initialize Context
    DojoContextProvider->>App: Render Application
    App->>HistoricalEvents: Fetch Events
    HistoricalEvents-->>App: Display Event History
Loading

Possibly related PRs

  • [fix: typedoc ci #365] Changes to the setupWorld function, potentially related to client setup and utilization.

Poem

🐰 Hop, hop, through the code we go,
Schemas dance and connectors flow
StarkNet's embrace, a rabbit's delight
Refactoring magic takes flight!
New types bloom like spring's first rose 🌹


📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between ffc30b4 and 8dd2ca0.

📒 Files selected for processing (1)
  • worlds/dojo-starter (1 hunks)
✅ Files skipped from review due to trivial changes (1)
  • worlds/dojo-starter
⏰ Context from checks skipped due to timeout of 90000ms (1)
  • GitHub Check: check

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

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

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

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

CodeRabbit Commands (Invoked using PR comments)

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

Other keywords and placeholders

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

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

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

@MartianGreed MartianGreed force-pushed the feat/update-react-sdk-example branch from 0992803 to b87c5d2 Compare January 4, 2025 17:00
@MartianGreed MartianGreed force-pushed the feat/update-react-sdk-example branch from b87c5d2 to 50cc19e Compare January 4, 2025 17:02
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.

Caution

Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.

Actionable comments posted: 6

🧹 Nitpick comments (25)
examples/example-vite-react-sdk/src/typescript/contracts.gen.ts (3)

13-19: Consider extracting contract name and entrypoint constants.
You're repeating string literals ["actions", "move"]. Extracting them as constants promotes clarity, maintainability, and reduces the risk of typos.

-            contractName: "actions",
-            entrypoint: "move",
+            contractName: ACTIONS_CONTRACT,
+            entrypoint: MOVE_ENTRYPOINT,

21-35: Evaluate error handling strategy.
The try/catch block logs errors to the console before rethrowing them. For production code, consider a more robust logging approach or enhanced error handling strategy (e.g., using a logging library or refined error messages).


37-43: Generalize spawn's calldata if parameters will expand.
Currently, actions_spawn uses an empty calldata. If future changes require spawn parameters, consider a consistent approach to constructing an optional calldata object.

examples/example-vite-react-sdk/src/DojoContext.tsx (2)

61-61: Avoid hardcoding chain ID if environment-based.
You’re passing "1" as the chain ID to the Account constructor. If this chain ID is environment specific, consider retrieving it from your config or environment variables.


70-70: Consider memoizing calls to setupWorld.
If dojoProvider changes, you may need a fresh client. Currently, setupWorld(dojoProvider) is invoked on every render. Using a memo hook can improve performance and consistency.

packages/sdk/src/getEventMessages.ts (2)

51-51: Minor naming consideration.
isHistorical is clear, but if you foresee more modes in the future, consider a more descriptive name like useHistoricalData.


103-106: Consistent final return.
Returning parsed entities in the same function that streams them to the callback is logical. Consider adding test coverage to confirm correct results for both historical and real-time scenarios.

packages/sdk/src/parseHistoricalEvents.ts (1)

22-51: Event-to-entity conversion.
Sorting keys numerically and reconstructing model names is a neat approach. However, consider potential edge cases, such as partial keys or no numeric suffix. Add a safety check for extremely large numeric parts to avoid performance issues.

packages/sdk/src/subscribeEventQuery.ts (2)

37-37: Callback type updated.
Accepting both single and array-based data ensures better flexibility. Document usage in the subscription docs for clarity.


43-43: Boolean casting.
Using !!historical is concise. If more states are required later, a different approach might be needed.

packages/sdk/src/__example__/index.ts (1)

118-136: Subscription pattern shift.
Grouping query and callback into one object is clearer, but ensure that new devs understand how to provide their own custom callbacks and queries.

packages/sdk/src/types.ts (1)

390-390: Unify the callback’s handling of single vs. multiple data results.

By allowing data?: StandardizedQueryResult<T> | StandardizedQueryResult<T>[], consumers of the SDK must differentiate between single and multiple query outcomes. Consider normalizing the return to a consistent structure (e.g., always an array) to reduce complexity.

packages/sdk/src/__tests__/parseEntities.test.ts (2)

225-225: Avoid suppressing TypeScript errors with @ts-ignore.

The // @ts-ignore can be undefined comment disables valuable type checking. Consider using optional chaining (e.g., ?.) or runtime checks to ensure robust handling of possibly undefined properties instead of ignoring type errors.


280-280: Use safer null/undefined handling mechanisms.

Again, // @ts-ignore can be undefined circumvents TypeScript warnings. Relying on @ts-ignore can mask potential runtime issues.

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

19-20: Consider removing unused imports
useConnect does not appear to be used in this file. If confirmed unnecessary, removing it will keep the code tidy.


103-111: Similar approach for fetchEntities
Both the subscribe and fetch routines rely on the same query pattern. Consider refactoring to a shared utility if the queries expand or become more complex.


203-206: Non-null assertion for account
Using account! can lead to runtime errors if account is unexpectedly undefined when a user clicks these controls. Consider a safer guard check or a disabled button state if no account is available.

- onClick={async () => await client.actions.move(account!, direction)}
+ onClick={async () => {
+   if (!account) return;
+   await client.actions.move(account, direction);
+ }}
packages/predeployed-connector/src/index.ts (3)

42-42: ConnectorIcons type commented out
If no longer needed, you may safely remove it to reduce clutter.


82-85: Attaching to global window
Consider using a more controlled approach rather than extending window directly. Potential naming collisions can arise if multiple connectors do the same.


63-64: Centralized error handling
Many thrown errors share the same structure. Refactor repeated throw patterns into a helper function to reduce duplication.

- throw {
-    code: 63,
-    message: "An unexpected error occurred",
-    data: "wallet_switchStarknetChain not implemented",
- } as Errors.UNEXPECTED_ERROR;
+ function notImplementedError(feature: string): Errors.UNEXPECTED_ERROR {
+    return {
+      code: 63,
+      message: "An unexpected error occurred",
+      data: feature + " not implemented",
+    };
+ }
+ ...
+ throw notImplementedError("wallet_switchStarknetChain");
examples/example-vite-react-sdk/src/wallet-account.tsx (3)

17-26: Consider handling ongoing connection attempts more robustly.

When multiple connectors (wallets) are being connected in quick succession, setting pendingConnectorId(undefined) right after an attempt might unset the pending state prematurely if a second attempt hasn’t finished. You might refine the state transitions or cancel an in-progress connection when initiating a new one.


23-23: Replace console logging with proper error handling.

Relying on console.error might be insufficient in a production setting. Consider providing user feedback (e.g., via toast notifications) or performing more structured logging for troubleshooting.


49-66: Disable wallet buttons that are already connecting.

Consider disabling all wallet buttons except the one that was just clicked when initiating a connection. Attempting multiple connections simultaneously may cause conflicts or unexpected states in the underlying wallet APIs.

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

87-101: Improve detection of empty events.

Before rendering events, consider handling scenarios where event.models?.dojo_starter?.Moved is missing or partially undefined. This prevents unexpected UI states if data is incomplete.

examples/example-vite-react-sdk/src/typescript/models.gen.ts (1)

1-131: Modularize large type definitions.

You’ve introduced a comprehensive set of interfaces and enums in a single file. Consider splitting them by domain or grouping them into smaller modules (e.g., “moves.ts”, “position.ts”) to enhance maintainability and clarity.

🛑 Comments failed to post (6)
packages/sdk/src/getEventMessages.ts (1)

49-49: 🛠️ Refactor suggestion

Return type mismatch in jsdoc.
The function docstring states a single-type return, but the code now returns either a singular or array result. Please update the docstring to match the new return type.

- * @returns {Promise<StandardizedQueryResult<T>>} - A promise that resolves to the standardized query result.
+ * @returns {Promise<StandardizedQueryResult<T> | StandardizedQueryResult<T>[]>} - A promise that resolves to one or more standardized query results.

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

packages/sdk/src/__tests__/parseEntities.test.ts (1)

338-338: 🛠️ Refactor suggestion

Improve type-safety for possibly undefined enumerations.

Like the other test segments, consider using optional chaining (?.) or a type guard rather than suppressing TypeScript checks with @ts-ignore.

Could you confirm if you want me to submit an updated version of the test that uses optional chaining or explicit checks?

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

10-15: 🛠️ Refactor suggestion

Asynchronous assignment of connectors
Assigning pa in the .then() callback means pa is initially empty. If the provider renders immediately, connectors are missing. Consider storing the result in React state or awaiting it before rendering.

- predeployedAccounts({ ... }).then((p) => (pa = p));
- ...
- <StarknetConfig connectors={pa} ... >
+ const [predeployedConnectors, setPredeployedConnectors] = useState<PredeployedAccountsConnector[]>([]);
+ useEffect(() => {
+   predeployedAccounts({ ... }).then((p) => {
+     setPredeployedConnectors(p);
+   });
+ }, []);
+ ...
+ <StarknetConfig connectors={predeployedConnectors} ... >

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

🧰 Tools
🪛 Biome (1.9.4)

[error] 15-15: The assignment should not be in an expression.

The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.

(lint/suspicious/noAssignInExpressions)

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

20-20: ⚠️ Potential issue

Avoid non-null assertions for account.

Using account! can mask potential null/undefined states and lead to runtime errors if a user has not connected a wallet. Consider a guard clause or a fallback to ensure safer handling:

- return getEntityIdFromKeys([BigInt(account!.address)]);
+ if (!account) {
+   throw new Error("No account found. Please connect your wallet.");
+ }
+ return getEntityIdFromKeys([BigInt(account.address)]);

Also applies to: 27-27, 56-56

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

39-69: 🛠️ Refactor suggestion

Unsubscribe on component unmount or account changes.

You set the “unsubscribe” callback after subscribing, but you only call unsubscribe() if an error occurs. To avoid potential memory leaks and to gracefully handle account switching, consider returning a cleanup function from your useEffect:

 useEffect(() => {
   async function subscribeHistoricalEvent(account: AccountInterface) {
     // ...
   }

   if (account) {
     subscribeHistoricalEvent(account);
   }

+  return () => {
+    if (unsubscribe) {
+      unsubscribe();
+    }
+  };
 }, [account, setEvents]);
📝 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.

    useEffect(() => {
        async function subscribeHistoricalEvent(account: AccountInterface) {
            try {
                const s = await sdk.subscribeEventQuery({
                    // query: {
                    //   event_messages_historical: {
                    //     Moved: {
                    //       $: { where: { player: { $eq: addAddressPadding(account.address) } } }
                    //     }
                    //   }
                    // },
                    query: { entityIds: [addAddressPadding(account.address)] },
                    callback: (resp, error) => {
                        console.log(resp, error);
                    },
                    historical: true,
                });
                setSubscription(s);
            } catch (error) {
                setEvents([]);
                if (unsubscribe) {
                    unsubscribe();
                }
                console.error(error);
            }
        }

        if (account) {
            subscribeHistoricalEvent(account);
        }

        return () => {
            if (unsubscribe) {
                unsubscribe();
            }
        };
    }, [account, setEvents]);
packages/sdk/src/convertQuerytoClause.ts (1)

22-31: 💡 Codebase verification

🛠️ Refactor suggestion

Early return for entityIds namespace prevents composite clauses

The review comment is correct. Looking at the implementation in convertQuerytoClause.ts, there's a clear issue:

for (const [namespace, models] of Object.entries(query)) {
    if (namespace === "entityIds") {
        return {  // <-- Early return here
            Keys: {
                keys: [...models],
                pattern_matching: "VariableLen",
                models: [],
            },
        };
    }
    // ... rest of the namespaces are never processed if entityIds exists
}

The early return statement when processing the entityIds namespace prevents the function from processing any other namespaces in the query object. This means if a query contains both entityIds and other namespace conditions, only the entityIds clause will be included in the result, and all other conditions will be ignored.

The function should be refactored to:

  1. Process the entityIds clause without returning immediately
  2. Add it to the clauses array
  3. Continue processing other namespaces
  4. Finally combine all clauses using the composite OR operator
🔗 Analysis chain

Consider returning composite clauses when combining entityIds with other namespaces.

Currently, the function returns immediately when namespace === "entityIds". If the query object includes other namespaces alongside entityIds, those additional clauses will never be processed. Consider refactoring to either merge a clause for entityIds into the overall composite or ensure users are aware that specifying entityIds discards other mappings in the same query object.

Would you like a script to scan references to convertQueryToClause calls across the codebase to confirm whether multi-namespace queries are being used?


Let me check the test file to see if there are any test cases combining entityIds with other namespaces.


Let me check the implementation of convertQueryToClause to understand how it handles multiple namespaces.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Searching for references to convertQueryToClause in the entire repository
rg -A 3 'convertQueryToClause'

Length of output: 3875


Script:

#!/bin/bash
# Check test cases in convertQueryToClause.test.ts
cat packages/sdk/src/__tests__/convertQueryToClause.test.ts

Length of output: 7306


Script:

#!/bin/bash
# Check the implementation of convertQueryToClause
cat packages/sdk/src/convertQuerytoClause.ts

Length of output: 11661

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

🔭 Outside diff range comments (3)
examples/example-vite-react-sdk/src/App.tsx (1)

Line range hint 54-93: Enhance subscription error handling and cleanup.

The subscription logic could be improved with better error handling and cleanup patterns.

 const subscribe = async (account: AccountInterface) => {
+    let subscriptionRef: { cancel: () => void } | undefined;
     try {
         const subscription = await sdk.subscribeEntityQuery({
             // ... subscription config
         });
-        unsubscribe = () => subscription.cancel();
+        subscriptionRef = subscription;
     } catch (error) {
-        console.error("Error setting up entity sync:", error);
+        console.error("Failed to set up entity subscription:", error);
+        throw error;
+    } finally {
+        unsubscribe = () => subscriptionRef?.cancel();
     }
 };
packages/predeployed-connector/src/index.ts (2)

Line range hint 89-186: Refactor error handling in request method.

The error handling could be more consistent and reusable.

Create a helper function for error creation:

+    private createError(message: string): Errors.UNEXPECTED_ERROR {
+        return {
+            code: 63,
+            message: "An unexpected error occurred",
+            data: message,
+        } as Errors.UNEXPECTED_ERROR;
+    }

     request: RequestFn = async (call) => {
         switch (call.type) {
             case "wallet_watchAsset":
-                throw {
-                    code: 63,
-                    message: "An unexpected error occurred",
-                    data: "wallet_watchAsset not implemented",
-                } as Errors.UNEXPECTED_ERROR;
+                throw this.createError("wallet_watchAsset not implemented");

Line range hint 209-256: Improve type safety in predeployedAccounts function.

The function could benefit from better error handling and type safety.

 export async function predeployedAccounts(
     options: PredeployedAccountsConnectorOptions
 ): Promise<PredeployedAccountsConnector[]> {
+    if (!options.rpc || !options.id || !options.name) {
+        throw new Error('Missing required options');
+    }

     async function getAccounts(
         provider: ProviderInterface,
         id: string,
         name: string
     ): Promise<PredeployedAccount[]> {
+        if (!provider.channel) {
+            throw new Error('Provider channel not available');
+        }
         // @ts-ignore
         const res = await provider.channel.getPredeployedAccounts();
+        if (!Array.isArray(res)) {
+            throw new Error('Invalid response from getPredeployedAccounts');
+        }
         return res.map((a: PredeployedAccountRpcResponse, idx: number) => ({
🧹 Nitpick comments (14)
examples/example-vite-react-sdk/src/starknet-provider.tsx (1)

1-8: Consider environment-specific chain configuration

The component imports and uses mainnet by default, which might not be appropriate for an example project that likely needs to work with test networks.

Consider making the chain configurable:

-import { Chain, mainnet } from "@starknet-react/chains";
+import { Chain, mainnet, testnet } from "@starknet-react/chains";
+
+// Allow chain configuration through environment or props
+const defaultChain = process.env.REACT_APP_NETWORK === 'mainnet' ? mainnet : testnet;

Also, consider adding type safety for the dojoConfig import:

// dojoConfig.ts
export interface DojoConfig {
  rpcUrl: string;
  // ... other config properties
}
packages/sdk/src/parseHistoricalEvents.ts (3)

31-34: Add input validation and improve number parsing.

The regex pattern assumes a specific format and the parseInt lacks a radix parameter.

Consider these improvements:

 const getLastNumber = (str: string) => {
     const match = str.match(/-(\d+)$/);
-    return match ? parseInt(match[1]) : 0;
+    return match ? parseInt(match[1], 10) : 0;
 };

Also, consider adding validation for the expected format:

if (!str.includes('-')) {
    console.warn(`Unexpected key format: ${str}`);
}

47-47: Optimize array updates for better performance.

Using spread operator for array updates in a loop can be inefficient for large datasets.

Consider using array push instead:

-events = [...events, { [entityId]: { [modelName]: modelData } }];
+events.push({ [entityId]: { [modelName]: modelData } });

51-51: Consider adding error handling for parseEntities.

The map operation assumes parseEntities will always succeed for each event.

Consider wrapping each parsing operation in try-catch:

-return events.map((e) => parseEntities<T>(e, options));
+return events.map((e) => {
+    try {
+        return parseEntities<T>(e, options);
+    } catch (error) {
+        console.error(`Failed to parse event:`, error);
+        throw error;
+    }
+});
packages/sdk/src/getEventMessages.ts (1)

64-64: Document the implications of dont_include_hashed_keys and historical parameter.

The changes to these parameters might affect the behavior of the function.

Consider adding comments explaining:

  1. Why dont_include_hashed_keys is set to true
  2. How the historical parameter affects the returned data format

Also applies to: 71-71

examples/example-vite-react-sdk/src/typescript/models.gen.ts (3)

5-5: Consider a readonly array for fieldOrder.

WithFieldOrder is currently using a mutable array type. If you want to prevent accidental modifications at runtime, converting fieldOrder to a readonly array (e.g., readonly string[]) might be safer.

-type WithFieldOrder<T> = T & { fieldOrder: string[] };
+type WithFieldOrder<T> = T & { fieldOrder: readonly string[] };

7-11: Evaluate the player field type for DirectionsAvailable.

player is typed as string, which might be sufficient if this is simply an address or username. If you expect more complex identification (e.g., strongly typed addresses or IDs), consider migrating to a more specialized type to improve correctness and maintainability.


82-130: Double-check default values in the schema.

Defaulting fields like directions to [Direction.Left], player to an empty string, or last_direction to None is reasonable if it accurately represents initial states. Otherwise, consider selecting a more generic placeholder to avoid potential confusion during testing or initialization.

examples/example-vite-react-sdk/src/historical-events.tsx (2)

7-11: Consider improving subscription cleanup.

The unsubscribe state management could be simplified by using a ref instead of state, as it's not used for rendering.

-    const [unsubscribe, setSubscription] = useState(null);
+    const unsubscribeRef = useRef(null);

12-37: Remove commented query structure.

The commented query structure should either be removed or implemented if it represents a better approach. Keeping commented code can lead to confusion.

-                    // query: {
-                    //   event_messages_historical: {
-                    //     Moved: {
-                    //       $: { where: { player: { $eq: addAddressPadding(account.address) } } }
-                    //     }
-                    //   }
-                    // },
examples/example-vite-react-sdk/src/App.tsx (3)

44-49: Improve entityId calculation with better type safety.

The entityId calculation could be made more robust with proper type checking.

 const entityId = useMemo(() => {
-    if (account) {
+    if (account?.address) {
         return getEntityIdFromKeys([BigInt(account.address)]);
     }
     return BigInt(0);
-}, [account]);
+}, [account?.address]);

168-170: Use optional chaining as suggested by static analysis.

The code can be simplified using optional chaining.

-{moves && moves.last_direction.isSome()
-    ? moves.last_direction.unwrap()
-    : ""}
+{moves?.last_direction?.isSome() ? moves.last_direction.unwrap() : ""}
🧰 Tools
🪛 Biome (1.9.4)

[error] 168-168: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)


250-253: Improve null handling in lastDirection calculation.

The lastDirection calculation could be more concise with optional chaining.

-const lastDirection =
-    moves?.last_direction.isSome()
-        ? moves.last_direction.unwrap()
-        : "N/A";
+const lastDirection = moves?.last_direction?.isSome() 
+    ? moves.last_direction.unwrap() 
+    : "N/A";
packages/predeployed-connector/src/index.ts (1)

47-56: Add return type annotations for better type safety.

The available method should have an explicit return type.

-    available(): boolean {
+    available(): boolean {
         return !!this.options.account;
     }
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 0992803 and 50cc19e.

⛔ Files ignored due to path filters (1)
  • pnpm-lock.yaml is excluded by !**/pnpm-lock.yaml
📒 Files selected for processing (21)
  • examples/example-vite-react-sdk/package.json (1 hunks)
  • examples/example-vite-react-sdk/src/App.tsx (9 hunks)
  • examples/example-vite-react-sdk/src/DojoContext.tsx (4 hunks)
  • examples/example-vite-react-sdk/src/bindings.ts (0 hunks)
  • examples/example-vite-react-sdk/src/contracts.gen.ts (0 hunks)
  • examples/example-vite-react-sdk/src/historical-events.tsx (1 hunks)
  • examples/example-vite-react-sdk/src/main.tsx (3 hunks)
  • examples/example-vite-react-sdk/src/starknet-provider.tsx (1 hunks)
  • examples/example-vite-react-sdk/src/typescript/contracts.gen.ts (1 hunks)
  • examples/example-vite-react-sdk/src/typescript/models.gen.ts (1 hunks)
  • examples/example-vite-react-sdk/src/useModel.tsx (2 hunks)
  • examples/example-vite-react-sdk/src/useSystemCalls.ts (3 hunks)
  • examples/example-vite-react-sdk/src/wallet-account.tsx (1 hunks)
  • packages/predeployed-connector/src/index.ts (3 hunks)
  • packages/sdk/src/__tests__/parseEntities.test.ts (3 hunks)
  • packages/sdk/src/convertQuerytoClause.ts (1 hunks)
  • packages/sdk/src/getEventMessages.ts (6 hunks)
  • packages/sdk/src/index.ts (1 hunks)
  • packages/sdk/src/parseHistoricalEvents.ts (1 hunks)
  • packages/sdk/src/subscribeEventQuery.ts (2 hunks)
  • packages/sdk/src/types.ts (3 hunks)
💤 Files with no reviewable changes (2)
  • examples/example-vite-react-sdk/src/bindings.ts
  • examples/example-vite-react-sdk/src/contracts.gen.ts
🚧 Files skipped from review as they are similar to previous changes (12)
  • examples/example-vite-react-sdk/src/typescript/contracts.gen.ts
  • examples/example-vite-react-sdk/src/useSystemCalls.ts
  • examples/example-vite-react-sdk/src/useModel.tsx
  • packages/sdk/src/tests/parseEntities.test.ts
  • packages/sdk/src/index.ts
  • examples/example-vite-react-sdk/package.json
  • examples/example-vite-react-sdk/src/DojoContext.tsx
  • packages/sdk/src/convertQuerytoClause.ts
  • examples/example-vite-react-sdk/src/main.tsx
  • examples/example-vite-react-sdk/src/wallet-account.tsx
  • packages/sdk/src/subscribeEventQuery.ts
  • packages/sdk/src/types.ts
🧰 Additional context used
🪛 Biome (1.9.4)
examples/example-vite-react-sdk/src/starknet-provider.tsx

[error] 15-15: The assignment should not be in an expression.

The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.

(lint/suspicious/noAssignInExpressions)

examples/example-vite-react-sdk/src/App.tsx

[error] 168-168: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)

🔇 Additional comments (8)
packages/sdk/src/parseHistoricalEvents.ts (1)

1-21: LGTM! Well-documented function signature with comprehensive JSDoc.

The type imports and function signature are well-structured. The JSDoc provides clear documentation of parameters, return type, and includes a helpful example.

packages/sdk/src/getEventMessages.ts (1)

40-40: LGTM! Type signature correctly updated for historical events support.

The callback data type has been properly updated to handle both single and array results.

examples/example-vite-react-sdk/src/typescript/models.gen.ts (4)

18-24: Looks good for handling movement logic.

The Moves interface provides sufficient clarity for tracking moves, directions, and a boolean to indicate movement capability. The usage of BigNumberish for remaining aligns well with typical StarkNet approaches. Overall, no further concerns here.


50-60: Validate defaults for direction when a move is made.

In Moved and MovedValue, you are using Direction as a required field. Confirm that you never need a "no-op" scenario or an optional direction. If so, consider wrapping it with CairoOption<Direction> to handle missing directions systematically.


131-142: The model mapping appears consistent.

ModelsMapping neatly ties together your model definitions. This improves readability and debugging by providing clear, stable references to each type. No issues detected.


1-3: Consider verifying the "@dojoengine/sdk" version compatibility.

You have introduced ISchemaType from @dojoengine/sdk. Ensure that your newly added types align with the current version of the SDK in use. If the SDK version is outdated or has breaking changes, it may cause runtime or type mismatches.

Run the following script to check the installed version of @dojoengine/sdk:

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

1-5: LGTM! Imports are well organized.

All necessary dependencies are properly imported.

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

1-26: LGTM! Well-structured imports and store setup.

The imports are comprehensive and the store is properly typed with SchemaType.

Comment on lines +10 to +15
let pa: PredeployedAccountsConnector[] = [];
predeployedAccounts({
rpc: dojoConfig.rpcUrl as string,
id: "katana",
name: "Katana",
}).then((p) => (pa = p));
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Critical: Move predeployed accounts setup into component

The current implementation has several issues:

  1. Uses global mutable state which can cause race conditions
  2. Async initialization happens outside React's lifecycle
  3. Components might render before accounts are loaded

Refactor to use React hooks for proper lifecycle management:

-let pa: PredeployedAccountsConnector[] = [];
-predeployedAccounts({
-    rpc: dojoConfig.rpcUrl as string,
-    id: "katana",
-    name: "Katana",
-}).then((p) => (pa = p));

 export default function StarknetProvider({ children }: PropsWithChildren) {
+    const [connectors, setConnectors] = useState<PredeployedAccountsConnector[]>([]);
+    
+    useEffect(() => {
+        async function initializeConnectors() {
+            try {
+                const accounts = await predeployedAccounts({
+                    rpc: dojoConfig.rpcUrl as string,
+                    id: "katana",
+                    name: "Katana",
+                });
+                setConnectors(accounts);
+            } catch (error) {
+                console.error('Failed to initialize connectors:', error);
+            }
+        }
+        
+        initializeConnectors();
+    }, []);

Don't forget to add the necessary imports:

import { useState, useEffect } from 'react';
🧰 Tools
🪛 Biome (1.9.4)

[error] 15-15: The assignment should not be in an expression.

The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.

(lint/suspicious/noAssignInExpressions)

Comment on lines 17 to 33
export default function StarknetProvider({ children }: PropsWithChildren) {
const provider = jsonRpcProvider({
rpc: (chain: Chain) => ({ nodeUrl: dojoConfig.rpcUrl as string }),
});

return (
<StarknetConfig
chains={[mainnet]}
provider={provider}
connectors={pa}
explorer={voyager}
autoConnect
>
{children}
</StarknetConfig>
);
}
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Enhance provider robustness and error handling

The current implementation lacks error handling and loading states, which could lead to runtime issues.

Consider these improvements:

 export default function StarknetProvider({ children }: PropsWithChildren) {
+    const [error, setError] = useState<Error | null>(null);
+    
+    if (!dojoConfig.rpcUrl) {
+        throw new Error('RPC URL is required in dojoConfig');
+    }
+
     const provider = jsonRpcProvider({
-        rpc: (chain: Chain) => ({ nodeUrl: dojoConfig.rpcUrl as string }),
+        rpc: (chain: Chain) => {
+            try {
+                return { nodeUrl: dojoConfig.rpcUrl! };
+            } catch (err) {
+                setError(err as Error);
+                return { nodeUrl: '' };
+            }
+        },
     });
 
+    if (error) {
+        return <div>Failed to initialize StarkNet provider: {error.message}</div>;
+    }
+
+    if (!connectors.length) {
+        return <div>Loading StarkNet configuration...</div>;
+    }
+
     return (
         <StarknetConfig
             chains={[mainnet]}
             provider={provider}
-            connectors={pa}
+            connectors={connectors}
             explorer={voyager}
             autoConnect
         >
             {children}
         </StarknetConfig>
     );
 }

Also, consider wrapping this provider with an error boundary:

class StarknetProviderErrorBoundary extends React.Component {
    // Implementation of error boundary
}

Comment on lines +80 to +82
const parsedEntities = isHistorical
? parseHistoricalEvents<T>(allEntities, options)
: parseEntities<T>(allEntities, options);
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Optimize to avoid duplicate parsing operations.

The same parsing operation is performed twice: once for the callback and once for the return value.

Consider storing the parsed result:

-            const parsedEntities = isHistorical
-                ? parseHistoricalEvents<T>(allEntities, options)
-                : parseEntities<T>(allEntities, options);
+            let parsedEntities;
+            try {
+                parsedEntities = isHistorical
+                    ? parseHistoricalEvents<T>(allEntities, options)
+                    : parseEntities<T>(allEntities, options);
+            } catch (error) {
+                if (options?.logging) {
+                    console.error("Error parsing entities:", error);
+                }
+                callback({ error: error as Error });
+                throw error;
+            }

             callback({ data: parsedEntities });

             if (Object.keys(entities).length < limit) {
                 continueFetching = false;
+                return parsedEntities;
             } else {
                 cursor += limit;
             }
-    return isHistorical
-        ? parseHistoricalEvents<T>(allEntities, options)
-        : parseEntities<T>(allEntities, options);

Also applies to: 104-106

Comment on lines 87 to 101
function Event({ event }) {
if (!event) return null;
const player = event.models?.dojo_starter?.Moved?.player;
const direction = event.models?.dojo_starter?.Moved?.direction;

return (
<div className="text-white flex gap-3">
<div>{event.entityId}</div>
<div>
<div>Player: {player}</div>
<div>Direction: {direction}</div>
</div>
</div>
);
}
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Add type safety and improve null checks in Event component.

The Event component could benefit from TypeScript types and better null checks.

-function Event({ event }) {
+interface EventProps {
+    event: {
+        entityId: string;
+        models?: {
+            dojo_starter?: {
+                Moved?: {
+                    player: string;
+                    direction: string;
+                }
+            }
+        }
+    }
+}
+
+function Event({ event }: EventProps) {
     if (!event) return null;
-    const player = event.models?.dojo_starter?.Moved?.player;
-    const direction = event.models?.dojo_starter?.Moved?.direction;
+    const player = event.models?.dojo_starter?.Moved?.player ?? 'Unknown';
+    const direction = event.models?.dojo_starter?.Moved?.direction ?? 'Unknown';
📝 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
function Event({ event }) {
if (!event) return null;
const player = event.models?.dojo_starter?.Moved?.player;
const direction = event.models?.dojo_starter?.Moved?.direction;
return (
<div className="text-white flex gap-3">
<div>{event.entityId}</div>
<div>
<div>Player: {player}</div>
<div>Direction: {direction}</div>
</div>
</div>
);
}
interface EventProps {
event: {
entityId: string;
models?: {
dojo_starter?: {
Moved?: {
player: string;
direction: string;
}
}
}
}
}
function Event({ event }: EventProps) {
if (!event) return null;
const player = event.models?.dojo_starter?.Moved?.player ?? 'Unknown';
const direction = event.models?.dojo_starter?.Moved?.direction ?? 'Unknown';
return (
<div className="text-white flex gap-3">
<div>{event.entityId}</div>
<div>
<div>Player: {player}</div>
<div>Direction: {direction}</div>
</div>
</div>
);
}

Comment on lines 39 to 69
useEffect(() => {
async function subscribeHistoricalEvent(account: AccountInterface) {
try {
const s = await sdk.subscribeEventQuery({
// query: {
// event_messages_historical: {
// Moved: {
// $: { where: { player: { $eq: addAddressPadding(account.address) } } }
// }
// }
// },
query: { entityIds: [addAddressPadding(account.address)] },
callback: (resp, error) => {
console.log(resp, error);
},
historical: true,
});
setSubscription(s);
} catch (error) {
setEvents([]);
if (unsubscribe) {
unsubscribe();
}
console.error(error);
}
}

if (account) {
subscribeHistoricalEvent(account);
}
}, [account, setEvents]);
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 error handling and cleanup in subscription logic.

  1. Remove commented query structure
  2. Replace console.log with proper error handling
  3. Move subscription cleanup to finally block
 callback: (resp, error) => {
-    console.log(resp, error);
+    if (error) {
+        console.error('Subscription error:', error);
+        return;
+    }
+    // Handle response
 },

Also, consider using a try-finally block for better cleanup:

 try {
     const s = await sdk.subscribeEventQuery({
         // ... subscription logic
     });
-    setSubscription(s);
+    unsubscribeRef.current = s;
 } catch (error) {
     setEvents([]);
-    if (unsubscribe) {
-        unsubscribe();
-    }
     console.error(error);
+} finally {
+    if (unsubscribeRef.current) {
+        unsubscribeRef.current();
+    }
 }

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

Comment on lines +58 to +65
private _inner: PredeployedWallet;
constructor(base: any, rpc: string) {
super({ nodeUrl: rpc }, base);
this._inner = base;
}
request: RequestFn = async (call) => {
return await this._inner.request(call);
};
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Add error handling to request method.

The request method should handle potential errors from the inner wallet.

     request: RequestFn = async (call) => {
+        try {
             return await this._inner.request(call);
+        } catch (error) {
+            console.error('Wallet request failed:', error);
+            throw error;
+        }
     };
📝 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 _inner: PredeployedWallet;
constructor(base: any, rpc: string) {
super({ nodeUrl: rpc }, base);
this._inner = base;
}
request: RequestFn = async (call) => {
return await this._inner.request(call);
};
private _inner: PredeployedWallet;
constructor(base: any, rpc: string) {
super({ nodeUrl: rpc }, base);
this._inner = base;
}
request: RequestFn = async (call) => {
try {
return await this._inner.request(call);
} catch (error) {
console.error('Wallet request failed:', error);
throw error;
}
};

Comment on lines +83 to +85
if (typeof window !== "undefined") {
(window as any)[`starknet_${id}`] = this;
}
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 window object type safety.

The window object assignment should be more type-safe.

-        if (typeof window !== "undefined") {
-            (window as any)[`starknet_${id}`] = this;
-        }
+        if (typeof window !== "undefined") {
+            interface ExtendedWindow extends Window {
+                [key: string]: any;
+            }
+            (window as ExtendedWindow)[`starknet_${id}`] = this;
+        }
📝 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
if (typeof window !== "undefined") {
(window as any)[`starknet_${id}`] = this;
}
if (typeof window !== "undefined") {
interface ExtendedWindow extends Window {
[key: string]: any;
}
(window as ExtendedWindow)[`starknet_${id}`] = this;
}

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: 3

🧹 Nitpick comments (10)
examples/example-vite-react-sdk/src/useModel.tsx (2)

3-3: Consider removing the explicit .ts extension in imports.
Generally, imports in TypeScript do not require the .ts extension. Omitting it can simplify refactoring and ensure consistency across the codebase.

- import { SchemaType } from "./typescript/models.gen.ts";
+ import { SchemaType } from "./typescript/models.gen";

12-15: Make careful note of the updated function signature.
Swapping from string to BigNumberish is a breaking change if other modules still pass string. Ensure that all call sites have been updated accordingly.

examples/example-vite-react-sdk/tsconfig.app.tsbuildinfo (1)

1-1: *Consider excluding .tsbuildinfo files from version control.

This file is generated by the TypeScript compiler and is typically used for incremental builds. Storing it in version control can cause unnecessary merge conflicts and bloat. Consider adding *.tsbuildinfo files to the project's .gitignore.

examples/example-vite-react-sdk/src/typescript/models.gen.ts (2)

1-4: Consider documenting the intent of these new types.
It would be helpful to add short doc comments summarizing each interface’s purpose within the system (e.g., a game domain concept, a user action, etc.), facilitating easier onboarding for new developers.


5-6: Ensure consistent naming for utility types.
The WithFieldOrder type is descriptive, but consider whether it might be more consistent to name it WithSerializedFields or similar, matching how the type might be used in serialization.

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

54-77: Provide error handling for subscription setup.
This block manages a subscription but only logs errors to the console. Consider surfacing errors to the UI or a state variable to alert users or retry the subscription.


167-169: Use optional chaining to avoid potential runtime errors.
Static analysis suggests changing this to an optional chain. If moves.last_direction is undefined, moves.last_direction?.unwrap() would be safer.

- {moves && moves.last_direction.isSome()
-     ? moves.last_direction.unwrap()
-     : ""}
+ {moves?.last_direction?.isSome()
+     ? moves.last_direction.unwrap()
+     : ""}
🧰 Tools
🪛 Biome (1.9.4)

[error] 167-167: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)


249-252: Clarify the “N/A” handling for lastDirection.
When last_direction is absent, the table displays N/A. Confirm whether the user should see “N/A” or a friendlier message like “No moves yet.”

packages/predeployed-connector/src/index.ts (2)

49-53: Clarify options in the constructor comment.
This constructor merges options and calls super. Consider documenting that WithAccount<PredeployedAccountsConnectorOptions> includes the actual wallet account, which must be available for subsequent operations.


62-65: Use typed constructor parameters.
Currently, base: any obscures the nature of PredeployedWallet. Consider specifying a more precise type for improved maintainability.

-class PredeployedWalletAccount extends WalletAccount {
-    constructor(base: any, rpc: string) {
+class PredeployedWalletAccount extends WalletAccount {
+    constructor(base: PredeployedWallet, rpc: string) {
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 50cc19e and ffc30b4.

📒 Files selected for processing (9)
  • examples/example-vite-react-sdk/src/App.tsx (9 hunks)
  • examples/example-vite-react-sdk/src/historical-events.tsx (1 hunks)
  • examples/example-vite-react-sdk/src/starknet-provider.tsx (1 hunks)
  • examples/example-vite-react-sdk/src/typescript/contracts.gen.ts (1 hunks)
  • examples/example-vite-react-sdk/src/typescript/models.gen.ts (1 hunks)
  • examples/example-vite-react-sdk/src/useModel.tsx (2 hunks)
  • examples/example-vite-react-sdk/tsconfig.app.tsbuildinfo (1 hunks)
  • packages/predeployed-connector/src/index.ts (3 hunks)
  • packages/sdk/src/types.ts (4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
  • examples/example-vite-react-sdk/src/typescript/contracts.gen.ts
  • examples/example-vite-react-sdk/src/historical-events.tsx
🧰 Additional context used
🪛 Biome (1.9.4)
examples/example-vite-react-sdk/src/App.tsx

[error] 167-167: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)

examples/example-vite-react-sdk/src/starknet-provider.tsx

[error] 15-15: The assignment should not be in an expression.

The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.

(lint/suspicious/noAssignInExpressions)

⏰ Context from checks skipped due to timeout of 90000ms (1)
  • GitHub Check: check
🔇 Additional comments (12)
examples/example-vite-react-sdk/src/useModel.tsx (2)

21-23: Confirm the usage of entityId.toString() as an object key.
While this is valid for a typical dictionary, consider large numeric values or potential leading zeros in hex strings that might cause confusion. You may want to validate or document how entityId is expected to convert to string.


1-1: Confirm consistent usage of BigNumberish across the codebase.
Check if all other references or function calls that pass or manipulate entityId also rely on BigNumberish. This helps avoid unwanted type mismatches.

To confirm, feel free to run a search script on the entire codebase:

✅ Verification successful

BigNumberish usage is consistent across the codebase
The type is consistently used for numeric fields in models and is correctly imported from 'starknet' package throughout the codebase. No type mismatches were found.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify usage of BigNumberish
rg "BigNumberish" -A 5

Length of output: 12830

packages/sdk/src/types.ts (4)

146-156: Ensure clarity on nested query structures.
The union of BaseQueryType and dynamic keys referencing SubscriptionWhereOptions can introduce complexity. Testing is recommended to confirm that the nested queries reflect the intended subscription behavior.


303-303: Good expansion of return type.
Allowing getEventMessages to return either a single or an array of StandardizedQueryResult<T> provides more flexibility. Confirm that all consumers of this function handle both possibilities.


390-390: Callback type extension is aligned with multi-result returns.
Updating the callback signature to accept an array or a single StandardizedQueryResult<T> is consistent with the adjusted return type. Ensure external callers are updated to handle both scenarios gracefully.


404-404: Preserve type consistency for GetParams callbacks.
Including the union for data maintains uniformity across the SDK. Keep the callback logic synchronized with subscribeEventQuery to avoid discrepancies in handling single vs. multiple results.

examples/example-vite-react-sdk/tsconfig.app.tsbuildinfo (1)

1-1: Verify added file references.

The root array contains newly referenced files: "./src/historical-events.tsx", "./src/starknet-provider.tsx", "./src/wallet-account.tsx", "./src/typescript/contracts.gen.ts", "./src/typescript/models.gen.ts". Ensure these files exist, build correctly, and align with your updated imports.

✅ Verification successful

All referenced files exist and are properly located

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check that newly referenced files exist in the repository.
fd historical-events.tsx
fd starknet-provider.tsx
fd wallet-account.tsx
fd contracts.gen.ts
fd models.gen.ts

Length of output: 1011

examples/example-vite-react-sdk/src/typescript/models.gen.ts (1)

61-67: Validate directional enums against practical usage.
Ensure that consumers of the Direction enum handle out-of-range or uninitialized directions to avoid edge cases (e.g., a direction unrecognized by the game logic).

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

10-15: ⚠️ Potential issue

Avoid side-effect assignments in promise callbacks.
Assigning pa = p within the then callback introduces global mutable state, which can lead to race conditions in a concurrent environment. Refactor to store p in component state or via React lifecycle hooks for safer updates.

- predeployedAccounts({
-   rpc: dojoConfig.rpcUrl as string,
-   id: "katana",
-   name: "Katana",
- }).then((p) => (pa = p));
+ useEffect(() => {
+   predeployedAccounts({
+     rpc: dojoConfig.rpcUrl as string,
+     id: "katana",
+     name: "Katana",
+   }).then((p) => setPa(p));
+ }, []);

Likely invalid or redundant comment.

🧰 Tools
🪛 Biome (1.9.4)

[error] 15-15: The assignment should not be in an expression.

The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.

(lint/suspicious/noAssignInExpressions)

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

38-49: Handle cases where account is undefined.
Currently, the code uses BigInt(0) when account is undefined. Double-check that this fallback ID will not collide with valid accounts or introduce confusion.

packages/predeployed-connector/src/index.ts (2)

87-89: Favor an extended window interface over any.
When assigning (window as any)[\starknet_${id}`], consider refining the type. This ensures better type safety if your code base depends on window.starknet_`.


Line range hint 128-146: Handle partial or missing predeployed accounts gracefully.
If provider.channel.getPredeployedAccounts() returns an empty list or an error, ensure that the UI or host code can safely handle the absence of accounts.

Comment on lines +82 to +130
export const schema: SchemaType = {
dojo_starter: {
DirectionsAvailable: {
fieldOrder: ["player", "directions"],
player: "",
directions: [Direction.Left],
},
DirectionsAvailableValue: {
fieldOrder: ["directions"],
directions: [Direction.Left],
},
Moves: {
fieldOrder: ["player", "remaining", "last_direction", "can_move"],
player: "",
remaining: 0,
last_direction: new CairoOption(CairoOptionVariant.None),
can_move: false,
},
MovesValue: {
fieldOrder: ["remaining", "last_direction", "can_move"],
remaining: 0,
last_direction: new CairoOption(CairoOptionVariant.None),
can_move: false,
},
Position: {
fieldOrder: ["player", "vec"],
player: "",
vec: { x: 0, y: 0 },
},
PositionValue: {
fieldOrder: ["vec"],
vec: { x: 0, y: 0 },
},
Vec2: {
fieldOrder: ["x", "y"],
x: 0,
y: 0,
},
Moved: {
fieldOrder: ["player", "direction"],
player: "",
direction: Direction.Left,
},
MovedValue: {
fieldOrder: ["direction"],
direction: Direction.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

Use caution with default field values.
The schema object provides default values (e.g., [Direction.Left], 0, false), which might mask missing or invalid data. Verify that this usage aligns with domain requirements and does not inadvertently overwrite meaningful defaults or indicate partial user data.

Comment on lines +17 to +33
export default function StarknetProvider({ children }: PropsWithChildren) {
const provider = jsonRpcProvider({
rpc: () => ({ nodeUrl: dojoConfig.rpcUrl as string }),
});

return (
<StarknetConfig
chains={[mainnet]}
provider={provider}
connectors={pa}
explorer={voyager}
autoConnect
>
{children}
</StarknetConfig>
);
}
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Initialize connectors inside React lifecycle.
This provider sets up the StarknetConfig with a mutable pa array. Consider transitioning to a React hook state, so the provider reliably re-renders upon new accounts becoming available. This approach aligns with recommended React patterns.

Comment on lines +202 to +205
await client.actions.move(
account!,
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

Check for uninitialized account in move calls.
onClick directly calls client.actions.move(account!, direction). If account is not set or an error occurs, it may break. Replace the assertion ! operator with safer checks or error boundaries.

@MartianGreed MartianGreed merged commit d37e8c4 into main Jan 6, 2025
4 checks passed
@MartianGreed MartianGreed deleted the feat/update-react-sdk-example branch January 6, 2025 20:20
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