Skip to content

fix: tests, improvements #302

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Oct 12, 2024
Merged

fix: tests, improvements #302

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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: 11 additions & 1 deletion examples/example-vite-react-app-recs/src/dojo/setup.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { DojoConfig, DojoProvider } from "@dojoengine/core";
import { BurnerManager } from "@dojoengine/create-burner";
import { getSyncEvents } from "@dojoengine/state";
import { getSyncEntities, getSyncEvents } from "@dojoengine/state";
import * as torii from "@dojoengine/torii-client";
import { Account, ArraySignatureType } from "starknet";

Expand All @@ -24,6 +24,15 @@ export async function setup({ ...config }: DojoConfig) {
// Define contract components based on the world configuration
const contractComponents = defineContractComponents(world);

const getSync = await getSyncEntities(
toriiClient,
contractComponents as any,
undefined,
[],
3000,
true
);
Comment on lines +27 to +34
Copy link

Choose a reason for hiding this comment

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

💡 Codebase verification

Clarification Needed on getSyncEntities Parameters

The initialization of getSync looks good, but there are a few points that need clarification:

  1. The contractComponents is cast to any. Is this necessary, or can we use a more specific type?
  2. Could you explain the purpose of the hardcoded values (undefined, empty array, 3000, true)?
  3. Consider adding comments to explain the significance of these parameters for better maintainability.
🔗 Analysis chain

Please clarify the parameters passed to getSyncEntities.

The initialization of getSync looks good, but there are a few points that need clarification:

  1. The contractComponents is cast to any. Is this necessary, or can we use a more specific type?
  2. Could you explain the purpose of the hardcoded values (undefined, empty array, 3000, true)?
  3. Consider adding comments to explain the significance of these parameters for better maintainability.

To ensure getSyncEntities is used consistently across the project, let's check its usage:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for other usages of getSyncEntities
rg --type typescript 'getSyncEntities\(' -A 5

Length of output: 82

Script:

#!/bin/bash
# Search for other usages of getSyncEntities in TypeScript files
rg --type ts --type tsx 'getSyncEntities\(' -A 5

Length of output: 78

Script:

#!/bin/bash
# Search for other usages of getSyncEntities in .ts and .tsx files
rg 'getSyncEntities\(' --glob '*.ts' --glob '*.tsx' -A 5

Length of output: 4520


// Create client-side components that mirror the contract components
const clientComponents = createClientComponents({ contractComponents });

Expand Down Expand Up @@ -79,5 +88,6 @@ export async function setup({ ...config }: DojoConfig) {
burnerManager,
toriiClient,
eventSync,
getSync,
};
}
2 changes: 1 addition & 1 deletion packages/state/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@
},
"dependencies": {
"@dojoengine/recs": "2.0.13",
"@dojoengine/torii-client": "workspace:*",
"@dojoengine/torii-client": "1.0.0-alpha.20",
Copy link

Choose a reason for hiding this comment

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

💡 Codebase verification

Inconsistent @dojoengine/torii-client Versions Detected

The packages/state/package.json is using version 1.0.0-alpha.20 for @dojoengine/torii-client, while other packages are referencing it as workspace:*. This discrepancy may lead to version conflicts and affect dependency management across the project.

  • Action Required:
    • Update all package.json files to use a consistent version for @dojoengine/torii-client.
    • Alternatively, revert packages/state/package.json to use workspace:* if the specific versioning is not necessary.
🔗 Analysis chain

LGTM! Consider the impact on development workflow.

The change from workspace:* to a specific version 1.0.0-alpha.20 for @dojoengine/torii-client is approved. This update aligns with the package version, which is a good practice for version consistency.

However, please consider the following:

  1. This change might impact the development workflow, especially if you were previously using a local workspace reference.
  2. Ensure that this version is consistent with other packages in the project that might depend on @dojoengine/torii-client.

To verify version consistency across the project, run the following script:

This script will help identify any inconsistencies in the usage of @dojoengine/torii-client across the project.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for inconsistencies in @dojoengine/torii-client versions across the project

# Search for @dojoengine/torii-client in all package.json files
echo "Searching for @dojoengine/torii-client in package.json files:"
rg --type json '"@dojoengine/torii-client"' -g 'package.json'

# Search for import statements of @dojoengine/torii-client in TypeScript/JavaScript files
echo -e "\nSearching for import statements of @dojoengine/torii-client:"
rg --type-add 'ts:*.{ts,tsx,js,jsx}' --type ts 'from\s+[''"]@dojoengine/torii-client[''"]'

Length of output: 3633

"vitest": "^1.6.0"
}
}
237 changes: 235 additions & 2 deletions packages/state/src/__tests__/utils.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { Type as RecsType } from "@dojoengine/recs";
import { describe, expect, it } from "vitest";
import { Type as RecsType, Schema } from "@dojoengine/recs";
import { describe, expect, it, vi } from "vitest";

