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 support for modules, move logic to RegistrationModule #482

Merged
merged 13 commits into from
Mar 14, 2023

Conversation

alvrs
Copy link
Member

@alvrs alvrs commented Mar 13, 2023

This PR adds support for "Modules" (see #393 for the original concept).
Modules encapsulate installation logic for coupled pieces of logic/data (see RegistrationModule as an example).

This PR also moves out all logic and tables related to "registration" to a RegistrationModule and the initialization of a couple core tables to CoreModule. This is mainly to reduce the bytecode size of World, which recently passed the contract size limit. With these changes we're well below the limit again.

@alvrs alvrs marked this pull request as ready for review March 13, 2023 15:58
@alvrs alvrs requested a review from holic as a code owner March 13, 2023 15:58
@alvrs alvrs requested a review from dk1a March 13, 2023 15:59
@alvrs alvrs self-assigned this Mar 13, 2023
holic
holic previously approved these changes Mar 13, 2023
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.

very cool to see this pattern in action! nothing blocking from a review standpoint, mostly questions/thoughts

@@ -237,6 +237,8 @@ export async function deploy(mudConfig: MUDConfig, deployConfig: DeployConfig):
throw new MUDError(
`Error deploying ${contractName}: invalid bytecode. Note that linking of public libraries is not supported yet, make sure none of your libraries use "external" functions.`
);
} else if (error?.message.includes("CreateContractLimit")) {
throw new MUDError(`Error deploying ${contractName}: CreateContractLimit exceeded.`);
Copy link
Member

Choose a reason for hiding this comment

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

no changes needed here, but we should be sure to not throw away the original error in case its useful (today or in the future) for debugging

viem has a good example of wrapping errors with nicer messages: https://github.com/wagmi-dev/viem/blob/main/src/errors/contract.ts

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed! In this case the original error message includes the entire bytecode, so it makes your terminal explode and buries the actual error message, but we could add a CLI flag (sth like --debug) to print the full error message too

Copy link
Member Author

Choose a reason for hiding this comment

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

(will leave that to a follow PR to not bloat this one too much)

packages/world/src/WithMsgSender.sol Outdated Show resolved Hide resolved
@@ -114,7 +117,9 @@ contract WorldTest is Test {

function setUp() public {
world = IWorld(address(new World()));
world.initialize();
world.installRootModule(new CoreModule());
world.installRootModule(new RegistrationModule());
Copy link
Member

Choose a reason for hiding this comment

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

this is very cool!

@alvrs alvrs changed the base branch from alvrs/fallback to main March 14, 2023 01:10
@alvrs alvrs dismissed holic’s stale review March 14, 2023 01:10

The base branch was changed.

@alvrs
Copy link
Member Author

alvrs commented Mar 14, 2023

// Require the CoreModule to be installed in the root namespace
if (InstalledModules.get(ROOT_NAMESPACE, CORE_MODULE_NAME).moduleAddress == address(0)) {
revert RequiredModuleNotFound(ResourceSelector.from(ROOT_NAMESPACE, CORE_MODULE_NAME).toString());
}
Copy link
Member

Choose a reason for hiding this comment

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

we're so close to having a real package system hahaha

@@ -60,12 +64,20 @@ export async function deploy(mudConfig: MUDConfig, deployConfig: DeployConfig):
World: worldContractName
? deployContractByName(worldContractName)
: deployContract(WorldABI, WorldBytecode, "World"),
CoreModule: deployContract(CoreModuleData.abi, CoreModuleData.bytecode, "CoreModule"),
RegistrationModule: deployContract(RegistrationModuleData.abi, RegistrationModuleData.bytecode, "RootModule"),
Copy link
Member

Choose a reason for hiding this comment

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

is this intentionally called RootModule?

Copy link
Member Author

Choose a reason for hiding this comment

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

it's not, good catch!

Copy link
Member

Choose a reason for hiding this comment

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

curious what this would have affected if left uncaught?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's just used for logging, the logs would have said "Deploying RootModule"

// TODO: this is a workaround to use `getRecord` instead of `getField` in the autogen library,
// to allow using the table before it is registered. This is because `getRecord` passes the schema
// to store, while `getField` loads it from storage. Remove this once we have support for passing the
// schema in `getField` too.
Copy link
Member

Choose a reason for hiding this comment

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

can we add this to the tracker?

@alvrs alvrs merged commit 624cbbc into main Mar 14, 2023
@holic holic deleted the alvrs/modules branch June 23, 2023 11:36
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.

2 participants