feat(world): add callFrom entry point #1364

alvrs · 2023-08-25T15:28:58Z

fixes #1358
see changeset for details on this change

changeset-bot · 2023-08-25T15:29:02Z

🦋 Changeset detected

Latest commit: 042893f

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

This PR includes changesets to release 27 packages

Name	Type
@latticexyz/world	Major
@latticexyz/cli	Major
@latticexyz/dev-tools	Major
@latticexyz/store-sync	Major
@latticexyz/store-indexer	Major
@latticexyz/block-logs-stream	Major
@latticexyz/common	Major
@latticexyz/config	Major
create-mud	Major
@latticexyz/ecs-browser	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

holic · 2023-08-25T16:51:06Z

packages/world/src/interfaces/IWorldErrors.sol

@@ -10,4 +10,5 @@ interface IWorldErrors {
  error FunctionSelectorExists(bytes4 functionSelector);
  error FunctionSelectorNotFound(bytes4 functionSelector);
  error ModuleAlreadyInstalled(string module);
+  error NoDelegationFound(address grantor, address grantee);


Suggested change

error NoDelegationFound(address grantor, address grantee);

error DelegationNotFound(address grantor, address grantee);

(for consistency with our other errors)

alvrs · 2023-08-29T16:28:13Z

Did some refactors around Call.withSender and World._call which i'll move to a separate PR. Reason is that it's actually cheaper (and better dev ex) to call the delegation control system via its resource selector instead of storing its address, because then we can use delegatecall for delegation control checks that were registered as root systems, which makes their storage accesses cheaper.

alvrs · 2023-08-30T14:22:32Z

Current dependencies on/for this PR:

main
- PR fix(cli): make mud test exit with code 1 on test error #1371
  - PR refactor(world): separate call utils into WorldContextProvider and SystemCall #1370
    - PR feat(world): add callFrom entry point #1364 👈

This comment was auto-generated by Graphite.

holic · 2023-09-01T06:55:23Z

packages/world/src/modules/core/implementations/WorldRegistrationSystem.sol

+   */
+  function registerDelegation(
+    address delegatee,
+    bytes32 delegationControl,


took me a bit to realize we switched from address to resource selector/ID, and wondering if we should clarify the naming here to delegationControlId or something?

holic · 2023-09-01T06:59:00Z

packages/world/src/modules/std-delegations/DisposableDelegationControl.sol

+        resourceSelector: resourceSelector,
+        funcSelectorAndArgsHash: funcSelectorAndArgsHash,
+        availableCalls: availableCalls - 1
+      });


should we delete once we get to zero?

this can be unchecked, since you do availableCalls > 0 already

holic · 2023-09-01T07:07:33Z

packages/world/src/modules/std-delegations/DisposableDelegationControl.sol

+import { DelegationControl } from "../../DelegationControl.sol";
+import { DisposableDelegations } from "./tables/DisposableDelegations.sol";
+
+contract DisposableDelegationControl is DelegationControl {


I like this better than "decaying" 👍

Another potential name idea is "callbound" (to go with "timebound")

holic · 2023-09-01T07:10:53Z

packages/world/test/StandardDelegationsModule.t.sol

+
+    // Set the timestamp to maxTimestamp+1 and expect the delegation to be expired
+    vm.warp(maxTimestamp + 1);
+    vm.expectRevert(abi.encodeWithSelector(IWorldErrors.DelegationNotFound.selector, delegator, delegatee));


tiny thing but I tend to put the expectRevert immediately before the thing we expect to revert, to not confuse this with "I expect vm.prank to revert"

holic · 2023-09-01T07:14:11Z

packages/world/src/modules/std-delegations/DisposableDelegationControl.sol

@@ -0,0 +1,55 @@
+// SPDX-License-Identifier: MIT


~~thoughts on just delegations instead of std-delegations for the dir name?~~

oh nevermind, this seems to reflect the module name and these files are tied to the module

dk1a · 2023-09-01T08:45:33Z

packages/world/src/modules/std-delegations/DisposableDelegationControl.sol

+        resourceSelector: resourceSelector,
+        funcSelectorAndArgsHash: funcSelectorAndArgsHash,
+        availableCalls: availableCalls - 1
+      });