import { convertValues } from "../utils";

Expand Down Expand Up @@ -108,4 +108,237 @@ describe("convertValues", () => {
expect(result.nested).toEqual({ innerField: { value: "42" } });
});
});

it("should handle null and undefined values", () => {
const schema: Schema = {
name: RecsType.String,
age: RecsType.Number,
};
const values = {
name: { value: "Alice", type: "string" },
age: undefined,
};
const expected = {
name: "Alice",
age: undefined,
};
expect(convertValues(schema, values)).toEqual(expected);
});

it("should convert enum types correctly", () => {
const schema: Schema = {
status: RecsType.String,
};
const values = {
status: { value: { option: "ACTIVE" }, type: "enum" },
};
const expected = {
status: "ACTIVE",
};
expect(convertValues(schema, values)).toEqual(expected);
});

it("should handle RecsType.StringArray with empty array", () => {
const schema: Schema = {
tags: RecsType.StringArray,
};
const values = {
tags: { value: [], type: "array" },
};
const expected = {
tags: [],
};
expect(convertValues(schema, values)).toEqual(expected);
});

it("should handle RecsType.StringArray with enum items", () => {
const schema: Schema = {
tags: RecsType.StringArray,
};
const values = {
tags: {
value: [
{ value: { option: "TAG1" }, type: "enum" },
{ value: { option: "TAG2" }, type: "enum" },
],
type: "array",
},
};
const expected = {
tags: ["TAG1", "TAG2"],
};
expect(convertValues(schema, values)).toEqual(expected);
});

it("should handle RecsType.StringArray with BigInt conversion", () => {
const schema: Schema = {
ids: RecsType.StringArray,
};
const values = {
ids: {
value: [
{ value: "12345678901234567890", type: "string" },
{ value: "98765432109876543210", type: "string" },
],
type: "array",
},
};
const expected = {
ids: [
BigInt("12345678901234567890"),
BigInt("98765432109876543210"),
],
};
expect(convertValues(schema, values)).toEqual(expected);
});

Comment on lines +173 to +193
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Potential inconsistency: RecsType.StringArray expected to return strings

In this test case, RecsType.StringArray is used in the schema, but the expected result contains an array of BigInt values. Since RecsType.StringArray implies an array of strings, converting items to BigInt may be inconsistent with the type definition. Consider using a type like RecsType.BigIntArray if available, or adjust the expected output to reflect an array of strings.

it("should fallback to string if BigInt conversion fails", () => {
vi.spyOn(console, "warn").mockImplementation(() => {});

const schema: Schema = {
ids: RecsType.StringArray,
};
const values = {
ids: {
value: [{ value: "invalid_bigint", type: "string" }],
type: "array",
},
};
const expected = {
ids: ["invalid_bigint"],
};
expect(convertValues(schema, values)).toEqual(expected);
expect(console.warn).toHaveBeenCalledWith(
"Failed to convert invalid_bigint to BigInt. Using string value instead."
);
});

it("should handle RecsType.String", () => {
const schema: Schema = {
name: RecsType.String,
};
const values = {
name: { value: "Bob", type: "string" },
};
const expected = {
name: "Bob",
};
expect(convertValues(schema, values)).toEqual(expected);
});

it("should handle RecsType.BigInt with valid BigInt", () => {
const schema: Schema = {
balance: RecsType.BigInt,
};
const values = {
balance: { value: "1000000000000000000", type: "string" },
};
const expected = {
balance: BigInt("1000000000000000000"),
};
expect(convertValues(schema, values)).toEqual(expected);
});

