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

refactor(cli): split up deploy into modules #1384

Merged
merged 22 commits into from
Sep 15, 2023

Conversation

johngrantuk
Copy link
Contributor

Initial refactor of cli deploy script. Should make it easier to do follow up work - using Viem and create2 deployments

  • Added TxHelper class that manages nonce state during deploy/transactions
  • Create separate classes for World/CoreModule/Systems/Modules with independent deploy and install functions
  • Simplify promises/handling

@changeset-bot
Copy link

changeset-bot bot commented Sep 1, 2023

⚠️ No Changeset found

Latest commit: 8ec9e9d

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@johngrantuk johngrantuk changed the title Initial Refactor of cli:deploy refactor(cli:deploy): Initial Refactor of cli deploy Sep 1, 2023
@alvrs
Copy link
Member

alvrs commented Sep 4, 2023

Thank you for picking this up! One high level feedback i have is we try to use utility functions + simple objects instead of classes, because it tends to produce small chunks of code that are easier to reuse and test. (We should probably note this decision with more context on the reasoning down somewhere). Would you be down to refactor the refactor to use a more functional approach?

Copy link
Member

@alvrs alvrs left a comment

Choose a reason for hiding this comment

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

thank you! added some comments. High level feedback: let's try to create lots of small utils and then use them to create higher level utils. I also think it would be nice if each of the utils would live in a file with its name, and utils that belong to the same category are grouped in a folder (eg systems, modules, etc) with an index.ts file to import them from there.

return configModules.filter((module) => !defaultModules.some((m) => m.name === module.name));
}

export async function installModules(
Copy link
Member

Choose a reason for hiding this comment

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

should we create a installModule util that we can use to both install the core module and the user-defined modules?

const { confirmations, worldContract, modules, moduleContracts, tableIds } = input;
// Install modules - non-blocking for tx, await all at end
const modulePromisesNew: Promise<TransactionResponse | TransactionReceipt>[] = [];
for (const module of modules) {
Copy link
Member

Choose a reason for hiding this comment

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

if we have a installModule util we can do something like

const modulePromises = modules.map(installModule);
await Promise.all(modulePromises);

functionArgs: string;
}

export async function registerSystems(
Copy link
Member

Choose a reason for hiding this comment

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

same here, feels like it would be nice to have a small registerSystem and registerFunctionSelector util and then use it to create a small registerSystems util

packages/cli/src/utils/deploy.ts Outdated Show resolved Hide resolved
packages/cli/src/utils/world.ts Outdated Show resolved Hide resolved
@holic holic changed the title refactor(cli:deploy): Initial Refactor of cli deploy refactor(cli): split up deploy into modules Sep 14, 2023
Copy link
Member

@alvrs alvrs left a comment

Choose a reason for hiding this comment

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

So much cleaner! And can't wait for the viem refactor. Added a couple nits and will push a commit to resolve them shortly.

import { loadFunctionSignatures, toFunctionSelector } from "./utils";
import { CallData } from "../utils/types";

export function registerFunctionCalls(input: {
Copy link
Member

Choose a reason for hiding this comment

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

should we call this getRegisterFunctionSelector to make it consistent with the other function names? Or could even rename all to get<MethodName>CallData to make it extra clear (i was surprised for a second that this function doesn't actually register yet but just prepares the call by returning the calldata)

const schemaTypes = Object.values(schema).map((abiOrUserType) => {
const { schemaType } = resolveAbiOrUserType(abiOrUserType, mudConfig);
return schemaType;
// Deploy all contracts - World, Core, Systems, Module. Non-blocking.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// Deploy all contracts - World, Core, Systems, Module. Non-blocking.
// Deploy the World contract

const txConfig = {
...txParams,
signer,
debug: !!debug,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
debug: !!debug,
debug: Boolean(debug),

import { ContractCode } from "../utils/types";

// These modules are always deployed
export const defaultModules: ContractCode[] = [
Copy link
Member

Choose a reason for hiding this comment

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

for consistency with the other variable names

Suggested change
export const defaultModules: ContractCode[] = [
export const defaultModuleContracts: ContractCode[] = [

@holic holic merged commit 76055f4 into latticexyz:main Sep 15, 2023
9 of 10 checks passed
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