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.

Sign up for GitHub

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

Jump to bottom

fix: light account 1271 signing #963

Merged
merged 5 commits into from
Sep 16, 2024
Merged

fix: light account 1271 signing #963

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
f97ea0f
feat: fix light account 1271 signing
adamegyed Sep 13, 2024
26599c3
feat: fix signature generation for MultiOwnerLightAccount
adamegyed Sep 13, 2024
7b90c69
fix: use real RPC endpoint
adamegyed Sep 13, 2024
eb0f871
Merge branch 'main' into adam/fix-light-account-signing
adamegyed Sep 16, 2024
8bd39a6
chore: remove unneeded todo comment
adamegyed Sep 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 30 additions & 12 deletions aa-sdk/ethers/src/__tests__/provider-adapter.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,15 +1,22 @@
import dotenv from "dotenv";

import { createLightAccount } from "@account-kit/smart-contracts";
import { Wallet } from "@ethersproject/wallet";
import { Alchemy, Network, type AlchemyProvider } from "alchemy-sdk";
import { http } from "viem";
import { polygonMumbai } from "viem/chains";
import { createPublicClient, http, type Hex } from "viem";
import { sepolia } from "viem/chains";
import { EthersProviderAdapter } from "../../src/provider-adapter.js";
import { convertWalletToAccountSigner } from "../utils.js";
dotenv.config();

const endpoint =
process.env.VITEST_SEPOLIA_FORK_URL ??
"https://ethereum-sepolia-rpc.publicnode.com";

describe("Simple Account Tests", async () => {
const alchemy = new Alchemy({
apiKey: "test",
network: Network.MATIC_MUMBAI,
network: Network.ETH_SEPOLIA,
});
// demo mnemonic from viem docs
const dummyMnemonic =
Expand All @@ -19,13 +26,25 @@ describe("Simple Account Tests", async () => {

it("should correctly sign the message", async () => {
const provider = await givenConnectedProvider({ alchemyProvider, signer });
const message = "hello world";
const signature = (await provider.signMessage(message)) as Hex;

// We must use a public client, rather than an account client, to verify the message, because AA-SDK incorrectly attaches the account address as a "from" field to all actions taken by that client, including the `eth_call` used internally by viem's signature verifier logic. Per EIP-684, contract creation reverts on non-zero nonce, and the `eth_call`'s from field implicitly increases the nonce of the account contract, causing the contract creation to revert.
const publicClient = createPublicClient({
chain: sepolia,
transport: http(`${endpoint}`),
});

expect(
await provider.signMessage(
"0xa70d0af2ebb03a44dcd0714a8724f622e3ab876d0aa312f0ee04823285d6fb1b"
)
).toBe(
"0x007ecc361d63ab82d89faeecfb79d40127f376c1ef3d545aeec3578eadce9d0c405a4d1ae6177bdebdc8413065014f735ee98da9643cc0e25c07a7423b694f8ae71b"
);
await publicClient.verifyMessage({
Copy link
Contributor

Choose a reason for hiding this comment

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

I wrote this comment before realizing that the codebase is already doing things the way you did everywhere, so no need to change. But since I already wrote the comment, you get it anyways as a free FYI.

Nit: it's more idiomatic to write this assertion as

await expect(publicClient.verifyMessage({ ... })).resolves.toBe(true)

That way gives a clearer error message in the case where the assertion fails.

Likewise throughout.

address: provider.account.address,
message,
signature,
// todo: This provider doesn't support signMessageWith6492, so we have to manually provider the factory data to the verifier. Add support here for 6492 signature generation.
factory: await provider.account.getFactoryAddress(),
factoryData: await provider.account.getFactoryData(),
})
).toBe(true);
});
});

Expand All @@ -36,17 +55,16 @@ const givenConnectedProvider = async ({
alchemyProvider: AlchemyProvider;
signer: Wallet;
}) => {
const chain = polygonMumbai;
const chain = sepolia;

return EthersProviderAdapter.fromEthersProvider(
alchemyProvider,
chain
).connectToAccount(
await createLightAccount({
chain,
accountAddress: "0x856185aedfab56809e6686d2d6d0c039d615bd9c",
signer: convertWalletToAccountSigner(signer),
transport: http(`${alchemyProvider.connection.url}`),
transport: http(`${endpoint}`),
})
);
};
21 changes: 15 additions & 6 deletions account-kit/smart-contracts/src/light-account/accounts/base.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,15 +139,18 @@ export async function createLightAccountBase<
});
};

