-
Notifications
You must be signed in to change notification settings - Fork 79
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: transfer_ownership_v2 #488
Conversation
…into feat/transfer_ownership
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm. just some logs. Nice work
size-limit report 📦
|
bundlerUrl: `https://bundler.biconomy.io/api/v2/84532/nJPK7B3ru.dd7f7861-190d-41bd-af80-6877f74b8f44`, | ||
chainId: 84532, | ||
accountAddress: await smartAccount.getAccountAddress() | ||
}) | ||
* ``` | ||
*/ | ||
async transferOwnership( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since you want to modularize it,
I'd say instead of newOwner make it an object called newOwnershipInfo and have eoa pubkey (newOwner) as one of possible args
encodedCall call also be just passed optional which will directly call the chosen module. currently encoded call only builds for transferOwnership(address _newower) which is only applicable for moduleAddress passed as ECDSA or Multi-chain
or we can just let it be as this is. food for thought :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The usecase is only for ECDSA and Multi Chain anyway, I would prefer to let it as it is. If we allow encodedCall as an argument, then anything can be passed to it, this method should only allow to do what it says, a transfer of ownership. Lmk what you think @livingrockrises
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes we can let it be and be precise on the description that module address passed would be ecdsa or multi-chain (any module which has single eoa owner and has transferOwnership method in it)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a type for this argument, to allow only ECDSA and Multichain module address
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reviewed
* feat: gas offset param for sendTransaction and sendUserOp (#474) * feat - gas offset param for sendTransaction * added percentage based gas offsets * added dummyPndOverride * refactor gas offset percentage * refactor names and ts doc + lint * improved tsdoc & fixed comments * refactor sendTransaction example tsdoc comment * refactor sendTransaction test --------- Co-authored-by: GabiDev <gv@popoo.io> * feat: transfer ownership (#484) feat: transfer ownership (#484) * feat: transfer_ownership_v2 (#488) * feat/transfer_ownership(in progress) * cleanup * fix linting * added test for transferOwnership with session key manager module * Fix linting * refactor: refactor based on PR review * improve ts doc + refactor tests * added "moduleAddress" param to transferOwnership() * fix module tests * removed console.logs * fixed lint + removed unused import * remove unused import * added argument type for module address --------- Co-authored-by: GabiDev <gv@popoo.io> --------- Co-authored-by: GabiDev <gv@popoo.io>
* feat: gas offset param for sendTransaction and sendUserOp (#474) * feat - gas offset param for sendTransaction * added percentage based gas offsets * added dummyPndOverride * refactor gas offset percentage * refactor names and ts doc + lint * improved tsdoc & fixed comments * refactor sendTransaction example tsdoc comment * refactor sendTransaction test --------- Co-authored-by: GabiDev <gv@popoo.io> * feat: transfer ownership (#484) feat: transfer ownership (#484) * feat: transfer_ownership_v2 (#488) * feat/transfer_ownership(in progress) * cleanup * fix linting * added test for transferOwnership with session key manager module * Fix linting * refactor: refactor based on PR review * improve ts doc + refactor tests * added "moduleAddress" param to transferOwnership() * fix module tests * removed console.logs * fixed lint + removed unused import * remove unused import * added argument type for module address --------- Co-authored-by: GabiDev <gv@popoo.io> * chore: release v4.3.0 (#491) * release v4.3.0 * refactor: increase timeout for transferOwnership tests --------- Co-authored-by: GabiDev <gv@popoo.io> --------- Co-authored-by: GabiDev <gv@popoo.io> Co-authored-by: Joe Pegler <joepegler123@gmail.com>
Added moduleAddress parameter to transferOwnership() method.
PR-Codex overview
The focus of this PR is to enhance ownership transfer functionality in the smart account module.
Detailed summary
TransferOwnershipCompatibleModule
type for specifying validation modules.