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: add upgrade functionality for light account to msca #298

Merged

Conversation

avasisht23
Copy link
Contributor

@avasisht23 avasisht23 commented Dec 3, 2023

Pull Request Checklist

  • Did you add new tests and confirm existing tests pass? (yarn test)
  • Did you update relevant docs? (docs are found in the site folder, and guidelines for updating/adding docs can be found in the contribution guide)
  • Do your commits follow the Conventional Commits standard?
  • Does your PR title also follow the Conventional Commits standard?
  • If you have a breaking change, is it correctly reflected in your commit message? (e.g. feat!: breaking change)
  • Did you run lint (yarn lint:check) and fix any issues? (yarn lint:fix)
  • Is the base branch you're merging into development and not main?
  • Did you follow the contribution guidelines?

PR-Codex overview

Detailed summary

  • Updated @alchemy/aa-core dependency to version 1.2.4 in multiple packages.
  • Added encodeUpgradeToAndCall method to ISmartContractAccount interface.
  • Added upgradeAccount method to ISmartAccountProvider interface.
  • Added implementation of encodeUpgradeToAndCall method to LightSmartContractAccount class.
  • Added implementation of upgradeAccount method to SmartAccountProvider class.
  • Added tests for upgrading a deployed light account to MSCA.

The following files were skipped due to too many changes: packages/accounts/src/msca/utils.ts, packages/accounts/src/msca/abis/UpgradeableModularAccount.ts

✨ Ask PR-Codex anything about this PR by commenting with /codex {your question}

@avasisht23
Copy link
Contributor Author

avasisht23 commented Dec 3, 2023

@avasisht23 avasisht23 force-pushed the 12-02-feat_add_upgrade_functionality_for_light_account_to_msca branch from a09a936 to 67db9b9 Compare December 3, 2023 02:04
@@ -195,6 +200,52 @@ 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.

@avasisht23 avasisht23 force-pushed the 12-02-feat_add_upgrade_functionality_for_light_account_to_msca branch from 67db9b9 to 5926ddc Compare December 3, 2023 02:12
@avasisht23 avasisht23 force-pushed the 12-02-feat_add_upgrade_functionality_for_light_account_to_msca branch from 5926ddc to 71fc692 Compare December 4, 2023 19:42
Base automatically changed from msca-base to main January 8, 2024 18:06
@avasisht23 avasisht23 force-pushed the 12-02-feat_add_upgrade_functionality_for_light_account_to_msca branch from 71fc692 to 24fddf7 Compare January 9, 2024 01:13
@moldy530 moldy530 force-pushed the 12-02-feat_add_upgrade_functionality_for_light_account_to_msca branch 6 times, most recently from 2cddc7e to 922486f Compare January 10, 2024 21:03
@@ -16,7 +16,7 @@ export interface IMSCA<

extendWithPluginMethods: <AD, PD>(
plugin: Plugin<AD, PD>
) => IMSCA<TTransport, TProviderDecorators & PD> & AD;
) => this & IMSCA<TTransport, TProviderDecorators & PD> & AD;
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 this necessary? why?

Copy link
Collaborator

Choose a reason for hiding this comment

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

yea, so before if you didn't include it then if you previously extended the account you would lose them on the second extend.

If you try it out on the main branch right now, you can see that happening in the multi owner builder function. Once you extend with TokenReceiver, the methods for MultiOwner no longer resolve

@moldy530 moldy530 force-pushed the 12-02-feat_add_upgrade_functionality_for_light_account_to_msca branch 3 times, most recently from cb33f09 to e91fc85 Compare January 10, 2024 23:41
@moldy530 moldy530 marked this pull request as ready for review January 10, 2024 23:41
@moldy530 moldy530 force-pushed the 12-02-feat_add_upgrade_functionality_for_light_account_to_msca branch 3 times, most recently from 70480cb to b488adf Compare January 11, 2024 19:41
@moldy530 moldy530 changed the base branch from main to moldy/fix-la-1271 January 11, 2024 19:41
@moldy530 moldy530 force-pushed the 12-02-feat_add_upgrade_functionality_for_light_account_to_msca branch from b488adf to 461d54a Compare January 11, 2024 19:44
@moldy530 moldy530 force-pushed the 12-02-feat_add_upgrade_functionality_for_light_account_to_msca branch from 461d54a to 45913e5 Compare January 11, 2024 20:08
@moldy530 moldy530 force-pushed the 12-02-feat_add_upgrade_functionality_for_light_account_to_msca branch from 45913e5 to a3773dd Compare January 11, 2024 20:09
@moldy530 moldy530 force-pushed the 12-02-feat_add_upgrade_functionality_for_light_account_to_msca branch from a3773dd to 1c02c4c Compare January 11, 2024 20:17
@moldy530 moldy530 force-pushed the moldy/fix-la-1271 branch 3 times, most recently from 22020f4 to 2ebec5d Compare January 12, 2024 16:49
@moldy530 moldy530 force-pushed the 12-02-feat_add_upgrade_functionality_for_light_account_to_msca branch from 1c02c4c to 53f6a9f Compare January 12, 2024 16:51
Base automatically changed from moldy/fix-la-1271 to main January 12, 2024 18:29
@moldy530 moldy530 force-pushed the 12-02-feat_add_upgrade_functionality_for_light_account_to_msca branch from 53f6a9f to 2b90751 Compare January 12, 2024 18:30
Copy link
Contributor Author

@avasisht23 avasisht23 left a comment

Choose a reason for hiding this comment

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

some open questions, but overall lgtm. looks like you can approve it when you make any necessary changes since it was my og pr haha.

Comment on lines 163 to 166
const storage = await provider.getStorageAt({
address: accountAddress,
slot: "0x360894a13ba1a3210667c828492db98dca3e2076cc3735a920a3ca505d382bbc",
});
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

packages/accounts/src/light-account/account.ts Outdated Show resolved Hide resolved
Comment on lines +236 to +244
new LightSmartContractAccount({
rpcClient,
chain: rpcClient.chain,
owner: newOwner,
entryPointAddress: provider.account.getEntryPointAddress(),
factoryAddress: provider.account.getFactoryAddress(),
index: provider.account.index,
initCode,
accountAddress,
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

packages/accounts/src/msca/utils.ts Outdated Show resolved Hide resolved
value: toHex(1000000000000000n),
});

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

@avasisht23 avasisht23 Jan 12, 2024

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

Co-authored-by: Ajay Vasisht <43521356+avasisht23@users.noreply.github.com>
@moldy530 moldy530 merged commit 18f51d9 into main Jan 12, 2024
2 checks passed
@moldy530 moldy530 deleted the 12-02-feat_add_upgrade_functionality_for_light_account_to_msca branch January 12, 2024 21:17
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.

2 participants