feat(world): add viem actions for delegation #2366

tash-2s · 2024-03-04T19:37:37Z

This code has been pulled out from #2314.

By extending viem clients with this actions function after delegation, the delegation is automatically applied to World contract writes. Internally, it transforms the writeContract arguments into one that wraps it with callFrom.

It can be used like this:

walletClient.extend(
  delegation({
    worldAddress,
    delegatorAddress,
    getSystemId: (functionSelector) =>
      network.useStore.getState().getValue(network.tables.FunctionSelectors, { functionSelector })!.systemId,
  }),
);

changeset-bot · 2024-03-04T19:37:41Z

🦋 Changeset detected

Latest commit: 42a802b

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

This PR includes changesets to release 24 packages

Name	Type
@latticexyz/world	Patch
@latticexyz/cli	Patch
@latticexyz/dev-tools	Patch
@latticexyz/store-sync	Patch
@latticexyz/world-modules	Patch
@latticexyz/store-indexer	Patch
@latticexyz/abi-ts	Patch
@latticexyz/block-logs-stream	Patch
@latticexyz/common	Patch
@latticexyz/config	Patch
create-mud	Patch
@latticexyz/faucet	Patch
@latticexyz/gas-report	Patch
@latticexyz/protocol-parser	Patch
@latticexyz/query	Patch
@latticexyz/react	Patch
@latticexyz/recs	Patch
@latticexyz/schema-type	Patch
@latticexyz/services	Patch
solhint-config-mud	Patch
solhint-plugin-mud	Patch
@latticexyz/store	Patch
@latticexyz/utils	Patch
mock-game-contracts	Patch

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

tash-2s · 2024-03-04T19:42:30Z

packages/world/package.json

