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

feat: add upgrade functionality for light account to msca #298

Merged
merged 7 commits into from
Jan 12, 2024
Merged

feat: add upgrade functionality for light account to msca #298

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
afe0ad4
chore: rebase
avasisht23 Dec 19, 2023
a141a5f
feat: add upgrade functionality for light account to msca
avasisht23 Dec 3, 2023
cb24cf6
feat: rebase
avasisht23 Jan 9, 2024
019f752
feat: rebase
avasisht23 Jan 9, 2024
0c44ef7
feat: rebase
avasisht23 Jan 9, 2024
2b90751
refactor: genericize contract upgrades
moldy530 Jan 10, 2024
4b4b66a
style: apply PR comments
moldy530 Jan 12, 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
8 changes: 4 additions & 4 deletions examples/aa-simple-dapp/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,10 +9,10 @@
"lint": "next lint"
},
"dependencies": {
"@alchemy/aa-accounts": "^1.2.3",
"@alchemy/aa-alchemy": "^1.2.3",
"@alchemy/aa-core": "^1.2.3",
"@alchemy/aa-signers": "^1.2.3",
"@alchemy/aa-accounts": "^1.2.4",
"@alchemy/aa-alchemy": "^1.2.4",
"@alchemy/aa-core": "^1.2.4",
"@alchemy/aa-signers": "^1.2.4",
"@t3-oss/env-core": "^0.7.1",
"@t3-oss/env-nextjs": "^0.7.1",
"magic-sdk": "^21.3.1",
Expand Down
6 changes: 3 additions & 3 deletions examples/alchemy-daapp/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,9 @@
"lint": "next lint"
},
"dependencies": {
"@alchemy/aa-accounts": "^1.2.3",
"@alchemy/aa-alchemy": "^1.2.3",
"@alchemy/aa-core": "^1.2.3",
"@alchemy/aa-accounts": "^1.2.4",
"@alchemy/aa-alchemy": "^1.2.4",
"@alchemy/aa-core": "^1.2.4",
"@chakra-ui/react": "^2.6.1",
"@emotion/react": "^11.11.0",
"@emotion/styled": "^11.11.0",
Expand Down
2 changes: 1 addition & 1 deletion packages/accounts/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@
"test:run-e2e": "vitest run --config vitest.config.e2e.ts"
},
"devDependencies": {
"@alchemy/aa-core": "^1.2.3",
"@alchemy/aa-core": "^1.2.4",
"@wagmi/cli": "^1.5.2",
"change-case": "^5.1.2",
"dedent": "^1.5.1",
Expand Down
61 changes: 58 additions & 3 deletions packages/accounts/src/light-account/account.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,16 +1,18 @@
import {
SimpleSmartContractAccount,
SmartAccountProvider,
type ISmartAccountProvider,
type SignTypedDataParams,
type SmartAccountSigner,
} from "@alchemy/aa-core";
import {
concatHex,
decodeFunctionResult,
encodeFunctionData,
fromHex,
hashMessage,
hashTypedData,
isBytes,
trim,
type Address,
type FallbackTransport,
type Hash,
Expand Down Expand Up @@ -150,6 +152,44 @@ export class LightSmartContractAccount<
return decodedCallResult;
}

encodeUpgradeToAndCall = async (
upgradeToImplAddress: Address,
upgradeToInitData: Hex
): Promise<Hex> => {
const provider = this.rpcProvider;
// Step 1: check if the account we're upgrading is in fact a LightAccount
moldy530 marked this conversation as resolved.
Show resolved Hide resolved
const accountAddress = await this.getAddress();

const storage = await provider.getStorageAt({
address: accountAddress,
slot: "0x360894a13ba1a3210667c828492db98dca3e2076cc3735a920a3ca505d382bbc",
moldy530 marked this conversation as resolved.
Show resolved Hide resolved
});
Copy link
Contributor Author

Choose a reason for hiding this comment

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

wanna leave a comment to humanize the slot and why this is where we look?

Copy link
Collaborator

Choose a reason for hiding this comment

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

ah yea lemme do that


if (storage == null) {
throw new Error("could not get storage");
}

const implementationAddresses = Object.values(LightAccountVersions).map(
(x) => x.implAddress
);

// only upgrade undeployed accounts (storage 0) or deployed light accounts, error otherwise
if (
fromHex(storage, "number") !== 0 &&
implementationAddresses.some((x) => x === trim(storage))
) {
throw new Error(
"could not determine if smart account implementation is light account"
);
}

return encodeFunctionData({
abi: LightAccountAbi,
functionName: "upgradeToAndCall",
args: [upgradeToImplAddress, upgradeToInitData],
});
};

/**
* Encodes the transferOwnership function call using Light Account ABI.
*
Expand All @@ -174,9 +214,10 @@ export class LightSmartContractAccount<
* @returns {Hash} the userOperation hash, or transaction hash if `waitForTxn` is true
*/
static async transferOwnership<
P extends ISmartAccountProvider,
TTransport extends Transport | FallbackTransport = Transport
>(
provider: SmartAccountProvider<TTransport> & {
provider: P & {
account: LightSmartContractAccount<TTransport>;
},
newOwner: SmartAccountSigner,
Expand All @@ -188,7 +229,21 @@ export class LightSmartContractAccount<
data,
});

provider.account.owner = newOwner;
const accountAddress = await provider.getAddress();
const initCode = await provider.account.getInitCode();
provider.connect(
(rpcClient) =>
new LightSmartContractAccount({
rpcClient,
chain: rpcClient.chain,
owner: newOwner,
entryPointAddress: provider.account.getEntryPointAddress(),
factoryAddress: provider.account.getFactoryAddress(),
index: provider.account.index,
initCode,
accountAddress,
Comment on lines +236 to +244
Copy link
Contributor Author

Choose a reason for hiding this comment

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

is there a reason reconnect the provider with the same initCode as the original provider's account? can't we just set the owner to the new owner?

also, do we need to first disconnect at all if we're going to reconnect?

Copy link
Collaborator

Choose a reason for hiding this comment

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

yea I don't know if we do need it since we send a UO anyways so it will get deployed with the only account, but just in case I wanted to keep it here

Copy link
Collaborator

Choose a reason for hiding this comment

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

I was disconnecting first since I wanna make some changes to disconnect to clear out the provider methods that connect adds

})
);

if (waitForTxn) {
return provider.waitForUserOperationTransaction(result.hash);
Expand Down
43 changes: 43 additions & 0 deletions packages/accounts/src/light-account/e2e-tests/light-account.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import {
createLightAccountProvider,
LightSmartContractAccount,
} from "../../index.js";
import { getMSCAUpgradeToData } from "../../msca/utils.js";
import {
API_KEY,
LIGHT_ACCOUNT_OWNER_MNEMONIC,
Expand Down Expand Up @@ -172,6 +173,48 @@ describe("Light Account Tests", () => {
expect(newOwnerViaProvider).not.toBe(oldOwner);
expect(newOwnerViaProvider).toBe(newOwner);
}, 100000);

it("should upgrade a deployed light account to msca successfully", async () => {
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 tested undeployed light account using our gas manager, but now since aa-alchemy optionally depends on aa-accounts, making aa-alchemy a dev dependency of aa-accounts introduces a circular dependency that won't build correctly.

const provider = givenConnectedProvider({
owner,
chain,
});

// create a throwaway address
const throwawayOwner = LocalAccountSigner.privateKeyToAccountSigner(
generatePrivateKey()
);
const throwawayProvider = givenConnectedProvider({
owner: throwawayOwner,
chain,
});

const accountAddress = await throwawayProvider.getAddress();
const ownerAddress = await throwawayOwner.getAddress();

// fund + deploy the throwaway address
await provider.sendTransaction({
from: await provider.getAddress(),
to: accountAddress,
data: "0x",
value: toHex(1000000000000000n),
});

const { connectFn, ...upgradeToData } = await getMSCAUpgradeToData(
Copy link
Contributor Author

@avasisht23 avasisht23 Jan 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

oh this is cool, but do we think we should just make this (upgradeToData) an actual field on the returned object and nest the implAddress + initializationData, or leave them as top-line field? do we think devs would ever want to decouple them?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I decoupled them here because having the connect happen as part of the upgrade call makes typing super hard. so this makes it a bit easier for peeps

throwawayProvider
);

await throwawayProvider.upgradeAccount(upgradeToData, true);

const upgradedProvider = throwawayProvider.connect(connectFn);

const upgradedAccountAddress = await upgradedProvider.getAddress();

const owners = await upgradedProvider.account.readOwners();

expect(upgradedAccountAddress).toBe(accountAddress);
expect(owners).toContain(ownerAddress);
}, 200000);
});

const givenConnectedProvider = ({
Expand Down
Loading
Loading