it("should handle RecsType.Boolean", () => {
const schema: Schema = {
isActive: RecsType.Boolean,
};
const values = {
isActive: { value: true, type: "boolean" },
};
const expected = {
isActive: true,
};
expect(convertValues(schema, values)).toEqual(expected);
});

it("should handle RecsType.Number", () => {
const schema: Schema = {
score: RecsType.Number,
};
const values = {
score: { value: "42", type: "string" },
};
const expected = {
score: 42,
};
expect(convertValues(schema, values)).toEqual(expected);
});

it("should handle nested structs", () => {
const nestedSchema: Schema = {
street: RecsType.String,
zip: RecsType.Number,
};
const schema: Schema = {
name: RecsType.String,
address: nestedSchema,
};
const values = {
name: { value: "Charlie", type: "string" },
address: {
value: {
street: { value: "123 Main St", type: "string" },
zip: { value: "12345", type: "string" },
},
type: "struct",
},
};
const expected = {
name: "Charlie",
address: {
street: "123 Main St",
zip: 12345,
},
};
expect(convertValues(schema, values)).toEqual(expected);
});

it("should handle map structures", () => {
const nestedSchema: Schema = {
key1: RecsType.String,
key2: RecsType.Number,
};
const schema: Schema = {
config: nestedSchema,
};
const values = {
config: {
value: new Map([
["key1", { value: "value1", type: "string" }],
["key2", { value: "100", type: "string" }],
]),
type: "struct",
},
};
const expected = {
config: {
key1: "value1",
key2: 100,
},
};
expect(convertValues(schema, values)).toEqual(expected);
});

it("should handle primitive fallback in default case", () => {
const schema: Schema = {
miscellaneous: RecsType.String,
};
const values = {
miscellaneous: { value: "some value", type: "unknown" },
};
const expected = {
miscellaneous: "some value",
};
expect(convertValues(schema, values)).toEqual(expected);
});

it("should handle empty schema", () => {
const schema: Schema = {};
const values = {
anyKey: { value: "any value", type: "string" },
};
const expected = {};
expect(convertValues(schema, values)).toEqual(expected);
});
});
45 changes: 34 additions & 11 deletions packages/state/src/recs/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ export const getSyncEntities = async <S extends Schema>(
clause: Clause | undefined,
entityKeyClause: EntityKeysClause[],
limit: number = 100,
logging: boolean = true
logging: boolean = false
) => {
if (logging) console.log("Starting getSyncEntities");
await getEntities(client, clause, components, limit, logging);
Expand Down Expand Up @@ -199,7 +199,7 @@ export const getEntitiesQuery = async <S extends Schema>(
entityKeyClause: EntityKeysClause,
patternMatching: PatternMatching = "FixedLen",
limit: number = 1000,
logging: boolean = true
logging: boolean = false
) => {
if (logging) console.log("Starting getEntitiesQuery");
let cursor = 0;
Expand Down Expand Up @@ -310,7 +310,8 @@ export const setEntities = async <S extends Schema>(
components: Component<S, Metadata, undefined>[],
logging: boolean = false
) => {
if (logging) console.log(entities);
if (logging) console.log("Entities to set:", entities);

for (let key in entities) {
if (!Object.hasOwn(entities, key)) {
continue;
Expand All @@ -330,14 +331,32 @@ export const setEntities = async <S extends Schema>(

if (recsComponent) {
try {
setComponent(
recsComponent,
key as Entity,
convertValues(
recsComponent.schema,
entities[key][componentName]
) as ComponentValue
);
const rawValue = entities[key][componentName];
if (logging)
console.log(
`Raw value for ${componentName} on ${key}:`,
rawValue
);

const convertedValue = convertValues(
recsComponent.schema,
rawValue
) as ComponentValue;

if (logging)
console.log(
`Converted value for ${componentName} on ${key}:`,
convertedValue
);

if (!convertedValue) {
console.error(
`convertValues returned undefined or invalid for ${componentName} on ${key}`
);
}

setComponent(recsComponent, key as Entity, convertedValue);

if (logging)
console.log(
`Set component ${recsComponent.metadata?.name} on ${key}`
Expand All @@ -348,6 +367,10 @@ export const setEntities = async <S extends Schema>(
error
);
}
} else {
console.warn(
`Component ${componentName} not found in provided components.`
);
}
}
}
Expand Down
Loading
Loading