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

fix(cli): support public library methods in modules #3308

Merged
merged 25 commits into from
Oct 24, 2024
Merged
Show file tree
Hide file tree
Changes from 24 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
f4a22ef
Initial attempt to support deploying libraries needed for modules
vdrg Oct 18, 2024
44228a8
Remove console.log
vdrg Oct 18, 2024
fd2cb00
minor changes
vdrg Oct 18, 2024
ba71428
Refactor to do the module library search inside resolveConfig
vdrg Oct 21, 2024
98ac5e9
PR feedback
vdrg Oct 21, 2024
4397e4a
Add comment about path resolution
vdrg Oct 21, 2024
665394c
Add pupet modules to e2e mud config
vdrg Oct 21, 2024
23ad6cf
Use module out dir for finding libraries
vdrg Oct 21, 2024
8e50101
Update e2e worlds.json
vdrg Oct 21, 2024
47fd223
Add erc20 module to e2e tests, local e2e tests working
vdrg Oct 21, 2024
e59d442
Update worlds.json
vdrg Oct 21, 2024
2855cdc
Add erc20 module dev dependency to store-sync, as it needs to be reso…
vdrg Oct 21, 2024
df8b52b
Remove modules to better understand e2e failures
vdrg Oct 22, 2024
d1d4376
Add e2e module test package
vdrg Oct 22, 2024
7ca8bd0
Remove erc20 module from store-sync deps
vdrg Oct 22, 2024
a4c92b6
Add missing dep to module-test
vdrg Oct 22, 2024
f57e8b5
Change anvil port so it doesn't conflict with contracts package tests
vdrg Oct 22, 2024
7b05bf0
Add back missing dep to e2e contracts package
vdrg Oct 22, 2024
b8bf3f5
Move puppet modules test to main test directory, and turn it into a p…
vdrg Oct 22, 2024
860af17
Revert change to worlds.json
vdrg Oct 22, 2024
6605d7a
Force add of .env for puppet-modules test
vdrg Oct 22, 2024
2cbec49
Update gas-report
vdrg Oct 22, 2024
c9320ed
Create gentle-carrots-kneel.md
vdrg Oct 22, 2024
f1e6e2d
Remove puppet module test from changeset
vdrg Oct 22, 2024
dc5ac42
Split changeset in two (one for cli and one for token modules)
vdrg Oct 22, 2024
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
7 changes: 7 additions & 0 deletions .changeset/gentle-carrots-kneel.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
---
"@latticexyz/cli": patch
"@latticexyz/world-module-erc20": patch
"@latticexyz/world-modules": patch
---

