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

Upgrades methods in Account V1 #306

Merged
merged 13 commits into from
Nov 9, 2023
Merged

Conversation

livingrockrises
Copy link
Contributor

@livingrockrises livingrockrises commented Sep 27, 2023

Summary

test script for upgrade
bcnmy/sdk-examples@d4c395e

After we merge this PR, we also need to finalise AddressResolver contract and deploy it on all supported EVM networks. then update the address and abi in the sdk.

Related Issue: # (issue number)

Change Type

  • Bug Fix
  • Refactor
  • New Feature
  • Breaking Change
  • Documentation Update
  • Performance Improvement
  • Other

Checklist

  • My code follows this project's style guidelines
  • I've reviewed my own code
  • I've added comments for any hard-to-understand areas
  • I've updated the documentation if necessary
  • My changes generate no new warnings
  • I've added tests that prove my fix is effective or my feature works
  • All unit tests pass locally with my changes
  • Any dependent changes have been merged and published

Additional Information

Branch Naming

@livingrockrises livingrockrises changed the title M sma 73 development Upgrades methods in Account V1 Sep 27, 2023
@livingrockrises livingrockrises marked this pull request as draft September 27, 2023 18:41
@AmanRaj1608
Copy link
Contributor

branch feat/SMA-73-SA-upgrades also have same code, can delete that branch

@livingrockrises livingrockrises self-assigned this Sep 29, 2023
@livingrockrises livingrockrises marked this pull request as ready for review September 29, 2023 04:54
@bcnmy bcnmy locked and limited conversation to collaborators Nov 7, 2023
@bcnmy bcnmy unlocked this conversation Nov 7, 2023
Comment on lines 180 to 190
if (validationModule.getAddress() === DEFAULT_ECDSA_OWNERSHIP_MODULE) {
const eoaSigner = await validationModule.getSigner();
const eoaAddress = await eoaSigner.getAddress();
const accountAddress = await this.getV1AccountsUpgradedToV2(eoaAddress, index);
Logger.log("account address from V1 ", accountAddress);
if (accountAddress !== ethers.constants.AddressZero) {
return accountAddress;
}
} else {
// can check using module.getAddress() and module.getInitData() for some other auth module
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not keep it generic, take signer and call getAddress with module.getAddress() and module.getInitData()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 am also thinking about more specific methods in the contract and have another review.

like I added this comment
https://github.com/bcnmy/scw-contracts/blob/features/SMA-321-account-address-resolver/contracts/smart-account/utils/AddressResolver.sol#L19C5-L19C76
What do you think @AmanRaj1608 @ankurdubey521

because here we're and will always be only interested in querying possible addresses from V1 factory which have implementation as V2. so why query V2 addresses?

Copy link
Contributor

Choose a reason for hiding this comment

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

I feel both is fine, if removing the query of v2 address can save some time let's remove it.

Copy link
Contributor

@AmanRaj1608 AmanRaj1608 left a comment

Choose a reason for hiding this comment

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

LGTM!

@livingrockrises livingrockrises merged commit dae78fb into develop Nov 9, 2023
2 checks passed
@livingrockrises livingrockrises deleted the m_SMA-73_development branch November 29, 2023 10:06
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