Skip to content

Conversation

@livingrockrises
Copy link
Contributor

@livingrockrises livingrockrises commented Apr 14, 2025

added PREVALIDATION_HOOKMANAGER_STORAGE_LOCATION

commented for questions in PR file diff view

_tryUninstallValidators();
_tryUninstallExecutors();
_tryUninstallHook(_getHook());
// Review
Copy link
Contributor Author

Choose a reason for hiding this comment

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

should we uninstall pre validation hooks as well?

why try... used instead of regular uninstall methods?

Copy link
Contributor

Choose a reason for hiding this comment

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

Regular uninstall could revert right?

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 noticed we're not posting any data for onUninstall
commented versions of some methods like _tryUninstallValidators had that

Copy link
Contributor

Choose a reason for hiding this comment

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

hmm actually that's a good point. Let keep and use the versions that post data with tryUninstall

Copy link
Contributor Author

Choose a reason for hiding this comment

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

would this need changes in the erc for onRedelegation method--can accept data?

interface IRedelegableDelegatedAccount {
	/*
	 * @dev    Function called before redelegation.
	 *         This function should prepare the account for a delegation to a different implementation.
	 *         This function could be triggered by the new wallet that wants to redelegate an already delegated EOA.
	 *         It should uninitialize storages if needed and execute wallet-specific logic to prepare for redelegation.
	 *         msg.sender should be the owner of the account.
	 */
	function onRedelegation() external returns (bool);
}

Copy link
Contributor Author

@livingrockrises livingrockrises Apr 27, 2025

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah that makes sense

Copy link
Contributor Author

Choose a reason for hiding this comment

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

pushed please check.

@kopy-kat kopy-kat requested a review from highskore April 14, 2025 20:43
Copy link
Contributor

@highskore highskore left a comment

Choose a reason for hiding this comment

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

looks good now, lets just make sure the linter is passing

@livingrockrises livingrockrises requested a review from highskore May 7, 2025 16:07
@livingrockrises
Copy link
Contributor Author

livingrockrises commented May 7, 2025

@highskore
There's an audit comment on Fallback handlers persisting after redelegation.
so I plan to push a commit which maintains an array of fallback selectors besides the current mapping and update install, uninstall method and add a new one _tryUninstallFallbackHandlers()

Screenshot 2025-05-07 at 8 15 08 PM

@highskore
Copy link
Contributor

@highskore

There's an audit comment on Fallback handlers persisting after redelegation.

so I plan to push a commit which maintains an array of fallback selectors besides the current mapping and update install, uninstall method and add a new one _tryUninstallFallbackHandlers()

Screenshot 2025-05-07 at 8 15 08 PM

nice sounds good

@livingrockrises
Copy link
Contributor Author

@highskore
There's an audit comment on Fallback handlers persisting after redelegation.
so I plan to push a commit which maintains an array of fallback selectors besides the current mapping and update install, uninstall method and add a new one _tryUninstallFallbackHandlers()
Screenshot 2025-05-07 at 8 15 08 PM

nice sounds good

pushed.

@livingrockrises
Copy link
Contributor Author

please give me until next week to finalise this PR. I spoke to David and we agreed on not changing the onRedelegation interface now

Copy link

@Rahul-cyber26 Rahul-cyber26 left a comment

Choose a reason for hiding this comment

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

if (isERC7702) {
_addStorageBase(MODULEMANAGER_STORAGE_LOCATION);
_addStorageBase(HOOKMANAGER_STORAGE_LOCATION);
_addStorageBase(PREVALIDATION_HOOKMANAGER_STORAGE_LOCATION);

Choose a reason for hiding this comment

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

        _addStorageBase(PREVALIDATION_HOOKMANAGER_STORAGE_LOCATION);

@@ -1,5 +1,5 @@
[profile.default]
evm_version = "cancun"
evm_version = "prague"

Choose a reason for hiding this comment

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

evm_version = "prague"

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