@@ -11,6 +11,7 @@
  "type": "module",
  "exports": {
    ".": "./dist/ts/index.js",
+    "./actions": "./dist/ts/actions/index.js",


Following the common library's actions path, I've made a new path for this. But we only have this function now, so I can move the function into ./ts/delegationActions.ts if preferred.

tash-2s · 2024-03-04T20:23:20Z

@holic Could you review this when you have a moment?

holic

This is a great first pass!

A few notes around naming, sync vs async interface, and how we're translating between world function selectors and system function selectors.

holic · 2024-03-05T09:24:28Z

packages/world/ts/actions/delegation.ts

+) => Pick<WalletActions<TChain, TAccount>, "writeContract"> {
+  return (client) => ({
+    // Applies to: `client.writeContract`, `getContract(client, ...).write`
+    writeContract: (originalArgs): Promise<WriteContractReturnType> => {


Suggested change

writeContract: (originalArgs): Promise<WriteContractReturnType> => {

writeContract: (writeArgs): Promise<WriteContractReturnType> => {

writeArgs or writeContractArgs might be a little more clear when we're juggling with callFromArgs

Right, writeArgs has more info in the name, thus it is better.

holic · 2024-03-05T09:27:45Z

packages/world/ts/actions/delegation.ts

+type DelegationParameters = {
+  worldAddress: Hex;
+  delegatorAddress: Hex;
+  getSystemId: (functionSelector: Hex) => Hex;


Suggested change

getSystemId: (functionSelector: Hex) => Hex;

getSystemId: (functionSelector: Hex) => Promise<Hex>;

We should probably make this async because in the future 1) our client query APIs will be async and 2) we may wanna automatically retrieve this from the world

holic · 2024-03-05T09:29:17Z

packages/world/ts/actions/delegation.ts

+
+// By extending clients with this function after delegation, the delegation automatically applies
+// to the World contract writes, meaning these calls are made on behalf of the delegator.
+export function delegation<TChain extends Chain, TAccount extends Account>({


Suggested change

export function delegation<TChain extends Chain, TAccount extends Account>({

export function callFrom<TChain extends Chain, TAccount extends Account>({

Since "delegation" is a bigger concept, how about calling this callFrom or delegatedCalls or similar?

The change makes sense!

holic · 2024-03-05T09:29:52Z

packages/world/ts/actions/delegation.ts

+          systemId,
+          functionName: originalArgs.functionName,
+          args: originalArgs.args,
+        } as unknown as SystemCallFrom),


ooc why do we need to cast this?

When wrapping this kind of viem functions that take an abi, we often need this kind of type cast. This is because it requires the actual abi types to provide strong typing. (in this case, to type functionName and args)

E.g., https://github.com/wevm/viem/blob/viem%402.7.19/src/actions/wallet/writeContract.ts#L156-L162

holic · 2024-03-05T09:30:18Z

.changeset/nervous-nails-notice.md

+  delegation({
+    worldAddress,
+    delegatorAddress,
+    getSystemId: (functionSelector) =>


Suggested change

getSystemId: (functionSelector) =>

getSystemId: async (functionSelector) =>

similar as the other comment

holic · 2024-03-05T09:37:19Z

packages/world/ts/actions/delegation.ts

+
+      // `callFrom` requires `systemId`.
+      const functionSelector = toFunctionSelector(formatAbiItem(functionAbiItem));
+      const systemId = getSystemId(functionSelector);


I think we might be missing some nuance around world function selectors vs system function selectors here.

When we call a namespaced system, the originalArgs.functionName will be something like namespace__functionName and the ABI will likely be the IWorld ABI (all of the world functions).

But when we translate that to a system function call, we need to pass in the system ID + system function selector (i.e. just functionName without the namespace, since the namespace is part of the system ID), not the world function selector. If you saw this working before, it could be because the root namespace would mask this nuance (i.e. no namespace prefix for world function selectors).

Maybe getSystemId needs to be something like worldFunctionToSystemFunction or getSystemCall or similar? It takes in a world function selector and returns both the system ID and system function selector. The args will be the same, but we may have to manually encode the args instead of using encodeSystemCallFrom.

I was misunderstanding this point. As you mentioned, it only worked for root namespace systems.

tash-2s

Thank you so much for your comments! 😸 I've addressed all issues.

The callFrom function has been tested with calls for root/non-root systems, with and without call args.

tash-2s · 2024-03-06T04:39:40Z

packages/dev-tools/src/actions/WriteSummary.tsx

-    const functionData = decodeFunctionData({ abi: worldAbi, data: functionSelectorAndArgs });
-    functionName = functionData.functionName;
-    functionArgs = functionData.args;
+
+    // TODO: Since `functionSelectorAndArgs` corresponds to a System's function, decoding it using
+    // the World ABI may not always be successful. For instance, namespaced system calls could
+    // result in an error.
+    try {
+      const functionData = decodeFunctionData({ abi: worldAbi, data: functionSelectorAndArgs });
+      functionName = functionData.functionName;
+      functionArgs = functionData.args;
+    } catch (error) {
+      if (!(error instanceof AbiFunctionSignatureNotFoundError)) {
+        throw error;
+      }
+    }


I've found an error in dev-tools. I've made a workaround, but I'm not sure what's the best way to address this, so leaving this as TODO now.

For root namespaced calls:

For non-root namespaced calls (This was an error before this change):

we could try to do the inverse, where we resolve the system function selector form the world one and assume the same args? but I guess we don't have the system signature in that case

we don't have the system signature

right 🤔

added an issue to track this here: #2382

holic · 2024-03-06T15:38:41Z

packages/world/ts/actions/callFrom.ts

+  worldFunctionToSystemFunction,
+}: CallFromParameters): (
+  client: WalletClient<Transport, TChain, TAccount>,
+) => Pick<WalletActions<TChain, TAccount>, "writeContract"> {


since we already have an assumption here about this being a world with callFrom, we could add an async check that the client account has a proper delegation set up for delegator address and warn if not

We could implement a delegation check here, but would it be useful?

This function should throw an error if the delegation is not set up, and the current implementation throws the error already. Adding a warning message isn't necessary since it would fail with a clear revert message. Also, we shouldn't allow the call to proceed without delegation, as its usage is intended with delegation in place.

makes sense!

holic

had another thought about making this API easier to use by creating a default method for retrieving the system function selectors

curious what you think of the approach?

holic · 2024-03-06T15:47:59Z

packages/world/ts/actions/callFrom.ts

+export function callFrom<TChain extends Chain, TAccount extends Account>({
+  worldAddress,
+  delegatorAddress,
+  worldFunctionToSystemFunction,


thought about this some more and since we already assume you're operating on a world and function selectors never change once set (i.e. we can safely cache them here), I think we can get rid of this option in favor of something like:

type SystemFunction = { readonly systemId: Hex, readonly systemFunctionSelector: Hex }; const worldFunctionSelectorCache = new Map<Hex, SystemFunction>(); async function worldFunctionToSystemFunction(client: PublicClient, worldAddress: Address, worldFunctionSelector: Hex): Promise<SystemFunction> { const cached = worldFunctionSelectorCache.get(worldFunctionSelector); if (cached != null) return cached; const systemFunction = await readContract(client, { address: worldAddress, abi: worldAbi, functionName: 'getRecord', args: [worldConfig.tables.FunctionSelectors.tableId, worldFunctionSelector] }); worldFunctionSelectorCache.set(worldFunctionSelector, systemFunction); return systemFunction; }

(Note that this is pseudocode and may require parsing the record from the getRecord call, and I'm not sure how best to expect the public client, wether we expect to extend a public client or take it as an argument)

Right, it's better to have that automatic retrieving, so users without this info sync can use this function more easily.

Getting a public client as an argument is more flexible, as it doesn't ask users to extend their wallet clients with public actions, so they have options. I'll implement this.

tash-2s · 2024-03-07T01:20:24Z

@holic I've added the default function that reads data from the world contract. Could you take a look? 🥷

…legation-actions

tash-2s · 2024-03-08T01:13:41Z

Regarding the dev-tools issue, thanks to #2392, we can now use FunctionSignatures to display human-readable call / callFrom actions. Since it's an off-chain table, we can use zustand's useStore if provided.

…legation-actions

…to contract call

tash-2s

I've merged main and aligned with the changes in main.

tash-2s · 2024-03-23T23:38:19Z

packages/world/ts/actions/callFrom.ts

+  const table = worldConfig.tables.world__FunctionSelectors;
+
+  const _keySchema = getKeySchema(table);
+  const keySchema = mapObject<typeof _keySchema, { [K in keyof typeof _keySchema]: (typeof _keySchema)[K]["type"] }>(
+    _keySchema,
+    ({ type }) => type,
+  );


This keySchema's type is { worldFunctionSelector: "bytes4" }. Without specifying the generics type argument (using mapObject(_keySchema, ...)), the type results in { worldFunctionSelector: unknown }.

Regarding getting schemas for encoding/decoding, is there a better way to handle it with the new config?

ah, getSchemaTypes

alvrs · 2024-03-25T16:17:49Z

packages/world/package.json

@@ -11,6 +11,7 @@
  "type": "module",
  "exports": {
    ".": "./dist/index.js",
+    "./actions": "./dist/actions.js",


I don't think we need to make this a separate export now that we don't have global side effects on the global export. We can just export this action via /ts/exports/index.ts (or ts/exports/internal.ts for now until we want to make it "public")

alvrs · 2024-03-25T16:31:02Z

packages/world/ts/actions/callFrom.ts

+      // Construct the System's calldata.
+      // If there's no args, use the System's function selector as calldata.
+      // Otherwise, use the World's calldata, replacing the World's function selector with the System's.
+      const systemCalldata =
+        worldCalldata === worldFunctionSelector
+          ? systemFunctionSelector
+          : concat([systemFunctionSelector, slice(worldCalldata, 4)]);


why not always concat([systemFunctionSelector, slice(worldCalldata, 4)]);? if worldCalldata === worldFunctionSelector then that means slice(worldCalldata, 4) is empty, so concat would be a noop, so we don't need to handle it with a special case?

Using const systemCalldata = concat([systemFunctionSelector, slice(worldCalldata, 4)]); could result in an out-of-bounds error in slice() if there's no arg.
(SliceOffsetOutOfBoundsError: Slice starting at offset "4" is out-of-bounds (size: 4).)

I could alternatively use size(worldCalldata) === 4 as a condition. Would that be better?

could we use readHex from @latticexyz/common here? That should handle the case where the start would be out of bounds

Maybe having a helper function that splits the function calldata into selector and argument data would simplify this function a bit.

@alvrs I'll look into readHex. thanks!

alvrs

This is great! just some minor nits, but aside from those i think this is ready to be merged

alvrs · 2024-03-25T16:43:46Z

.changeset/nervous-nails-notice.md

@@ -0,0 +1,23 @@
+---
+"@latticexyz/world": minor


I would opt for not exporting this from world/external yet to test it a bit and bundle the external feature release with a couple other libraries for the wallet connection flow. That means i would declare this as patch for now and add a note to the changeset that it's not ready for "stable" consumption yet.

makes sense!

tash-2s · 2024-03-25T19:00:40Z

@alvrs Thanks for your comments! I've made the function internal and updated the changeset.

world: add ts/actions/delegation.ts

d842193

tash-2s requested review from alvrs, holic and yonadaa as code owners March 4, 2024 19:37

tash-2s commented Mar 4, 2024

View reviewed changes

add changeset

8b41461

vercel bot deployed to Preview March 4, 2024 19:55 View deployment

fix changeset

a49f45c

vercel bot deployed to Preview March 4, 2024 19:59 View deployment

tash-2s mentioned this pull request Mar 4, 2024

Better wallet support #2256

Closed

holic reviewed Mar 5, 2024

View reviewed changes

fix namespaced system call issue and refactor

33ef129

vercel bot deployed to Preview March 6, 2024 04:00 View deployment

tash-2s commented Mar 6, 2024

View reviewed changes

holic reviewed Mar 6, 2024

View reviewed changes

tash-2s added 2 commits March 6, 2024 19:02

make arg optional, retrieve system func info from contract

89d9530

update comment

7c6adb8

vercel bot deployed to Preview March 7, 2024 01:12 View deployment

tash-2s requested a review from holic March 7, 2024 01:18

Merge remote-tracking branch 'origin/main' into tash-2s/viem-world-de…

ad828c9

…legation-actions

vercel bot deployed to Preview March 8, 2024 00:24 View deployment

tash-2s added 2 commits March 7, 2024 18:27

no need to manually decode getRecord result

e100e70

follow rename #2391

46cfec0

vercel bot deployed to Preview March 8, 2024 00:32 View deployment

tash-2s added a commit that referenced this pull request Mar 9, 2024

pull out of #2314 and #2366

1568ada

tash-2s mentioned this pull request Mar 9, 2024

fix(dev-tools): fix callFrom transaction display errors for non-root namespace functions #2399

Open

remove dev-tools diff as it's addressed in #2399

35af4ed

vercel bot deployed to Preview March 9, 2024 05:03 View deployment

tash-2s mentioned this pull request Mar 11, 2024

feat(templates): integrate external wallet into React template #2314

Closed

tash-2s added 5 commits March 23, 2024 10:57

Merge remote-tracking branch 'origin/main' into tash-2s/viem-world-de…

a9b6774

…legation-actions

update to align with tsup config change in main

aa7b881

use new config

762f96c

add caching for worldFunctionToSystemFunction results, in addition …

36ea82b

…to contract call

update comments

8431b65

vercel bot deployed to Preview March 23, 2024 23:15 View deployment

tash-2s commented Mar 23, 2024

View reviewed changes

use getSchemaTypes

af6b3a3

vercel bot deployed to Preview March 24, 2024 14:08 View deployment

alvrs reviewed Mar 25, 2024

View reviewed changes

make callFrom action as internal and document it in changeset

733bdbb

vercel bot deployed to Preview March 25, 2024 19:00 View deployment

refactor: simplify code around system calldata

42a802b

vercel bot deployed to Preview March 25, 2024 19:30 View deployment

alvrs approved these changes Mar 25, 2024

View reviewed changes

alvrs merged commit a09bf25 into main Mar 25, 2024
11 checks passed

alvrs deleted the tash-2s/viem-world-delegation-actions branch March 25, 2024 19:41

	writeContract: (originalArgs): Promise<WriteContractReturnType> => {
	writeContract: (writeArgs): Promise<WriteContractReturnType> => {

	getSystemId: (functionSelector: Hex) => Hex;
	getSystemId: (functionSelector: Hex) => Promise<Hex>;

	export function delegation<TChain extends Chain, TAccount extends Account>({
	export function callFrom<TChain extends Chain, TAccount extends Account>({

	getSystemId: (functionSelector) =>
	getSystemId: async (functionSelector) =>

feat(world): add viem actions for delegation #2366

feat(world): add viem actions for delegation #2366

Conversation

tash-2s commented Mar 4, 2024

changeset-bot bot commented Mar 4, 2024 • edited Loading

🦋 Changeset detected

Choose a reason for hiding this comment

tash-2s commented Mar 4, 2024

holic 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

holic Mar 5, 2024 • 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 Mar 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tash-2s left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

holic Mar 6, 2024 • 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

holic left a comment

Choose a reason for hiding this comment

holic Mar 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tash-2s commented Mar 7, 2024

tash-2s commented Mar 8, 2024

tash-2s left a comment

Choose a reason for hiding this comment

tash-2s Mar 23, 2024 • 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

Choose a reason for hiding this comment

alvrs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tash-2s commented Mar 25, 2024 • edited Loading

changeset-bot bot commented Mar 4, 2024 •

edited

Loading

holic Mar 5, 2024 •

edited

Loading

holic Mar 5, 2024 •

edited

Loading

holic Mar 6, 2024 •

edited

Loading

holic Mar 6, 2024 •

edited

Loading

tash-2s Mar 23, 2024 •

edited

Loading

tash-2s commented Mar 25, 2024 •

edited

Loading