feat(cli,world): add user defined salt in WorldFactory.deployWorld() #2219

partylikeits1983 · 2024-01-31T16:53:44Z

This PR adds user-defined salt functionality to the WorldFactory.deployWorld() function.

Resolves #2214

The modified WorldFactory.deployWorld() function allows the world creator to pass in an arbitrary salt value. This makes it easier for developers to have deterministic addresses across chains.

The edited function still increments worldCounts when creating a new world, but the worldCount nonce is not used to create a unique salt.

The salt value still uses msg.sender as a parameter to generate a salt. If msg.sender calls deployWorld() a second time with the same bytes _salt value, the transaction will revert.

function deployWorld(bytes memory _salt) public returns (address worldAddress) {
    // Deploy a new World and increase the WorldCount
    bytes memory bytecode = type(World).creationCode;
    uint256 salt = uint256(keccak256(abi.encode(msg.sender, _salt)));
    worldCounts[msg.sender]++;
    
    worldAddress = Create2.deploy(bytecode, salt);
    IBaseWorld world = IBaseWorld(worldAddress);

    // Initialize the World and transfer ownership to the caller
    world.initialize(coreModule);
    world.transferOwnership(ROOT_NAMESPACE_ID, msg.sender);

    emit WorldDeployed(worldAddress);
}

This PR also updates the Factories.t.sol test.

In the test, the calculated address matches the deployed address via Create2.

Additionally the test expects revert when attempting to deploy a second world with the same _salt value.

Please let me know if I need to edit the PR.

changeset-bot · 2024-01-31T16:53:47Z

🦋 Changeset detected

Latest commit: c98bd03

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 30 packages

Name	Type
@latticexyz/cli	Major
@latticexyz/world	Major
@latticexyz/dev-tools	Major
@latticexyz/store-sync	Major
@latticexyz/world-modules	Major
@latticexyz/store-indexer	Major
@latticexyz/abi-ts	Major
@latticexyz/block-logs-stream	Major
@latticexyz/common	Major
@latticexyz/config	Major
create-mud	Major
@latticexyz/ecs-browser	Major
@latticexyz/faucet	Major
@latticexyz/gas-report	Major
@latticexyz/network	Major
@latticexyz/noise	Major
@latticexyz/phaserx	Major
@latticexyz/protocol-parser	Major
@latticexyz/react	Major
@latticexyz/recs	Major
@latticexyz/schema-type	Major
@latticexyz/services	Major
@latticexyz/solecs	Major
solhint-config-mud	Major
solhint-plugin-mud	Major
@latticexyz/std-client	Major
@latticexyz/std-contracts	Major
@latticexyz/store-cache	Major
@latticexyz/store	Major
@latticexyz/utils	Major

Not sure what this means? Click here to learn what changesets are.

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

yonadaa

Looking good! Just a couple comments.

packages/world/src/WorldFactory.sol

packages/world/test/Factories.t.sol

packages/world/src/WorldFactory.sol

partylikeits1983 · 2024-01-31T19:42:42Z

packages/world/test/Factories.t.sol