const signWith1271WrapperV1 = async (hashedMessage: Hex): Promise<Hex> => {
const signWith1271Wrapper = async (
hashedMessage: Hex,
version: string
): Promise<Hex> => {
return signer.signTypedData({
// EIP712Domain(string name,string version,uint256 chainId,address verifyingContract)
// https://github.com/alchemyplatform/light-account/blob/main/src/LightAccount.sol#L236
domain: {
chainId: Number(client.chain.id),
name: type,
verifyingContract: accountAddress,
version: "1",
version,
},
types: {
LightAccountMessage: [{ name: "message", type: "bytes" }],
Expand Down Expand Up @@ -207,9 +210,12 @@ export async function createLightAccountBase<
case "v1.0.2":
throw new Error(`${type} ${String(version)} doesn't support 1271`);
case "v1.1.0":
return signWith1271WrapperV1(hashMessage(message));
return signWith1271Wrapper(hashMessage(message), "1");
case "v2.0.0":
const signature = await signWith1271WrapperV1(hashMessage(message));
const signature = await signWith1271Wrapper(
hashMessage(message),
"2"
);
// TODO: handle case where signer is an SCA.
return concat([SignatureType.EOA, signature]);
default:
Expand All @@ -227,9 +233,12 @@ export async function createLightAccountBase<
`Version ${String(version)} of LightAccount doesn't support 1271`
);
case "v1.1.0":
return signWith1271WrapperV1(hashTypedData(params));
return signWith1271Wrapper(hashTypedData(params), "1");
case "v2.0.0":
const signature = await signWith1271WrapperV1(hashTypedData(params));
const signature = await signWith1271Wrapper(
hashTypedData(params),
"2"
);
// TODO: handle case where signer is an SCA.
return concat([SignatureType.EOA, signature]);
default:
Expand Down
62 changes: 34 additions & 28 deletions account-kit/smart-contracts/src/light-account/clients/client.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import {
type UserOperationOverrides,
type UserOperationStruct,
} from "@aa-sdk/core";
import { custom, parseEther, type Address } from "viem";
import { custom, parseEther, type Address, publicActions } from "viem";
import { setBalance } from "viem/actions";
import { resetBalance } from "~test/accounts.js";
import { accounts } from "~test/constants.js";
Expand Down Expand Up @@ -65,32 +65,37 @@ describe("Light Account Tests", () => {
);

it.each(versions)("should correctly sign the message", async (version) => {
const { account } = await givenConnectedProvider({
const provider = await givenConnectedProvider({
signer,
version,
});
const message =
"0xa70d0af2ebb03a44dcd0714a8724f622e3ab876d0aa312f0ee04823285d6fb1b";

const { account } = provider;

const message = "hello world";
switch (version) {
case "v1.0.1":
expect(await account.signMessage({ message })).toBe(
"0x53f3169bd14b5ade19a0488bf04ba94a1afa46549014febf3d20d2850a92c98f200c729d723b0592c8b4cb64c0423bb40c5a7fb49f69f4ef5f71ecc672e782e71b"
);
break;
case "v1.0.2":
await expect(account.signMessage({ message })).rejects.toThrowError(
"LightAccount v1.0.2 doesn't support 1271"
);
break;
case "v1.0.1":
case "v1.1.0":
expect(await account.signMessage({ message })).toBe(
"0xddb694a4f22cb929476a39f63aae047c2ccd6a12df17c91ff2068d8679f7d2e57b019263691eae350ce53bc6f958d4f2200571614eab8343af9c382aa90579371c"
);
break;
case "v2.0.0":
expect(await account.signMessage({ message })).toBe(
"0x00823ee659d8f86abaee06bb36c4d84bfefa2938af2835ff62564e9559bc671d0c0e8603a5aa3ed9603b28a137eaf491b64c55ed73007c12efebcfd9a82cb181221b"
);
{
const signature = await account.signMessageWith6492({
message,
});

// We must use a public client, rather than an account client, to verify the message, because AA-SDK incorrectly attaches the account address as a "from" field to all actions taken by that client, including the `eth_call` used internally by viem's signature verifier logic. Per EIP-684, contract creation reverts on non-zero nonce, and the `eth_call`'s from field implicitly increases the nonce of the account contract, causing the contract creation to revert.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this a known thing that we're working on fixing?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

TBD from the clients team. We've raised this as an issue in the past, but I'm not sure what the plan for this is.

expect(
await client.extend(publicActions).verifyMessage({
address: account.address,
message,
signature,
})
).toBe(true);
}
break;
default:
throw new Error(`Unknown version ${version}`);
Expand All @@ -112,25 +117,26 @@ describe("Light Account Tests", () => {
},
} as const;
switch (version) {
case "v1.0.1":
expect(await account.signTypedData(typedData)).toBe(
"0x974c40bb8038696abf26fc9134676b9afd44bcfbe6821acd523193d80a354cfc7f3e8fdc28358c5551231156cec29e1c234ffe9cad09d51d633736d582d997811b"
);
break;
case "v1.0.2":
await expect(account.signTypedData(typedData)).rejects.toThrowError(
"Version v1.0.2 of LightAccount doesn't support 1271"
);
break;
case "v1.1.0":
expect(await account.signTypedData(typedData)).toBe(
"0x7d42ec333209a34a4a64d5bcb40ba67bc8ed3751f30898be60c1eba78d127e0e06b426a1faef69aa683f92aecc6a8e2292689b03e19f41417dcfa875410bd09d1c"
);
break;
case "v1.0.1":
case "v2.0.0":
expect(await account.signTypedData(typedData)).toBe(
"0x00813b860dce6079750e9383e932412620f6bc206261fed24bc450a1c858acb35912eb082c6fe69daf47f662b88941a9f67da425fa7084f2d93eb5bd2d2705905c1b"
);
{
const signature = await account.signTypedDataWith6492(typedData);

// See above comment for context on the duplicate client.
expect(
await client.extend(publicActions).verifyTypedData({
address: account.address,
signature,
...typedData,
})
).toBe(true);
}
break;
default:
throw new Error(`Unknown version ${version}`);
Expand Down
43 changes: 27 additions & 16 deletions account-kit/smart-contracts/src/light-account/clients/multiOwnerLightAccount.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import {
type Address,
type SmartAccountSigner,
} from "@aa-sdk/core";
import { custom, parseEther } from "viem";
import { custom, parseEther, publicActions } from "viem";
import { generatePrivateKey } from "viem/accounts";
import { mine, setBalance } from "viem/actions";
import { accounts } from "~test/constants.js";
Expand Down Expand Up @@ -102,30 +102,41 @@ describe("MultiOwner Light Account Tests", () => {
signer: undeployedSigner,
});

const typedData = {
types: {
Request: [{ name: "hello", type: "string" }],
},
primaryType: "Request",
message: {
hello: "world",
},
} as const;

const signature = await account.signTypedDataWith6492(typedData);

expect(
await account.signTypedDataWith6492({
types: {
Request: [{ name: "hello", type: "string" }],
},
primaryType: "Request",
message: {
hello: "world",
},
await client.extend(publicActions).verifyTypedData({
address: account.address,
signature,
...typedData,
})
).toMatchInlineSnapshot(
`"0x000000000000000000000000000000000019d2ee9f2729a65afe20bb0020aefc000000000000000000000000000000000000000000000000000000000000006000000000000000000000000000000000000000000000000000000000000001200000000000000000000000000000000000000000000000000000000000000084b54c02f20000000000000000000000000000000000000000000000000000000000000040000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000010000000000000000000000008391de4cacfb91f1cf953cf345948d92e137b6b900000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000004200e6821f95b4baff474a8f469467ac7c8f41c916932db259b8c1ca2d58ba327c4116fe018da508b7a0e65646a1c7b0ea6b8718ffbd6e1b184f79c846d817d727941c0000000000000000000000000000000000000000000000000000000000006492649264926492649264926492649264926492649264926492649264926492"`
);
).toBe(true);
});

it("should sign message with 6492 successfully for undeployed account", async () => {
const { account } = await givenConnectedProvider({
signer: undeployedSigner,
});
const message = "test";

const signature = await account.signMessageWith6492({ message });
expect(
await account.signMessageWith6492({ message: "test" })
).toMatchInlineSnapshot(
`"0x000000000000000000000000000000000019d2ee9f2729a65afe20bb0020aefc000000000000000000000000000000000000000000000000000000000000006000000000000000000000000000000000000000000000000000000000000001200000000000000000000000000000000000000000000000000000000000000084b54c02f20000000000000000000000000000000000000000000000000000000000000040000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000010000000000000000000000008391de4cacfb91f1cf953cf345948d92e137b6b90000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000420014415f75bc2032cac0daac2ac988561fbb27e8f936eb730a920bb34003d4c2a23a1e9356aabf1eeda097e8af5f107fe0222742bce6ca57709fe240f9e1bb2a991b0000000000000000000000000000000000000000000000000000000000006492649264926492649264926492649264926492649264926492649264926492"`
);
await client.extend(publicActions).verifyMessage({
address: account.address,
message,
signature,
})
).toBe(true);
});

it("should get on-chain owner addresses successfully", async () => {
Expand Down
Loading