Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 7 additions & 3 deletions examples/example-vite-react-sdk/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -10,13 +10,17 @@
"preview": "vite preview"
},
"dependencies": {
"@dojoengine/core": "1.0.0-alpha.12",
"@dojoengine/core": "workspace:*",
"@dojoengine/sdk": "workspace:*",
"@dojoengine/torii-wasm": "1.0.0-alpha.12",
"@dojoengine/torii-wasm": "workspace:*",
"@types/uuid": "^10.0.0",
"immer": "^10.1.1",
"react": "^18.3.1",
"react-dom": "^18.3.1",
"uuid": "^10.0.0",
"vite-plugin-top-level-await": "^1.4.4",
"vite-plugin-wasm": "^3.3.0"
"vite-plugin-wasm": "^3.3.0",
"zustand": "^4.5.5"
},
"devDependencies": {
"@eslint/js": "^9.11.1",
Expand Down
86 changes: 50 additions & 36 deletions examples/example-vite-react-sdk/src/App.tsx
Original file line number Diff line number Diff line change
@@ -1,10 +1,15 @@
import { useEffect, useState } from "react";
import { useEffect } from "react";
import "./App.css";
import { ParsedEntity, SDK } from "@dojoengine/sdk";
import { SDK, createDojoStore } from "@dojoengine/sdk";
import { Schema } from "./bindings.ts";

import { v4 as uuidv4 } from "uuid";

export const useGameState = createDojoStore<Schema>();

function App({ db }: { db: SDK<Schema> }) {
const [entities, setEntities] = useState<ParsedEntity<Schema>[]>([]);
const state = useGameState((state) => state);
const entities = useGameState((state) => state.entities);
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Simplify state usage to prevent unnecessary re-renders

You are accessing the entire state and state.entities separately using useGameState. This can lead to unnecessary re-renders. Since you have the entire state with state, you can use state.entities directly without needing to select it separately.


useEffect(() => {
let unsubscribe: (() => void) | undefined;
Expand All @@ -28,15 +33,7 @@ function App({ db }: { db: SDK<Schema> }) {
response.data &&
response.data[0].entityId !== "0x0"
) {
console.log(response.data);
setEntities((prevEntities) => {
return prevEntities.map((entity) => {
const newEntity = response.data?.find(
(e) => e.entityId === entity.entityId
);
return newEntity ? newEntity : entity;
});
});
state.setEntities(response.data);
}
},
{ logging: true }
Expand All @@ -54,8 +51,6 @@ function App({ db }: { db: SDK<Schema> }) {
};
}, [db]);

console.log("entities:");

useEffect(() => {
const fetchEntities = async () => {
try {
Expand All @@ -76,23 +71,7 @@ function App({ db }: { db: SDK<Schema> }) {
return;
}
if (resp.data) {
console.log(resp.data);
setEntities((prevEntities) => {
const updatedEntities = [...prevEntities];
resp.data?.forEach((newEntity) => {
const index = updatedEntities.findIndex(
(entity) =>
entity.entityId ===
newEntity.entityId
);
if (index !== -1) {
updatedEntities[index] = newEntity;
} else {
updatedEntities.push(newEntity);
}
});
return updatedEntities;
});
state.setEntities(resp.data);
}
}
);
Expand All @@ -104,20 +83,55 @@ function App({ db }: { db: SDK<Schema> }) {
fetchEntities();
}, [db]);

const optimisticUpdate = async () => {
const entityId =
"0x571368d35c8fe136adf81eecf96a72859c43de7efd8fdd3d6f0d17e308df984";
Comment on lines +87 to +88
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Avoid hardcoding the entityId

Hardcoding the entityId reduces flexibility and may cause issues if the ID changes. Consider passing the entityId as a parameter or obtaining it dynamically based on the application's context.


const transactionId = uuidv4();

state.applyOptimisticUpdate(transactionId, (draft) => {
draft.entities[entityId].models.dojo_starter.Moves!.remaining = 10;
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Ensure entity exists before applying optimistic update

When applying the optimistic update, ensure that draft.entities[entityId] exists to prevent potential runtime errors if the entity is undefined.

Suggested fix:

 state.applyOptimisticUpdate(transactionId, (draft) => {
+    if (draft.entities[entityId]) {
         draft.entities[entityId].models.dojo_starter.Moves!.remaining = 10;
+    }
 });
📝 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
draft.entities[entityId].models.dojo_starter.Moves!.remaining = 10;
state.applyOptimisticUpdate(transactionId, (draft) => {
if (draft.entities[entityId]) {
draft.entities[entityId].models.dojo_starter.Moves!.remaining = 10;
}
});

});

try {
// Wait for the entity to be updated before full resolving the transaction. Reverts if the condition is not met.
const updatedEntity = await state.waitForEntityChange(
entityId,
(entity) => {
// Define your specific condition here
return entity?.models.dojo_starter.Moves?.can_move === true;
Comment on lines +101 to +102
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Define the specific condition for waitForEntityChange

The condition in waitForEntityChange is currently a placeholder. Please replace it with the actual condition that determines when the entity has been updated.

Example:

 return entity?.models.dojo_starter.Moves?.can_move === true;
- // Define your specific condition here
+ // Condition: Checks if the 'can_move' flag is true

Committable suggestion was skipped due to low confidence.

}
);

console.log("Entity has been updated to active:", updatedEntity);

console.log("Updating entities...");
} catch (error) {
console.error("Error updating entities:", error);
state.revertOptimisticUpdate(transactionId);
} finally {
console.log("Updating entities...");
state.confirmTransaction(transactionId);
}
Comment on lines +112 to +115
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Review transaction confirmation placement

Calling state.confirmTransaction(transactionId); in the finally block means it executes regardless of success or failure. If the transaction fails and is reverted, confirming it may not be appropriate. Consider moving the confirmation inside the try block after the update succeeds.

Suggested change:

     } catch (error) {
         console.error("Error updating entities:", error);
         state.revertOptimisticUpdate(transactionId);
-    } finally {
+    } finally {
         console.log("Updating entities...");
-        state.confirmTransaction(transactionId);
     }
+    state.confirmTransaction(transactionId);
📝 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
} finally {
console.log("Updating entities...");
state.confirmTransaction(transactionId);
}
} catch (error) {
console.error("Error updating entities:", error);
state.revertOptimisticUpdate(transactionId);
} finally {
console.log("Updating entities...");
}
state.confirmTransaction(transactionId);

};

