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-modules): add callWithSignature #2592

Merged
merged 14 commits into from
Apr 3, 2024
Merged

Conversation

yonadaa
Copy link
Contributor

@yonadaa yonadaa commented Apr 2, 2024

Replace registerDelegationWithSignature with a more generic callWithSignature

Copy link

changeset-bot bot commented Apr 2, 2024

⚠️ No Changeset found

Latest commit: 5b10bc5

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

@yonadaa yonadaa changed the title feat(world-modules): add callWithSignature feat(world-modules): add callWithSignature Apr 2, 2024
@yonadaa yonadaa marked this pull request as ready for review April 2, 2024 22:38
@yonadaa yonadaa requested review from alvrs and holic as code owners April 2, 2024 22:38
@@ -2,51 +2,46 @@ import { Page } from "@playwright/test";
import { GetContractReturnType, PublicClient, WalletClient } from "viem";
import { AbiParametersToPrimitiveTypes, ExtractAbiFunction, ExtractAbiFunctionNames } from "abitype";

const DelegationAbi = [
const Abi = [
Copy link
Member

Choose a reason for hiding this comment

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

any reason we can't import this from world-modules?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess I didn't think to try it 🤯

{ name: "callData", type: "bytes" },
{ name: "nonce", type: "uint256" },
],
} as const;
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if there's a way we can express this as a struct in solidity then import the ABI here instead of manually defining in both spots

Copy link
Contributor Author

@yonadaa yonadaa Apr 3, 2024

Choose a reason for hiding this comment

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

I agree this would be great, I'm surprised I haven't seen it anyone do it. The problem is we need to derive the EIP712 typehash:

bytes32 constant CALL_TYPEHASH = keccak256("Call(address delegator,bytes32 systemId,bytes callData,uint256 nonce)");

We could define a struct:

struct Call {
  address delegator,
  bytes32 systemId,
  bytes memory callData,
  uint256 nonce
}

But we'd have to somehow get the names and datatypes of each member as strings, then concat them. It's like we need .selector for structs! (then ensure it's included in the ABI)

Maybe we could define a function selector instead? 🤔 I'll keep thinking about it

Copy link
Member

@holic holic Apr 3, 2024

Choose a reason for hiding this comment

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

do we only need the hash in solidity? if so, I think a struct + hash together makes sense and is much closer to ideal than this being defined in multiple places

I think .selector is only bytes4 and not the full bytes32 so a function definition wouldn't help much, but a very cool idea if it could

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see what you mean, see we define it twice, but in the same place? As struct and hash.

How do you include a struct in the ABI though? also we'd need to transform the ABI item into the proper viem type

Copy link
Member

@holic holic Apr 3, 2024

Choose a reason for hiding this comment

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

ahh I think I assumed structs would be part of the ABI but they aren't, unless they're used as a function arg but even then it's an ABI for the function args with an internalType named struct Call (not just Call)

close but not quite, but maybe close enough?

struct Call {
  address delegator,
  bytes32 systemId,
  bytes memory callData,
  uint256 nonce
}

interface ITypedData {
  function Call(Call memory typedData) public view;
}

* @return Return data from the system call.
*/
function callWithSignature(
address delegator,
Copy link
Member

Choose a reason for hiding this comment

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

Wonder if we should call this signer in this context. Delegations are similar but kind of a different concept to this path of calling on behalf of someone else

Suggested change
address delegator,
address signer,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree delegator is confusing in this context. signer is also what OpenZeppelin calls it in the SignatureChecker so i'm good with that

@alvrs alvrs merged commit cd7f1c1 into main Apr 3, 2024
12 checks passed
@alvrs alvrs deleted the yonadaaa/call-with-signature branch April 3, 2024 13:05
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