this can be unchecked, since you do availableCalls > 0 already

dk1a · 2023-09-01T09:04:12Z

packages/world/src/modules/std-delegations/constants.sol

+bytes16 constant MODULE_NAME = bytes16("stddelegations.m");
+
+// Disposable delegation
+bytes32 constant DISPOSABLE_DELEGATION = bytes32(abi.encodePacked(ROOT_NAMESPACE, bytes16("disposable.d")));
+
+// Timebound delegation
+bytes32 constant TIMEBOUND_DELEGATION = bytes32(abi.encodePacked(ROOT_NAMESPACE, bytes16("timebound.d")));


should it perhaps have a MODULE_NAMESPACE?

My thinking was that this module should ideally be installed as a root module (so it can install the systems as root systems), because it reduces the gas cost of all systems that read or change state (around 8k for the calls to DisposableDelegationSystem)

I don't think I have my head wrapped around the end-to-end, but are these delegation/callfrom checks always routed through the world? Ultimately wondering if it would be useful to inherit the namespace or allow the namespace to be configured, or if that only has negative consequences (only used by world, only more gas)

Yes, they are always routed through the World. The main challenge is that the delegation check system needs to know which table to access. We could make this dynamic and store the namespace in which the module is registered in storage and then load it from storage in the system, but that would cost more gas. A cheaper alternative would be to just use two different modules for root and non-root installations (it only needs to be installed once per world, so the namespace for non-root could also be fixed). For now, I'd just go with providing the root version, because I'd expect this standard module to be installed by default - if we find there's a need for a non-root version (to be installed on worlds that don't have the root version installed by default), we can just add another version of the module later

root only for now as the default/expected/most common path is totally fine by me

holic · 2023-09-01T12:04:35Z

packages/world/src/modules/std-delegations/CallboundDelegationControl.sol