return (
<div>
<h1>Game State</h1>
{entities.map((entity) => (
<div key={entity.entityId}>
<h2>Entity {entity.entityId}</h2>
<button onClick={optimisticUpdate}>update</button>
{Object.entries(entities).map(([entityId, entity]) => (
<div key={entityId}>
<h2>Entity {entityId}</h2>
<h3>Position</h3>
<p>
Player:{" "}
{entity.models.dojo_starter.Position?.player ?? "N/A"}
<br />
X: {entity.models.dojo_starter.Position?.vec.x ?? "N/A"}
X:{" "}
{entity.models.dojo_starter.Position?.vec?.x ?? "N/A"}
<br />
Y: {entity.models.dojo_starter.Position?.vec.y ?? "N/A"}
Y:{" "}
{entity.models.dojo_starter.Position?.vec?.y ?? "N/A"}
</p>
<h3>Moves</h3>
<p>
Expand Down
1 change: 1 addition & 0 deletions packages/sdk/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@
"dependencies": {
"@dojoengine/torii-client": "workspace:*",
"axios": "^0.27.2",
"immer": "^10.1.1",
"lodash": "^4.17.21",
"vite-plugin-wasm": "^3.3.0",
"zustand": "^4.5.5"
Expand Down
1 change: 1 addition & 0 deletions packages/sdk/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import { SchemaType, SDK, UnionOfModelData } from "./types";
import { Account, Signature, StarknetDomain, TypedData } from "starknet";

export * from "./types";
export * from "./state";
Copy link

Choose a reason for hiding this comment

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

💡 Codebase verification

Issue Detected: Missing state.ts Module in Export Statement.

The export statement export * from "./state"; in packages/sdk/src/index.ts references a non-existent state.ts file, which will lead to import errors for SDK consumers.

  • packages/sdk/src/state.ts does not exist.
🔗 Analysis chain

LGTM! Verify for potential naming conflicts.

The new export statement is consistent with the PR objective of implementing a state management system. It appropriately exposes the state module's functionality to SDK consumers.

To ensure there are no naming conflicts, please run the following script:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for potential naming conflicts between the new state exports and existing exports

# Get existing exports
existing_exports=$(rg -o --no-filename 'export \{.*?\}' packages/sdk/src/index.ts | tr -d '{}' | tr ',' '\n' | sed 's/^ *//g')

# Get exports from the state module
state_exports=$(rg -o --no-filename 'export .*' packages/sdk/src/state.ts | sed 's/export //g')

# Check for conflicts
conflicts=$(comm -12 <(echo "$existing_exports" | sort) <(echo "$state_exports" | sort))

if [ -n "$conflicts" ]; then
  echo "Potential naming conflicts detected:"
  echo "$conflicts"
else
  echo "No naming conflicts detected."
fi

Length of output: 543


interface SDKConfig {
client: torii.ClientConfig;
Expand Down
1 change: 1 addition & 0 deletions packages/sdk/src/state/index.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export * from "./zustand";
151 changes: 151 additions & 0 deletions packages/sdk/src/state/zustand.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,151 @@
import { create } from "zustand";
import { immer } from "zustand/middleware/immer";
import {
Draft,
Patch,
WritableDraft,
applyPatches,
produceWithPatches,
} from "immer";

import { enablePatches } from "immer";
import { subscribeWithSelector } from "zustand/middleware";
import { ParsedEntity, SchemaType } from "../types";

enablePatches();

interface PendingTransaction {
transactionId: string;
patches: Patch[];
inversePatches: Patch[];
}

interface GameState<T extends SchemaType> {
entities: Record<string, ParsedEntity<T>>;
pendingTransactions: Record<string, PendingTransaction>;
setEntities: (entities: ParsedEntity<T>[]) => void;
updateEntity: (entity: Partial<ParsedEntity<T>>) => void;
applyOptimisticUpdate: (
transactionId: string,
updateFn: (draft: Draft<GameState<T>>) => void
) => void;
revertOptimisticUpdate: (transactionId: string) => void;
confirmTransaction: (transactionId: string) => void;
subscribeToEntity: (
entityId: string,
listener: (entity: ParsedEntity<T> | undefined) => void
) => () => void;
waitForEntityChange: (
entityId: string,
predicate: (entity: ParsedEntity<T> | undefined) => boolean,
timeout?: number
) => Promise<ParsedEntity<T> | undefined>;
}

/**
* Factory function to create a Zustand store based on a given SchemaType.
*
* @template T - The schema type.
* @returns A Zustand hook tailored to the provided schema.
*/
export function createDojoStore<T extends SchemaType>() {
const useStore = create<GameState<T>>()(
subscribeWithSelector(
immer((set, get) => ({
entities: {},
pendingTransactions: {},
setEntities: (entities: ParsedEntity<T>[]) => {
set((state: Draft<GameState<T>>) => {
entities.forEach((entity) => {
state.entities[entity.entityId] =
entity as WritableDraft<ParsedEntity<T>>;
});
});
},
updateEntity: (entity: Partial<ParsedEntity<T>>) => {
set((state: Draft<GameState<T>>) => {
if (
entity.entityId &&
state.entities[entity.entityId]
) {
Object.assign(
state.entities[entity.entityId],
entity
);
}
});
},
Comment on lines +73 to +85
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Handle the case where an entity does not exist in the state

In the updateEntity method, if the entity.entityId does not exist in state.entities, the method currently does nothing. Consider handling this case explicitly, perhaps by logging a warning or creating a new entry, depending on the desired behavior.

applyOptimisticUpdate: (transactionId, updateFn) => {
const currentState = get();
const [nextState, patches, inversePatches] =
produceWithPatches(
currentState,
(draftState: Draft<GameState<T>>) => {
updateFn(draftState);
}
);

set(() => nextState);

set((state: Draft<GameState<T>>) => {
state.pendingTransactions[transactionId] = {
transactionId,
patches,
inversePatches,
};
});
},
revertOptimisticUpdate: (transactionId) => {
const transaction =
get().pendingTransactions[transactionId];
if (transaction) {
set((state: Draft<GameState<T>>) =>
applyPatches(state, transaction.inversePatches)
);
set((state: Draft<GameState<T>>) => {
delete state.pendingTransactions[transactionId];
});
}
},
Comment on lines +106 to +117
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 error handling for missing transactions in revertOptimisticUpdate

When a transaction ID is not found in pendingTransactions, the revertOptimisticUpdate method silently does nothing. Consider adding error handling or logging to notify when a transaction ID is invalid or missing.

confirmTransaction: (transactionId) => {
set((state: Draft<GameState<T>>) => {
delete state.pendingTransactions[transactionId];
});
},
subscribeToEntity: (entityId, listener): (() => void) => {
return useStore.subscribe((state) => {
const entity = state.entities[entityId];
listener(entity);
});
},
Comment on lines +123 to +128
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Clarify the use of useStore within store methods

The methods subscribeToEntity and waitForEntityChange are using useStore inside the store definition. While this works due to closures, it might be clearer to access the store via get() or restructure the code to avoid potential confusion.

waitForEntityChange: (entityId, predicate, timeout = 6000) => {
return new Promise<ParsedEntity<T> | undefined>(
(resolve, reject) => {
const unsubscribe = useStore.subscribe(
(state) => state.entities[entityId],
(entity) => {
if (predicate(entity)) {
clearTimeout(timer);
unsubscribe();
resolve(entity);
}
}
);

const timer = setTimeout(() => {
unsubscribe();
reject(
new Error(
`waitForEntityChange: Timeout of ${timeout}ms exceeded`
)
);
}, timeout);
}
);
},
}))
)
);

return useStore;
}
4 changes: 3 additions & 1 deletion packages/sdk/src/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -226,7 +226,9 @@ export type ParsedEntity<T extends SchemaType> = {
entityId: string;
models: {
[K in keyof T]: {
[M in keyof T[K]]?: T[K][M];
[M in keyof T[K]]?: T[K][M] extends object
? Partial<T[K][M]>
: T[K][M];
Comment on lines +230 to +232
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Refine type check for better type safety

The current implementation applies Partial<T[K][M]> when T[K][M] extends object. This includes arrays and functions, which might not be the intended behavior. To improve type safety, consider refining the type check to target only plain object types:

[M in keyof T[K]]?: T[K][M] extends object
    ? T[K][M] extends Record<string, any> ? Partial<T[K][M]> : T[K][M]
    : T[K][M];

This change ensures that Partial is applied only to plain objects, excluding arrays and functions.

};
};
};
Expand Down
Loading