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(world): add World prototype #405

Merged
merged 42 commits into from
Feb 22, 2023
Merged

feat(world): add World prototype #405

merged 42 commits into from
Feb 22, 2023

Conversation

alvrs
Copy link
Member

@alvrs alvrs commented Feb 16, 2023

Prototype for #393

@alvrs alvrs force-pushed the alvrs/world branch 2 times, most recently from a69b823 to 9950bab Compare February 17, 2023 23:35
@alvrs alvrs marked this pull request as ready for review February 17, 2023 23:35
@alvrs alvrs requested review from dk1a and holic February 17, 2023 23:35
packages/store/src/Store.sol Outdated Show resolved Hide resolved
packages/world/package.json Outdated Show resolved Hide resolved
packages/world/remappings.txt Outdated Show resolved Hide resolved
packages/world/src/World.sol Outdated Show resolved Hide resolved
packages/world/src/World.sol Outdated Show resolved Hide resolved
packages/world/src/World.sol Outdated Show resolved Hide resolved
packages/world/src/World.sol Outdated Show resolved Hide resolved
Copy link
Member

@dk1a dk1a left a comment

Choose a reason for hiding this comment

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

lgtm overall, just some nits/questions

Copy link
Member

@holic holic left a comment

Choose a reason for hiding this comment

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

mostly just questions from me!

packages/store/src/StoreView.sol Show resolved Hide resolved
packages/world/foundry.toml Outdated Show resolved Hide resolved
packages/world/hardhat.config.ts Show resolved Hide resolved
packages/world/src/schemas/RouteAccess.sol Outdated Show resolved Hide resolved
packages/world/src/schemas/RouteAccess.sol Outdated Show resolved Hide resolved
packages/world/src/schemas/Address.sol Outdated Show resolved Hide resolved
import { PackedCounter, PackedCounterLib } from "store/PackedCounter.sol";

// -- User defined schema and tableId --
struct AddressToBytes32Schema {
Copy link
Member

Choose a reason for hiding this comment

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

What's the reason for both Address and AddressToBytes32?

Copy link
Member Author

Choose a reason for hiding this comment

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

Address represents mapping(bytes32 => address), AddressToBytes32 represents mapping(address => bytes32). Address is the underlying schema for the OwnerTable (in this case only address can be owner of things), AddressToBytes32 is the underlying schema for the SystemRouteTable to store the routeId of a given system. The purpose of the latter is to check whether a system is already registered in a World. Systems (= contracts) can only be registered once per World, because otherwise it would mess up the access mechanism based on addresses.

packages/world/remappings.txt Outdated Show resolved Hide resolved
packages/world/index.yml Outdated Show resolved Hide resolved
Copy link
Member

@dk1a dk1a left a comment

Choose a reason for hiding this comment

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

lgtm

@alvrs alvrs merged commit c4ea97b into v2 Feb 22, 2023
@@ -9,6 +9,7 @@
"ds-test/=node_modules/ds-test/src/",
"solecs/=node_modules/@latticexyz/solecs/src/",
"royalty-registry/=node_modules/@manifoldxzy/royalty-registry/contracts/",
"std-contracts/=node_modules/@latticexyz/std-contracts/src/"
"std-contracts/=node_modules/@latticexyz/std-contracts/src/",
Copy link
Member

Choose a reason for hiding this comment

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

not blocking for this PR but we should update these usages to use the @latticexyz/std-contracts import format

Copy link
Member

Choose a reason for hiding this comment

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

considering it's a breaking change and not relevant for v2, is it worth it for v1 packages?

Copy link
Member

Choose a reason for hiding this comment

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

we do breaking changes all the time 🙈

) external;

// Register hooks to be called when a record or field is set or deleted
function registerHook(uint256 table, IStoreHook hooks) external;
function registerStoreHook(uint256 table, IStoreHook hooks) external;
Copy link
Member

Choose a reason for hiding this comment

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

this should probably be IStoreHook hook (singular) not IStoreHook hooks?

*/
function getSubslice(bytes memory data, uint256 start) internal pure returns (Slice) {
return getSubslice(data, start, data.length);
}
Copy link
Member

Choose a reason for hiding this comment

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

naming nit: what about just slice?

Copy link
Member

Choose a reason for hiding this comment

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

I named it that way pretty much just to avoid confusion with the Slice type, and make searching for it easier

// "skipDefaultLibCheck": true, /* Skip type checking .d.ts files that are included with TypeScript. */
// "skipLibCheck": true /* Skip type checking all .d.ts files. */
}
// "exclude": ["dist", "**/*.spec.ts", "packages"]
Copy link
Member

Choose a reason for hiding this comment

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

we can prob remove all these comments to make the tsconfig easier to read

@holic holic deleted the alvrs/world branch June 23, 2023 11:37
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.

3 participants