+    testWorldFactory(address(this), 0);
+  }
+
+  function testFuzzWorldDeploy(address account, uint _salt) public {


Added fuzzing test for salt.

You actually don't need a separate testFuzzWorldDeploy test, Foundry will automatically run testWorldFactory as a property-based test because it has parameters.

So we only need testWorldFactory(address account, uint256 salt) and testWorldFactoryGas()

partylikeits1983 · 2024-01-31T19:43:19Z

packages/world/test/Factories.t.sol

-    // Address we expect for World
+    // unchecked for the fuzzing test
+    bytes memory _salt1;
+    unchecked {


Added unchecked here so that the test doesn't revert on overflow/underflow during the fuzzing test.

Is the overall intention here just to generate two different salts?

A more clear way to do this would be to have two salt parameters in the fuzz test, and discard them if the fuzzer generates two that are equal. You can do this with a Foundry assume at the start of the test:

function testWorldFactory(address account, uint256 salt1, uint256 salt2) public { vm.assume(salt1 != salt2); ... )

partylikeits1983

Resolved issues brought up in review.

yonadaa

Almost there, just remove worldCounts and a couple nits 🙌

yonadaa · 2024-02-01T12:21:42Z

packages/world/test/Factories.t.sol

+    testWorldFactory(address(this), 0);
+  }
+
+  function testFuzzWorldDeploy(address account, uint _salt) public {


You actually don't need a separate testFuzzWorldDeploy test, Foundry will automatically run testWorldFactory as a property-based test because it has parameters.

So we only need testWorldFactory(address account, uint256 salt) and testWorldFactoryGas()

packages/world/test/Factories.t.sol

packages/world/src/WorldFactory.sol

yonadaa · 2024-02-01T12:37:23Z

packages/world/test/Factories.t.sol

-    // Address we expect for World
+    // unchecked for the fuzzing test
+    bytes memory _salt1;
+    unchecked {


Is the overall intention here just to generate two different salts?

A more clear way to do this would be to have two salt parameters in the fuzz test, and discard them if the fuzzer generates two that are equal. You can do this with a Foundry assume at the start of the test:

function testWorldFactory(address account, uint256 salt1, uint256 salt2) public { vm.assume(salt1 != salt2); ... )

Co-authored-by: yonada <[email protected]>

partylikeits1983 · 2024-02-02T08:09:09Z

Removed worldCounts from WorldFactory and simplified the testWorldFactory test.

packages/world/src/WorldFactory.sol

holic · 2024-02-02T13:17:24Z

packages/world/src/WorldFactory.sol

   * @return worldAddress The address of the newly deployed World contract.
   */
-  function deployWorld() public returns (address worldAddress) {
+  function deployWorld(bytes memory _salt) public returns (address worldAddress) {


small API thing: because Solidity supports calling functions with named parameters now, it might make sense to call this salt and internally we can call the combined one _salt

yonadaa · 2024-02-02T15:44:48Z

@partylikeits1983 are you good to fix the last suggestion (and get CI to pass)? Otherwise I'm happy to get this over the finish line, but you'll need to enable commits on your branches so I can push to your fork:
https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/allowing-changes-to-a-pull-request-branch-created-from-a-fork

partylikeits1983 · 2024-02-02T18:24:33Z

@holic @yonadaaa lmk if there is anything else to fix on this PR. All tests passed on this branch locally.

partylikeits1983 · 2024-02-02T18:35:11Z

@partylikeits1983 are you good to fix the last suggestion (and get CI to pass)? Otherwise I'm happy to get this over the finish line, but you'll need to enable commits on your branches so I can push to your fork: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/allowing-changes-to-a-pull-request-branch-created-from-a-fork

Edits by maintainers are turned on for this branch.

Is there something specific for repositories with GitHub actions that have secrets?

yonadaa · 2024-02-05T16:04:44Z

Edits by maintainers are turned on for this branch.

@partylikeits1983 thanks for pointing that out, seems my gh CLI was misconfigured but I was able to push by cloning your repo instead.

This PR is almost done - I will take this over and get it over the finish line!

yonadaa · 2024-02-05T17:57:12Z

packages/cli/src/runDeploy.ts

@alvrs @holic should/do we test CLI commands?

For example, we have contract tests for this salt but that doesn't tell us if the CLI option actually works.

we sort of do in that our e2e tests lean on deploying a world with the mud cli (I think)

partylikeits1983 · 2024-02-06T13:47:19Z

Edits by maintainers are turned on for this branch.

@partylikeits1983 thanks for pointing that out, seems my gh CLI was misconfigured but I was able to push by cloning your repo instead.

This PR is almost done - I will take this over and get it over the finish line!

Lmk if there is anything else I should fix.

holic · 2024-02-06T14:16:33Z

packages/cli/src/deploy/deploy.ts

@@ -58,7 +61,7 @@ export async function deploy<configInput extends ConfigInput>({

  const worldDeploy = existingWorldAddress
    ? await getWorldDeploy(client, existingWorldAddress)
-    : await deployWorld(client);
+    : await deployWorld(client, salt ? salt : `0x${randomBytes(32).toString("hex")}`);


we might want to check isHex ~~here~~ (edit: need to check where this is called, not here)

holic · 2024-02-06T14:21:06Z

packages/cli/src/commands/dev-contracts.ts

@@ -89,6 +89,7 @@ const commandModule: CommandModule<typeof devOptions, InferredOptionTypes<typeof
            saveDeployment: true,
            worldAddress,
            srcDir,
+            salt: undefined,


I think it might be nice to do our own internally incrementing counter for this process here, so that restarting the dev process always results in the same contract address.

Nevermind, this might be harder than I initially thought it would be. Since we already pass the worldAddress in to upgrade worlds, this should be fine.

Just a note that this means that our tests that generate data, snapshots, etc. will be non-deterministic for now (unless we update them to pass in a consistent salt)

feat: add user defined salt in WorldFactory.deployWorld()

c3de232

partylikeits1983 requested review from alvrs and holic as code owners January 31, 2024 16:53

partylikeits1983 mentioned this pull request Jan 31, 2024

WorldFactory: allow creator to pass in salt #2214

Closed

yonadaa reviewed Jan 31, 2024

View reviewed changes

packages/world/src/WorldFactory.sol Outdated Show resolved Hide resolved

packages/world/test/Factories.t.sol Outdated Show resolved Hide resolved

yonadaa reviewed Jan 31, 2024

View reviewed changes

packages/world/src/WorldFactory.sol Outdated Show resolved Hide resolved

feat: add user defined salt in WorldFactory.deployWorld()

785fad6

partylikeits1983 commented Jan 31, 2024

View reviewed changes

partylikeits1983 and others added 2 commits January 31, 2024 22:49

fix: fix natspec comment

7f5fe04

Merge branch 'main' into main

33959d2

yonadaa reviewed Feb 1, 2024

View reviewed changes

partylikeits1983 and others added 2 commits February 2, 2024 10:30

Update packages/world/test/Factories.t.sol

e898b5f

Co-authored-by: yonada <[email protected]>

feat: rm worldCounts from WorldFactory

5d6a95c

Merge branch 'main' into main

ce5ed96

holic reviewed Feb 2, 2024

View reviewed changes

packages/world/src/WorldFactory.sol Outdated Show resolved Hide resolved

Update packages/world/src/WorldFactory.sol

5a07e43

holic reviewed Feb 2, 2024

View reviewed changes

fix: rename _salt to salt

0bc3703

partylikeits1983 and others added 3 commits February 3, 2024 11:09

feat: update deployWorld.ts script

e3dd8c5

Merge branch 'main' into main

0bb5e91

Merge remote-tracking branch 'mud/main'

148e2f0

fix: viem hex salt

e15b57a

yonadaa added 6 commits February 5, 2024 16:24

chore: gas report

b35ecb4

feat: random salt

f297afd

refactor: iworldfactory interface

d930d2f

refactor: spacing

fb0fc34

feat: salt cli option

7d2e8a7

test: fix dev-contracts

59df327

yonadaa requested a review from holic February 5, 2024 17:55

yonadaa reviewed Feb 5, 2024

View reviewed changes

holic changed the title ~~feat: add user defined salt in WorldFactory.deployWorld()~~ feat(cli,world): add user defined salt in WorldFactory.deployWorld() Feb 6, 2024

holic reviewed Feb 6, 2024

View reviewed changes

Create happy-snails-sleep.md

3a70a1e

holic reviewed Feb 6, 2024

View reviewed changes

holic added 3 commits February 6, 2024 14:39

check that we have a salt hex

eb7db5d

remove unused import

0806e53

make salt consistent in dev-contracts

c98bd03

yonadaa approved these changes Feb 6, 2024

View reviewed changes

holic merged commit 618dd0e into latticexyz:main Feb 6, 2024
10 of 11 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(cli,world): add user defined salt in WorldFactory.deployWorld() #2219

feat(cli,world): add user defined salt in WorldFactory.deployWorld() #2219

partylikeits1983 commented Jan 31, 2024 •

edited

Loading

changeset-bot bot commented Jan 31, 2024 •

edited

Loading

yonadaa left a comment

partylikeits1983 Jan 31, 2024

yonadaa Feb 1, 2024

partylikeits1983 Jan 31, 2024

yonadaa Feb 1, 2024

partylikeits1983 left a comment

yonadaa left a comment

yonadaa Feb 1, 2024

yonadaa Feb 1, 2024

partylikeits1983 commented Feb 2, 2024 •

edited

Loading

holic Feb 2, 2024

partylikeits1983 Feb 2, 2024

yonadaa commented Feb 2, 2024 •

edited

Loading

partylikeits1983 commented Feb 2, 2024

partylikeits1983 commented Feb 2, 2024

yonadaa commented Feb 5, 2024

yonadaa Feb 5, 2024 •

edited

Loading

holic Feb 6, 2024

partylikeits1983 commented Feb 6, 2024

holic Feb 6, 2024 •

edited

Loading

holic Feb 6, 2024

holic Feb 6, 2024 •

edited

Loading

feat(cli,world): add user defined salt in WorldFactory.deployWorld() #2219

feat(cli,world): add user defined salt in WorldFactory.deployWorld() #2219

Conversation

partylikeits1983 commented Jan 31, 2024 • edited Loading

changeset-bot bot commented Jan 31, 2024 • edited Loading

🦋 Changeset detected

yonadaa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

partylikeits1983 left a comment

Choose a reason for hiding this comment

yonadaa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

partylikeits1983 commented Feb 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yonadaa commented Feb 2, 2024 • edited Loading

partylikeits1983 commented Feb 2, 2024

partylikeits1983 commented Feb 2, 2024

yonadaa commented Feb 5, 2024

yonadaa Feb 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

partylikeits1983 commented Feb 6, 2024

holic Feb 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

holic Feb 6, 2024 • edited Loading

Choose a reason for hiding this comment

partylikeits1983 commented Jan 31, 2024 •

edited

Loading

changeset-bot bot commented Jan 31, 2024 •

edited

Loading

partylikeits1983 commented Feb 2, 2024 •

edited

Loading

yonadaa commented Feb 2, 2024 •

edited

Loading

yonadaa Feb 5, 2024 •

edited

Loading

holic Feb 6, 2024 •

edited

Loading

holic Feb 6, 2024 •

edited

Loading