World Modules (and contracts imported by them) can now use public library methods.
holic marked this conversation as resolved.
Show resolved Hide resolved
38 changes: 3 additions & 35 deletions e2e/pnpm-lock.yaml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
},
"type": "module",
"scripts": {
"all-build": "for dir in packages/store packages/world packages/world-modules packages/cli test/mock-game-contracts e2e/packages/contracts examples/*/packages/contracts examples/multiple-namespaces templates/*/packages/contracts; do (cd \"$dir\" && pwd && pnpm build); done",
"all-build": "for dir in packages/store packages/world packages/world-modules packages/cli test/mock-game-contracts test/puppet-modules e2e/packages/contracts examples/*/packages/contracts examples/multiple-namespaces templates/*/packages/contracts; do (cd \"$dir\" && pwd && pnpm build); done",
"all-install": "for dir in . docs e2e examples/* templates/*; do (cd \"$dir\" && pwd && pnpm install); done",
"bench": "pnpm run --recursive bench",
"build": "turbo run build --concurrency=100%",
Expand Down
38 changes: 28 additions & 10 deletions packages/cli/src/deploy/resolveConfig.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@ import { groupBy } from "@latticexyz/common/utils";
import { findLibraries } from "./findLibraries";
import { createPrepareDeploy } from "./createPrepareDeploy";
import { World } from "@latticexyz/world";
import { findUp } from "find-up";
import { createRequire } from "node:module";

// TODO: replace this with a manifest/combined config output

Expand All @@ -22,18 +24,34 @@ export async function resolveConfig({
readonly systems: readonly System[];
readonly libraries: readonly Library[];
}> {
const libraries = findLibraries(forgeOutDir).map((library): Library => {
// foundry/solc flattens artifacts, so we just use the path basename
const contractData = getContractData(path.basename(library.path), library.name, forgeOutDir);
return {
path: library.path,
name: library.name,
abi: contractData.abi,
prepareDeploy: createPrepareDeploy(contractData.bytecode, contractData.placeholders),
deployedBytecodeSize: contractData.deployedBytecodeSize,
};
const requirePath = await findUp("package.json");
if (!requirePath) throw new Error("Could not find package.json to import relative to.");
const require = createRequire(requirePath);

const moduleOutDirs = config.modules.flatMap((mod) => {
Copy link
Member

@holic holic Oct 22, 2024

Choose a reason for hiding this comment

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

maybe wrap this in a set so if we install multiple ERC20s, we only need to load the libs once?

Suggested change
const moduleOutDirs = config.modules.flatMap((mod) => {
const moduleOutDirs = new Set(config.modules.flatMap((mod) => {

or ideally, we add support for multiple out dirs in findLibraries that can do this automatically

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah makes sense, I had actually thought of deduplicating but then forgot. Will try it withing findLibraries 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://github.com/latticexyz/mud/pull/3321/files

Let me know if this looks good to you 🙏

if (mod.artifactPath == undefined) {
return [];
}

// Navigate up two dirs to get the contract output directory
vdrg marked this conversation as resolved.
Show resolved Hide resolved
const moduleOutDir = path.join(require.resolve(mod.artifactPath), "../../");
holic marked this conversation as resolved.
Show resolved Hide resolved
return [moduleOutDir];
});

const libraries = [forgeOutDir, ...moduleOutDirs].flatMap((outDir) =>
findLibraries(outDir).map((library): Library => {
// foundry/solc flattens artifacts, so we just use the path basename
const contractData = getContractData(path.basename(library.path), library.name, outDir);
return {
path: library.path,
name: library.name,
abi: contractData.abi,
prepareDeploy: createPrepareDeploy(contractData.bytecode, contractData.placeholders),
deployedBytecodeSize: contractData.deployedBytecodeSize,
};
}),
);

const baseSystemContractData = getContractData("System.sol", "System", forgeOutDir);
const baseSystemFunctions = baseSystemContractData.abi
.filter((item): item is typeof item & { type: "function" } => item.type === "function")
Expand Down
1 change: 1 addition & 0 deletions packages/cli/src/runDeploy.ts
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,7 @@ export async function runDeploy(opts: DeployOptions): Promise<WorldDeploy> {
config,
forgeOutDir: outDir,
});

const artifacts = await findContractArtifacts({ forgeOutDir: outDir });
// TODO: pass artifacts into configToModules (https://github.com/latticexyz/mud/issues/3153)
const modules = await configToModules(config, outDir);
Expand Down
28 changes: 17 additions & 11 deletions packages/cli/src/utils/getContractArtifact.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,18 +11,24 @@ export type GetContractArtifactResult = {
deployedBytecodeSize: number;
};

function isBytecode(value: string): value is Hex {
return isHex(value, { strict: false });
}

const bytecodeSchema = z.object({
object: z.string().refine(isHex),
linkReferences: z.record(
z.record(
z.array(
z.object({
start: z.number(),
length: z.number(),
}),
object: z.string().refine(isBytecode),
linkReferences: z
.record(
z.record(
z.array(
z.object({
start: z.number(),
length: z.number(),
}),
),
),
),
),
)
.optional(),
});

const artifactSchema = z.object({
Expand All @@ -34,7 +40,7 @@ const artifactSchema = z.object({
export function getContractArtifact(artifactJson: unknown): GetContractArtifactResult {
// TODO: improve errors or replace with arktype?
const artifact = artifactSchema.parse(artifactJson);
const placeholders = findPlaceholders(artifact.bytecode.linkReferences);
const placeholders = findPlaceholders(artifact.bytecode.linkReferences ?? {});

return {
abi: artifact.abi,
Expand Down
2 changes: 1 addition & 1 deletion packages/world-module-erc20/gas-report.json
Original file line number Diff line number Diff line change
Expand Up @@ -135,7 +135,7 @@
"file": "test/ERC20Module.t.sol:ERC20ModuleTest",
"test": "testInstall",
"name": "install erc20 module",
"gasUsed": 4524262
"gasUsed": 4523802
},
{
"file": "test/ERC20Pausable.t.sol:ERC20PausableWithInternalStoreTest",
Expand Down
15 changes: 2 additions & 13 deletions packages/world-module-erc20/src/ERC20Module.sol
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,6 @@ contract ERC20Module is Module {
error ERC20Module_InvalidNamespace(bytes14 namespace);
error ERC20Module_NamespaceAlreadyExists(bytes14 namespace);

ERC20RegistryLib public immutable registryLib = new ERC20RegistryLib();

function install(bytes memory encodedArgs) public {
// TODO: we should probably check just for namespace, not for all args
requireNotInstalled(__self, encodedArgs);
Expand All @@ -44,15 +42,15 @@ contract ERC20Module is Module {
// The token should have transferred the namespace ownership to this module in its constructor
world.transferOwnership(namespaceId, _msgSender());

registryLib.delegateRegister(world, namespaceId, address(token));
ERC20RegistryLib.register(world, namespaceId, address(token));
}

function installRoot(bytes memory) public pure {
revert Module_RootInstallNotSupported();
}
}

contract ERC20RegistryLib {
library ERC20RegistryLib {
function register(IBaseWorld world, ResourceId namespaceId, address token) public {
ResourceId erc20RegistryTableId = ModuleConstants.registryTableId();
if (!ResourceIds.getExists(erc20RegistryTableId)) {
Expand All @@ -63,12 +61,3 @@ contract ERC20RegistryLib {
ERC20Registry.set(erc20RegistryTableId, namespaceId, address(token));
}
}

function delegateRegister(ERC20RegistryLib lib, IBaseWorld world, ResourceId namespaceId, address token) {
(bool success, bytes memory returnData) = address(lib).delegatecall(
abi.encodeCall(ERC20RegistryLib.register, (world, namespaceId, token))
);
if (!success) revertWithBytes(returnData);
}

using { delegateRegister } for ERC20RegistryLib;
5 changes: 1 addition & 4 deletions packages/world-module-erc20/test/ERC20Module.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -46,10 +46,7 @@ contract ERC20ModuleTest is Test, GasReporter {
ResourceId erc20NamespaceId = WorldResourceIdLib.encodeNamespace(TestConstants.ERC20_NAMESPACE);

// Token should retain access to the namespace
address token = ERC20Registry.get(
erc20RegistryTableId,
WorldResourceIdLib.encodeNamespace(TestConstants.ERC20_NAMESPACE)
);
address token = ERC20Registry.get(erc20RegistryTableId, erc20NamespaceId);

// Module should grant the token access to the token namespace
assertTrue(ResourceAccess.get(erc20NamespaceId, token), "Token does not have access to its namespace");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,6 @@ import { ERC20Metadata, ERC20MetadataData } from "./tables/ERC20Metadata.sol";
contract ERC20Module is Module {
error ERC20Module_InvalidNamespace(bytes14 namespace);

address immutable registrationLibrary = address(new ERC20ModuleRegistrationLibrary());

function install(bytes memory encodedArgs) public {
// Require the module to not be installed with these args yet
requireNotInstalled(__self, encodedArgs);
Expand All @@ -40,10 +38,7 @@ contract ERC20Module is Module {

// Register the ERC20 tables and system
IBaseWorld world = IBaseWorld(_world());
(bool success, bytes memory returnData) = registrationLibrary.delegatecall(
abi.encodeCall(ERC20ModuleRegistrationLibrary.register, (world, namespace))
);
if (!success) revertWithBytes(returnData);
ERC20ModuleRegistrationLib.register(world, namespace);

// Initialize the Metadata
ERC20Metadata.set(_metadataTableId(namespace), metadata);
Expand All @@ -69,7 +64,7 @@ contract ERC20Module is Module {
}
}

contract ERC20ModuleRegistrationLibrary {
library ERC20ModuleRegistrationLib {
/**
* Register systems and tables for a new ERC20 token in a given namespace
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,6 @@ import { ERC721Metadata, ERC721MetadataData } from "./tables/ERC721Metadata.sol"
contract ERC721Module is Module {
error ERC721Module_InvalidNamespace(bytes14 namespace);

address immutable registrationLibrary = address(new ERC721ModuleRegistrationLibrary());

function install(bytes memory encodedArgs) public {
// Require the module to not be installed with these args yet
requireNotInstalled(__self, encodedArgs);
Expand All @@ -43,10 +41,7 @@ contract ERC721Module is Module {

// Register the ERC721 tables and system
IBaseWorld world = IBaseWorld(_world());
(bool success, bytes memory returnData) = registrationLibrary.delegatecall(
abi.encodeCall(ERC721ModuleRegistrationLibrary.register, (world, namespace))
);
if (!success) revertWithBytes(returnData);
ERC721ModuleRegistrationLib.register(world, namespace);

// Initialize the Metadata
ERC721Metadata.set(_metadataTableId(namespace), metadata);
Expand All @@ -72,7 +67,7 @@ contract ERC721Module is Module {
}
}

contract ERC721ModuleRegistrationLibrary {
library ERC721ModuleRegistrationLib {
/**
* Register systems and tables for a new ERC721 token in a given namespace
*/
Expand Down
27 changes: 27 additions & 0 deletions pnpm-lock.yaml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading
Loading