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: draft zustand state management system #280

Merged
merged 7 commits into from
Sep 27, 2024
Merged
Show file tree
Hide file tree
Changes from 3 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
12 changes: 9 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,19 @@
"preview": "vite preview"
},
"dependencies": {
"@dojoengine/core": "1.0.0-alpha.12",
"@dojoengine/core": "workspace:*",
"@dojoengine/create-burner": "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",
"starknet": "6.11.0",
"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 useDojoStore = createDojoStore<Schema>();

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

Comment on lines +11 to +12
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider optimizing state selection

While using useDojoStore for state management is a good approach, selecting the entire state on line 11 might lead to unnecessary re-renders. Consider selecting only the specific parts of the state that this component needs.

Example:

const { entities, someOtherNeededState } = useDojoStore(state => ({
  entities: state.entities,
  someOtherNeededState: state.someOtherNeededState
}));

This way, the component will only re-render when these specific parts of the state change.

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
3 changes: 3 additions & 0 deletions packages/sdk/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
"./package.json": "./package.json"
},
"devDependencies": {
"@rollup/plugin-commonjs": "^28.0.0",
"@vitest/coverage-v8": "^1.6.0",
"eslint": "^8.57.1",
"prettier": "^2.8.8",
Expand All @@ -35,7 +36,9 @@
},
"dependencies": {
"@dojoengine/torii-client": "workspace:*",
"@dojoengine/create-burner": "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;
}
Loading
Loading