+
+    if (availableCalls > 0) {
+      // Decrement the number of available calls
+      unchecked {


I don't know the consequences of unchecked but is it safe for us to wrap this whole statement vs just the decrement? Would unchecked apply to the .set() or _msgSender() calls?

I don't think it would

moved it outside of the block, gas didn't change so seems like unchecked doesn't impact the nested calls, but fells still cleaner to have it explicit

vercel bot deployed to Preview August 25, 2023 15:30 View deployment

holic reviewed Aug 25, 2023

View reviewed changes

vercel bot deployed to Preview August 28, 2023 13:44 View deployment

vercel bot deployed to Preview August 29, 2023 11:29 View deployment

vercel bot deployed to Preview August 29, 2023 16:27 View deployment

alvrs mentioned this pull request Aug 30, 2023

refactor(world): separate call utils into WorldContextProvider and SystemCall #1370

Merged

alvrs force-pushed the alvrs/call-from branch from d0e2456 to d63e159 Compare August 30, 2023 13:17

vercel bot deployed to Preview August 30, 2023 13:18 View deployment

alvrs changed the base branch from main to alvrs/call-utils August 30, 2023 14:12

alvrs force-pushed the alvrs/call-utils branch from 31a033a to 3c13457 Compare August 30, 2023 14:22

alvrs force-pushed the alvrs/call-from branch from d63e159 to 16d858f Compare August 30, 2023 14:22

alvrs mentioned this pull request Aug 30, 2023

ci: pin foundry to fix issues in test running an anvil node #1372

Closed

vercel bot deployed to Preview August 30, 2023 14:24 View deployment

vercel bot deployed to Preview August 30, 2023 15:23 View deployment

alvrs force-pushed the alvrs/call-utils branch from 3c13457 to 79125bc Compare August 30, 2023 15:30

alvrs force-pushed the alvrs/call-from branch from da8d724 to 4cd1bfd Compare August 30, 2023 15:30

alvrs mentioned this pull request Aug 30, 2023

fix(cli): make mud test exit with code 1 on test error #1371

Merged

vercel bot deployed to Preview August 30, 2023 15:32 View deployment

vercel bot deployed to Preview August 30, 2023 15:56 View deployment

vercel bot deployed to Preview August 30, 2023 16:19 View deployment

vercel bot deployed to Preview August 30, 2023 16:24 View deployment

alvrs marked this pull request as ready for review August 30, 2023 16:29

alvrs requested review from dk1a and holic August 31, 2023 11:52

alvrs force-pushed the alvrs/call-utils branch from 79125bc to 70e8c7d Compare August 31, 2023 13:18

alvrs force-pushed the alvrs/call-from branch from 1cd3858 to 1e3071a Compare August 31, 2023 13:18

vercel bot deployed to Preview August 31, 2023 13:22 View deployment

holic reviewed Sep 1, 2023

View reviewed changes

holic previously approved these changes Sep 1, 2023

View reviewed changes

holic reviewed Sep 1, 2023

View reviewed changes

dk1a previously approved these changes Sep 1, 2023

View reviewed changes

alvrs added 4 commits September 1, 2023 13:02

add unchecked

4fced58

change order of expectRevert and prank

48d9828

rename disposable delegation to callbound delegation

6b08f1e

Merge branch 'main' into alvrs/call-from

927e55b

alvrs dismissed stale reviews from dk1a and holic via 927e55b September 1, 2023 11:16

vercel bot deployed to Preview September 1, 2023 11:17 View deployment

remove callbound delegation once availableCall reaches 0

d1bc4d3

vercel bot deployed to Preview September 1, 2023 11:22 View deployment

clarify naming to delegationControlId

ebd0590

vercel bot deployed to Preview September 1, 2023 11:25 View deployment

rebuild artifacts

b124e9d

vercel bot deployed to Preview September 1, 2023 11:52 View deployment

holic reviewed Sep 1, 2023

View reviewed changes

move unchecked

042893f

vercel bot deployed to Preview September 1, 2023 12:54 View deployment

holic approved these changes Sep 1, 2023

View reviewed changes

alvrs changed the title ~~feat: add callFrom entry point~~ feat(world): add callFrom entry point Sep 1, 2023

alvrs merged commit 1ca35e9 into main Sep 1, 2023

alvrs deleted the alvrs/call-from branch September 1, 2023 13:20

github-actions bot mentioned this pull request Sep 1, 2023

Version Packages (next) #1380

Merged

This was referenced Sep 1, 2023

add callFrom entry point / approval system #1124

Closed

Add client support for callFrom #1393

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(world): add callFrom entry point #1364

feat(world): add callFrom entry point #1364

alvrs commented Aug 25, 2023 •

edited

Loading

changeset-bot bot commented Aug 25, 2023 •

edited

Loading

holic Aug 25, 2023

alvrs commented Aug 29, 2023

alvrs commented Aug 30, 2023 •

edited

Loading

holic Sep 1, 2023

holic Sep 1, 2023

dk1a Sep 1, 2023

holic Sep 1, 2023

holic Sep 1, 2023

holic Sep 1, 2023 •

edited

Loading

dk1a Sep 1, 2023

dk1a Sep 1, 2023

alvrs Sep 1, 2023

holic Sep 1, 2023

alvrs Sep 1, 2023

holic Sep 1, 2023

holic Sep 1, 2023 •

edited

Loading

dk1a Sep 1, 2023

alvrs Sep 1, 2023

	error NoDelegationFound(address grantor, address grantee);
	error DelegationNotFound(address grantor, address grantee);

feat(world): add callFrom entry point #1364

feat(world): add callFrom entry point #1364

Conversation

alvrs commented Aug 25, 2023 • edited Loading

changeset-bot bot commented Aug 25, 2023 • edited Loading

🦋 Changeset detected

Choose a reason for hiding this comment

alvrs commented Aug 29, 2023

alvrs commented Aug 30, 2023 • edited Loading

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

holic Sep 1, 2023 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

holic Sep 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alvrs commented Aug 25, 2023 •

edited

Loading

changeset-bot bot commented Aug 25, 2023 •

edited

Loading

alvrs commented Aug 30, 2023 •

edited

Loading

holic Sep 1, 2023 •

edited

Loading

holic Sep 1, 2023 •

edited

Loading